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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Having thought about it more, I think we should have a slightly different behaviour for the case where we have symbol names and string table contents. It's likely if you've specified string table contents, you want to override the entire table contents with the specified strings. In this case, I'd do the following:

1. Internally generate the string table data as-if the strings weren't specified explicitly. Use this for populating symbol name offsets.
2. Once the offsets have been generated, throw away that data and use the YAML-specified Strings as the entire table contents.

Finally, you probably need a way to specify the table as raw contents. This allows you to have a test case for a table which is too short for the length field. This could be a "Contents" field (see ELF for similar examples for sections), which cannot be specified with other parameters (yaml2obj should emit an error in this case).

At some point, you should also consider an obj2yaml implementation that makes use of the StringTable property too, if the input is malformed somehow, but that can be a separate patch.



================
Comment at: llvm/lib/MC/StringTableBuilder.cpp:80
   if (K == WinCOFF)
-    support::endian::write32le(Buf, Size);
+    support::endian::write32le(Buf, Length ? Length : Size);
   else if (K == XCOFF)
----------------
Higuoxing wrote:
> Higuoxing wrote:
> > Personally, I don't like this approach. I would do the overwriting job in the client side. Besides, this change is dangerous, what will happen if `Length > Size` ?
> Sorry, I made a mistake here. I realize that you're modifying the first 4-byte of the buffer. But I still think we should do it in the client side.
+1 to not modifying the StringTableBuilder interface.

The majority of the clients don't have a need to specify a different size to the length. yaml2obj is different in that it is deliberately trying to create an invalid string table.


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:1
+## Check that yaml2obj is able to customize the string table.
+
----------------
This test needs a couple more cases:

1) The Length field is specified to be less than the total length of explicitly-specified strings (the data should be written, but the length field specified to be the shorter value).
2) As 1), but where there are implicit contents.
3) As 1), but where the length is greater than the content length (0 bytes should be used for padding probably).
4) As 2), but where the length is greater than the content length (again, pad with 0 bytes).


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:12-16
+--- !XCOFF
+FileHeader:
+  MagicNumber: 0x1DF
+Symbols:
+  - Name: alongname
----------------
Maybe have a case 1b, which is identical but with an empty `StringTable` tag?


================
Comment at: llvm/test/tools/yaml2obj/XCOFF/string-table.yaml:137
+StringTable:
+  Suppress: true
----------------
We probably want an error emitted by yaml2obj if Suppress is specified at the same time as other string table parameters. This will necessitate two more cases: 1) with Length + Suppress and 2) with Length + Strings


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107421