[PATCH] D95505: [yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

[EsmeYi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:93
+    if (InitSections[I].SectionData.binary_size()) {
+      InitSections[I].FileOffsetToData = CurrentOffset;
+      CurrentOffset += InitSections[I].SectionData.binary_size();
----------------
jhenderson wrote:
> Is "FileOffsetToData" the actual XCOFF field name, or is it something else? If something else, I recommend using the real field name.
Yes, it's also defined in XCOFF.h.
The name "FileOffsetToData" has been used in all XCOFF related implementations，which corresponds to "s_scnptr" in the documentation.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:102
+    // Calculate the physical/virtual address.
+    // This field should contain 0 for all sections except the .text , .data ,
+    // and .bss sections.
----------------
jhenderson wrote:
> # Can there be more than one each of .text/.data/.bss?
> # If so, are they all named the same?
> # Are the names actually ".text", ".data" etc?
1. Yes, multiple .text/.data/.bss sections are allowed.
2. Applications use the Flags field instead of the Name field to determine a section type. Two sections of the same type may have different names.
3. As introduced in  [[ https://www.ibm.com/support/knowledgecenter/ssw_aix_72/filesreference/XCOFF.html#XCOFF__a2zjvh2b9shar | Table 4. ]]


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:109
+    else
+      InitSections[I].Address = CurrentSecAddr;
+  }
----------------
jhenderson wrote:
> If I'm reading this code correctly, if a user has put their sections in an order like:
> ```
> .text
> .somethingelse
> .data
> ```
> all sections are size 4, and the address of .text is 16, the address of .data is going to be 24, not 20. Is that supposed to be the case?
Moving this calculation ahead of the calculation of FileOffsetToData gives the correct result.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:115
+      InitSections[I].NumberOfRelocations = InitSections[I].Relocations.size();
+      InitSections[I].FileOffsetToRelocations = CurrentOffset;
+      CurrentOffset += InitSections[I].NumberOfRelocations *
----------------
jhenderson wrote:
> Reading this code, it looks like you can't have in XCOFF relocations that aren't attached to a section. Is that correct?
Yes,  the relocations are always attached to sections in XCOFF.


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:113
+void MappingTraits<XCOFFYAML::Relocation>::mapping(IO &IO, XCOFFYAML::Relocation &R) {
+  IO.mapRequired("Address", R.VirtualAddress);
+  IO.mapRequired("Symbol", R.SymbolIndex);
----------------
jhenderson wrote:
> In ELF, the `r_offset` field of a relocation specifies where the relocation will patch, and is relative to either the section start or file start, depending on the context. Often in our test cases, it doesn't really matter where the relocation is patching, only that there is a relocation, hence the offset field is optional there. It seems likely that this will be the case for XCOFF too?
I agree that it's unfriendly to require the virtual address of the relocation from users.
While this item in XCOFF specifies the virtual address of the value that requires modification by the binder. And the offset to the data in the section will be calculated as follows:
offset_in_section = Relocation_Address - Section_Address
We can calculate the Section_Address from yaml contents, but hard to determine the Relocation_Address.
So this is hardly optional?


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:120
+void MappingTraits<XCOFFYAML::Section>::mapping(IO &IO, XCOFFYAML::Section &Sec) {
+  IO.mapRequired("Name", Sec.SectionName);
+  IO.mapOptional("Address", Sec.Address);
----------------
It's reasonable to make the name to be optimal and the flags to be required, since the flags are the unique identifier of the section type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505