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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:63-68
+  uint32_t TableSize = 0; // The actual size of the string table, including the
+                          // size of the length field.
+  uint32_t Length = 0; // The value of the length field for the first 4 bytes of
+                       // the table.
+  std::vector<StringRef> Strings;
+  yaml::BinaryRef RawContent;
----------------
Esme wrote:
> jhenderson wrote:
> > These fields probably all want to be Optional values. You need to be able to distinguish the cases where a field has a value of zero/is an empty array/is an empty string from where the field is missing. This is often important for the error case handling, if nothing else.
> Yes, agreed. It looks like some fields in other structs need such change as well, maybe we need a new patch to do this?
If you feel it would be useful in other cases, then those cases can be modified in other patches, but the StringTable class should be done in this patch, since it impacts how you customise it.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:149
+  size_t RawContentSize = Obj.StrTbl.RawContent.binary_size();
+  if (Obj.StrTbl.TableSize && Obj.StrTbl.TableSize < (RawContentSize + 4)) {
+    ErrHandler("specified TableSize (" + Twine(Obj.StrTbl.TableSize) +
----------------
Esme wrote:
> jhenderson wrote:
> > You don't need additional space for the Length field if you are using RawContent. If RawContent is used, then you should just have that content, plus some possible trailing padding if TableSize is greater than than the size of the RawContent.
> I think it would be confusing that the RawContent includes the length field and also the TableSize is specified, since there would be 3 different values for the string table size, ie. the length field in RawContent, the size of RawContent and the TableSize value.
> 
> For example, the RawContent is "000000090062006300" (where the "00000009" is the length field) and the TableSize is 10, then what would the string table look like? "00000009006200630000" or "00000010006200630000"? 
> And what if the RawContent is incorrect (ex. 000000070062006300) but the TableSize (ex. 9+) is correct?
> 
> Besides, I think it is not very convenient for users to calculate the content size for the input RawContent even they don't want to test the length value.
> 
> Any thoughts?
I suggested ContentSize, previously as the name, not TableSize, as I feel it is clear that it is tied to the RawContent field. Another more verbose name could be "AmountOfDataToWriteAtTheStringTableOffset" which obviously clarifies the intent but is too long.

I think you are missing the use-case here. When we use "RawContent" it is because we want to precisely control what is written at the offset of where the string table should start. This allows testing of various things like an empty or very short table, for example, or specific data patterns that might be necessary for some other reason. It will likely be fairly rare that RawContent should be used, and even rarer where it should be used and the Length field needs to be valid. In those limited cases, I think it is fine to require the user to provide the length field as part of the data. I also think it would be confusing to implicitly write additional data in addition to "RawContent", because that is different to how content is usually written in yaml2obj.

To answer the questions directly:
> For example, the RawContent is "000000090062006300" (where the "00000009" is the length field) and the TableSize is 10, then what would the string table look like? "00000009006200630000" or "00000010006200630000"? 
yaml2obj would just write the requested RawContent, and pad with zeroes up to TableSize. The TableSize is the total amount of data written, so there should be a total of 10 bytes written. The data would be `00000009006200630000`.
> And what if the RawContent is incorrect (ex. 000000070062006300) but the TableSize (ex. 9+) is correct?
As above, yaml2obj would just write the requested RawContent, and pad with zeroes up to TableSize.

This is covered by my points 1 to 3 in my out-of-line cases list from earlier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107421