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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:413
+    if (Obj.StrTbl.ContentSize &&
+        *Obj.StrTbl.ContentSize > Obj.StrTbl.RawContent->binary_size())
+      W.OS.write_zeros(*Obj.StrTbl.ContentSize -
----------------
I think this second clause can be an `assert` right? We've already reported an error if the ContentSize is less than the RawContent size, and I believe `write_zeros` is harmless to call with a zero size.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:420
+  size_t StrTblBuilderSize = StrTblBuilder.getSize();
+  // Write the string table directly if no length value is specified.
+  if (!Obj.StrTbl.Length && !Obj.StrTbl.ContentSize) {
----------------
I'm not sure why you're doing this. `Length` should be optional and a default value generated for it, based on other specified fields, if an explicit one is not specified.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:433
+  StrTblBuilder.write(Ptr);
+  // Replace the first 4 bytes with the specified length value.
+  memset(Ptr, 0, 4);
----------------
Unless I'm missing something, this doesn't seem right. You need 4 bytes for the Length field, and then the entire string table contents. The Length field doesn't overwrite the first four bytes worth of strings...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107421