[PATCH] D107421: [yaml2obj][XCOFF] Customize the string table.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:63-68
+  uint32_t TableSize = 0; // The actual size of the string table, including the
+                          // size of the length field.
+  uint32_t Length = 0; // The value of the length field for the first 4 bytes of
+                       // the table.
+  std::vector<StringRef> Strings;
+  yaml::BinaryRef RawContent;
----------------
These fields probably all want to be Optional values. You need to be able to distinguish the cases where a field has a value of zero/is an empty array/is an empty string from where the field is missing. This is often important for the error case handling, if nothing else.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:149
+  size_t RawContentSize = Obj.StrTbl.RawContent.binary_size();
+  if (Obj.StrTbl.TableSize && Obj.StrTbl.TableSize < (RawContentSize + 4)) {
+    ErrHandler("specified TableSize (" + Twine(Obj.StrTbl.TableSize) +
----------------
You don't need additional space for the Length field if you are using RawContent. If RawContent is used, then you should just have that content, plus some possible trailing padding if TableSize is greater than than the size of the RawContent.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:157
+    if (NumOfStrings || Obj.StrTbl.Length) {
+      ErrHandler("it's not allowed to specify Strings or Length when "
+                 "RawContent is specified");
----------------
This is shorter.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:397
+void XCOFFWriter::writeStringTable() {
+  size_t RawTableSize = Obj.StrTbl.RawContent.binary_size() + 4;
+  if (RawTableSize > 4) {
----------------
As noted above - the RawContent should be just that (plus possible trailing padding for a large TableSize). This `+ 4` business seems therefore incorrect to me.

I'd expect the code to look something like this:
```
if(Obj.StrTbl.RawContent) {
  Obj.StrTbl.RawContent.writeAsBinary(W.OS);
  if (Obj.StrTbl.TableSize && *Obj.StrTbl.TableSize > Obj.StrTbl.RawContent.size())
    W.OS.write_zeros(Obj.StrTbl.TableSize - Obj.StrTble.RawContent.size());
  return;
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107421/new/

https://reviews.llvm.org/D107421