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

Wed Sep 8 00:49:31 PDT 2021

jhenderson added inline comments.

================
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) {
----------------
Esme wrote:
> jhenderson wrote:
> > 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.
> If neither Length nor ContentSize is specified, we follow the original code logic, i.e. use `StrTblBuilder.write(W .OS)` to write the whole table directly with a calculated default length value.
> If Length or ContentSize is specified, we have to serialize the string table and rewrite the length field manually (as Higuoxing's comment).
> Since the latter approach is more time-consuming, I am doing this here to determine in advance whether we can write the string table directly.
Ah, thanks. I missed that bit.

It probably would be helpful to extend the comment to say something to the effect that the Length field is written automatically by the StrTblBuilder.write code.

================
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);
----------------
Esme wrote:
> jhenderson wrote:
> > 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...
> `StrTblBuilder.write(Ptr);` writes the whole content of string table (including the length field) into the buffer and we don't want to change the interface of StringTableBuilder, that's why I have to rewrite the length field here.
> ```
> void StringTableBuilder::write(uint8_t *Buf) const {
> ...
>   else if (K == XCOFF)
>     support::endian::write32be(Buf, Size);
> }
> ```
Thanks. Perhaps update the comment to say "Replace the first 4 bytes, which contain the auto-generated Length value, with the specified value."

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:52
+##         the content size.
+# RUN: not yaml2obj --docnum=1 %s -DSYMNAME='nameInStrTbl' -DCONTENTSIZE=10 \
+# RUN:   -o %t4 2>&1 | FileCheck %s --check-prefix=CASE4
----------------
Perhaps test the range edge, by using CONTENTSIZE == actual content size - 1.

It would also be a good idea to have an explicit ContentSize that matches the actual size.

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:88
+
+## Case 6: if the number of `Strings` is greater than default strings, all
+##         default strings will be overwritten by specified ones.
----------------

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:128
+
+## Case 7: if the number of `Strings` is less than default strings, default
+##         strings will be partially overwritten by specified ones, and the
----------------

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:129
+## Case 7: if the number of `Strings` is less than default strings, default
+##         strings will be partially overwritten by specified ones, and the
+##         unoverwritten strings will still be stored after specified strings
----------------

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:130
+##         strings will be partially overwritten by specified ones, and the
+##         unoverwritten strings will still be stored after specified strings
+##         in the string table.
----------------
As said elsewhere, "unoverwritten" isn't a word.

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:173
+## We can specify both `ContentSize` and `Strings` when the length value is equal
+## to or greater than the content size. Case 8-11 are trying to check this.
+
----------------

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:208
+
+# CASE10: yaml2obj: error: specified ContentSize (10) is less than the size of the data that would otherwise be written (22)
+
----------------
Up to you, but you can probably omit the "yaml2obj:" bit of these error message checks (same throughout).

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:236
+## Case 13: an error is reported when `ContentSize` is less than the `RawContent`
+##          `RawContent` data size.
+# RUN: not yaml2obj --docnum=1 %s -DRAWCONTENT="000000090062006300" -DCONTENTSIZE=6 \
----------------
Remove duplicate word.

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:242-243
+
+## Case 14: if `RawContent` is specified and `ContentSize` is greater than the
+##          `RawContent` data size. Pad the RawContent with trailing zeroes.
+# RUN: yaml2obj --docnum=1 %s -DRAWCONTENT="000000090062006300" -DCONTENTSIZE=11 -o %t14
----------------

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:253
+
+## Case 15: report an error when the string table is not null terminated.
+# RUN: yaml2obj --docnum=1 %s -DRAWCONTENT="000000090062000063" -o %t15
----------------
This is an llvm-readobj test case, not a yaml2obj test case. Please remove it from this patch.

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:269
+
+## Case 17: use the value of the `Length` field for the first 4 bytes of the table.
+# RUN: yaml2obj --docnum=1 %s -DSYMNAME='nameInStrTbl' -DSYMNAME2='name2InStrTbl' \
----------------
Probably point out that in this case, the Length value is less than the true length?

Remind me: does the Length value include the size of the Length field itself? If not, this value looks too large for what is reported in the StringTable.

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:279
+
+## Case 18: an error will be triggered in llvm-readobj if `Length` value is invalid
+##          for a string table.
----------------
Strictly, this is also an llvm-readobj test. Really, we want this case tested in llvm-readobj testing, but have something here to show we can actually generate this input.

I can't think of a way to inspect the actual Length value, short of manually dumping the hex bytes using `od` at the appropriate offset. We do this in a few other places in these sorts of tests, where there is no other option.

The comment also needs to say how this value is invalid. In this case, I think it's because the end of the string table appears to be off the end of the file?

================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:292
+
+## Case 20: report an error if `ContentSize` is less than 4 without `RawContent`.
+# RUN: not yaml2obj --docnum=1 %s -DCONTENTSIZE=3 -o %t20 2>&1 \
----------------
I'd move this case up to around Case 4, since it is similar.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107421