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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:55-56
+      {StringRef("N_UNDEF"), XCOFF::N_UNDEF}};
+  XCOFFYAML::FileHeader InitFileHdr = Obj.Header;
+  std::vector<XCOFFYAML::Section> InitSections = Obj.Sections;
+};
----------------
Why not just use `Obj.Header` and `Obj.Sections` directly in the referencing code? Why do you need these member variables separately to `Obj`?


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:62-63
+    if (!InitSections[I].Relocations.empty()) {
+      // Only the .text, .data, .tdata, and STYP_DWARF sections have
+      // relocations.
+      int32_t SecFlags = InitSections[I].Flags;
----------------
Is this a fundamental restriction of the XCOFF file format, or is it just what should happen? Is it possible to create (potentially by hand) a section with relocations if it isn't one of these types?

In general, to allow for testing of bad input paths etc, you want to allow as much flexibility in yaml2obj as possible. As such, it isn't yaml2obj's place to restrict what can be done, as long as it can physically represent what was requested.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:76
+      if (CurrentOffset > MaxRawDataSize) {
+        ErrHandler("relocation data overflowed the MaxRawDataSize: " +
+                   Twine(MaxRawDataSize));
----------------
`MaxRawDataSize` is an internal variable, right, not an XCOFF spec defined value? If `MaxRawDataSize` doesn't directly appear in the spec, don't mention it to the user in an error message. Instead, use general terms like "the maximum size permitted for XXX" (where XXX is the thing that is restricted, e.g. the object size).


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:119
+      CurrentOffset += InitSections[I].SectionData.binary_size();
+      // Ensure the address is aligned to DefaultSectionAlign.
+      CurrentOffset = alignTo(CurrentOffset, DefaultSectionAlign);
----------------
Is it definitely the address here that should be being aligned, or the offset? The two are different concepts, and the align function appears to align the offset, not the address.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:135
+
+  // Count the number of auxiliary symbols into the total number.
+  InitFileHdr.NumberOfSymTableEntries = Obj.Symbols.size();
----------------



================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:190-191
+        YamlSec.Address ? YamlSec.Address : DerivedSec.Address;
+    W.write<uint32_t>(SectionAddress);
+    W.write<uint32_t>(SectionAddress);
+    W.write<uint32_t>(YamlSec.Size ? YamlSec.Size : DerivedSec.Size);
----------------
Here, it's probably worth a comment saying which is the virtual address and which the physical address, like the inline edit, for example (which may be the wrong way around).


================
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.
----------------
Esme wrote:
> 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. ]]
It looks like from the spec that the names are merely conventions, so technically the names could be other thigns. It's probably fine to infer that if a section is text, it is called .text, unless otherwise specified by the YAML (and vice versa), but this bit applies to the section type, not the name, so we should refer to them by type rather than name (i.e. just "text", "data", "bss", not ".text" etc).


================
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);
----------------
Esme wrote:
> 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?
Could 0 be a reasonable default? If not, it's fine. I'm just wondering if it actually matters in all cases what the address is.


================
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);
----------------
Esme wrote:
> 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.
Sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505