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

Thu Mar 18 02:27:49 PDT 2021

jhenderson added inline comments.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:57
+
+bool XCOFFWriter::assignAddressesAndIndices() {
+  // Overwrites some fields of InitSections and InitFileHdr with inferred values
----------------
It might be a good idea to break this function up into smaller pieces, perhaps one function per loop, so you'd have a function that assigns the section offsets and addresses, another function for the relocations, another for the symbols etc.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:67-69
+  SectionIndexMap[StringRef("N_DEBUG")] = XCOFF::N_DEBUG;
+  SectionIndexMap[StringRef("N_ABS")] = XCOFF::N_ABS;
+  SectionIndexMap[StringRef("N_UNDEF")] = XCOFF::N_UNDEF;
----------------
Can you do this in the `SectionIndexMap` initialisation list, or at least in the constructor if that's not possible?

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:81
+    }
+    // Assigns indices for sections.
+    if (!SectionIndexMap[InitSections[I].SectionName]) {
----------------
Put a blank line before this line, to help break up this function.

Same goes below: if you put a comment, followed immediately by an if statement or loop relating to that comment, add a blank line.

Also, I think it's more common grammatically in code comments to right "Assign" rather than "Assigns" (i.e. use the imperative form). Same for "Calculates" (use "Calculate") below.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:93
+    if (InitSections[I].SectionData.binary_size()) {
+      InitSections[I].FileOffsetToData = CurrentOffset;
+      CurrentOffset += InitSections[I].SectionData.binary_size();
----------------
Is "FileOffsetToData" the actual XCOFF field name, or is it something else? If something else, I recommend using the real field name.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:95-96
+      CurrentOffset += InitSections[I].SectionData.binary_size();
+      // Make sure the address of the next section aligned to
+      // DefaultSectionAlign.
+      CurrentOffset = alignTo(CurrentOffset, DefaultSectionAlign);
----------------
And check whether the comment can be reflowed to fit within 80 characters width.

================
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.
----------------
# 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?

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:109
+    else
+      InitSections[I].Address = CurrentSecAddr;
+  }
----------------
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?

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

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:147
+  // Target machine. The default format of object file is XCOFF32.
+  W.write<uint16_t>(Obj.Header.Magic ? (uint16_t)Obj.Header.Magic : 0x01DF);
+  // Number of sections.
----------------
You probably need to make the `Header` fields optional themselves, so that this code can distinguish between the case where the user has explicitly specified the value as `0` and the case where it is unspecified.

The same likely goes for other structs like the Section.

Perhaps also worth pulling `0x01DF` into a named constant somewhere.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:148
+  W.write<uint16_t>(Obj.Header.Magic ? (uint16_t)Obj.Header.Magic : 0x01DF);
+  // Number of sections.
+  W.write<uint16_t>(Obj.Header.NumberOfSections ? Obj.Header.NumberOfSections
----------------
Where the field name is self-explanatory, you don't need to have a comment. Same goes below.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:174-176
+    W.write<uint32_t>(YamlSec.Address ? YamlSec.Address : DerivedSec.Address);
+    // Virtual address (same as physical address).
+    W.write<uint32_t>(YamlSec.Address ? YamlSec.Address : DerivedSec.Address);
----------------
Calculate the requested address value upfront, then use that value in both places, rather than repeat the logic to calculate it.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:207
+      if (PaddingSize < 0) {
+        ErrHandler("redundant data was wrote before section data");
+        return false;
----------------
Same in similar messages below.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:221
+    if (!YamlSec.Relocations.empty()) {
+      int32_t PaddingSize =
+          InitSections[I].FileOffsetToRelocations - (W.OS.tell() - StartOffset);
----------------
Probably best to make all these PaddingSize fields `int64_t` to prepare for 64-bit support.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:224
+      if (PaddingSize < 0) {
+        ErrHandler("redundant data was wrote before relocations");
+        return false;
----------------

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:271
+      // storage class (n_sclass) and type (n_type) of the symbol table entry.
+      W.write<uint32_t>(0);
+      W.write<uint32_t>(0);
----------------
At this point, it may be better to just use write_zeroes to fill the whole auxiliary entry. Alternatively, emit an error if `NumberOfAuxEntries` is non-zero.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:289
+  // TODO: Add support for 64-bit.
+  if ((uint16_t)Obj.Header.Magic != 0x01DF) {
+    ErrHandler("only XCOFF32 is supported now");
----------------
This sounds like it prevents a user from writing a 32-bit XCOFF file, but with a messed up magic field. Perhaps you should put an optional explicit format field in the YAML (defaulting to 32-bit XCOFF), and use that to determine the file format to use.

================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:294
+  bool Res = true;
+  Res &= assignAddressesAndIndices();
+  StartOffset = W.OS.tell();
----------------
It seems like if you failed to assign addresses or indices, it may be dangerous to continue? This should probably bail out. Same probably goes for the other cases where an error can occur too.

================
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);
----------------
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?

================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:114
+  IO.mapRequired("Address", R.VirtualAddress);
+  IO.mapRequired("Symbol", R.SymbolIndex);
+  IO.mapOptional("Info", R.Info);
----------------
Can XCOFF relocations have no symbol? If so, I think a 0 index value would be a reasonable default.

================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:116
+  IO.mapOptional("Info", R.Info);
+  IO.mapOptional("Type", R.Type);
+}
----------------
Is there a reasonable default for the relocation type? In ELF, there isn't really, so I think it's a required field.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505