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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:811
   uint32_t Size = support::endian::read32be(Obj->base() + Offset);
+  printf("Obj->base() + Offset: %lu\n", Offset);
+
----------------
Remove debug printing...


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:422
+  size_t StrTblBuilderSize = StrTblBuilder.getSize();
+  // Write the string table directly if no length value is specified.
+  if (!Obj.StrTbl.Length && !Obj.StrTbl.ContentSize) {
----------------
As suggested already, I recommend extending this comment to say why you can just write it (i.e. the Length field is already included).

Also, when referring to field names, I'd make the name capitalized, i.e. "Length", to match the name in the YAML.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:445
+    assert(*Obj.StrTbl.ContentSize >= StrTblBuilderSize &&
+           "Specified ContentSize is less than the StringTableB size.");
+    W.OS.write_zeros(*Obj.StrTbl.ContentSize - StrTblBuilderSize);
----------------
Fix this message (you seem to have caused it to regress since your previous diff).


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:75
+## names in string table) will be overwritten by specified values. Cases 7-9
+## are trying to check this function. Notice that we don't specify length
+## value in those cases, and the length value is implicitly generated.
----------------
Similar comment to elsewhere. When referring to the `Length` YAML field, use a capital letter. Applies here and below.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:291
+
+# CASE19: 00 00 00 14
----------------
Probably also dump the bytes that refer to the name, to show that this is the actual string table and not some other abitrariy block where we happen to have written the length value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107421