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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm in two minds about whether a length greater than the string length should write padding data. I'm inclined to think we need another optional field. In the ELF case, for sections, we have two relevant fields, a Size field and a ShSize field. The Size field increases the actual content size by padding with zeroes as necessary. It is an error to specify a value for the Size field that is smaller than any Content size. If ShSize is specified, the section header sh_size value is written to that value, regardless of the section content. It has no impact on the section content. If no ShSize is specified, the Size value is used. As such, if you want to change the actual data written, you use Size. Otherwise, you use ShSize.

I've been thinking about it, and I think for XCOFF string tables, the Length field is approximately equivalent to the ELF sh_size section header field. As such, I reckon it should be treated similarly to the ShSize field. You therefore probably want another field, roughly equivalent to the Size field of the ELF section, maybe called ContentSize? I think it would work like this:

1. If RawContent is specified and no ContentSize is specified, or ContentSize matches the size of the RawContent data, write the RawContent as-is and nothing else.
2. If RawContent is specified and ContentSize is less than the RawContent data size, emit an error.
3. If RawContent is specified and ContentSize is greater than the RawContent data size, pad the RawContent with trailing zeroes so that its size now matches ContentSize.
4. It is an error if both RawContent the Strings member are specified.
5. As you've already implemented, Strings should replace the used symbol names. If there are more symbols than strings, add them to the list.
6. It is an error if RawContent is not used and ContentSize is used and ContentSize is less than the size of the data that would otherwise be written.
7. If RawContent is not used and ContentSize is greater than the size of the data that would be written, pad with zeroes, as in case 3 above.
8. It is an error if the Length field is specified as well as RawContent.
9. Otherwise, if specified, use the value of the Length field for the first 4 bytes of the table (note that this means that `ContentSize <=3` will always be an error without RawContent). The value may not make sense for the data that is beign written. This is deliberate.
10. If Length is not specified, use the value as defined by the data size AFTER trailing zeroes are added (i.e. Length represents the length of the data written).

Hopefully these steps will provide a good basis for the updated tests.



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:385
+  // length is equal to the string table's size.
+  if (Obj.StrTbl.Length == 0 || PaddingSize == 0) {
+    StrTblBuilder.write(W.OS);
----------------
I'm not sure we need this special case? What's the use-case for it? Could a user just as easily use raw content to specify it?


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:390-394
+  if (PaddingSize < 0) {
+    ErrHandler("specified length (" + Twine(Obj.StrTbl.Length) +
+               ") is less than the content size (" + Twine(StrTblSize) + ")");
+    return false;
+  }
----------------
I think we shoul support this case. In other words, if a user has specified a Length field that is shorter than the strings that should be written, just write the specified length field and then all strings with no padding.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:401
+  StrTblBuilder.write(Ptr);
+  // Modify the first 4-bytes with the specified length value.
+  memset(Ptr, 0, 4);
----------------



================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:155
+void MappingTraits<XCOFFYAML::StringTable>::mapping(IO &IO, XCOFFYAML::StringTable &Str) {
+  IO.mapOptional("Suppress", Str.Suppressed);
+  IO.mapOptional("Length", Str.Length);
----------------
I don't think we need the `Suppress` field anymore - a user could use an empty `RawContent` in this case.


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/aix.yaml:80
 # CHECK-NEXT:     NumberOfAuxEntries: 1
-# CHECK-NEXT: ...
----------------
Either leave this in, or fix the check in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107421