[PATCH] D78927: [yaml2obj] - Introduce the "Offset" property for sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:303-304
       Doc.Header.SHEntSize ? (uint16_t)*Doc.Header.SHEntSize : sizeof(Elf_Shdr);
   // Immediately following the ELF header and program headers.
   // Align the start of the section header and write the ELF header.
+  uint64_t SHOff =
----------------
This comment looks stale to me. It doesn't even read right.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:451-452
 
+    // Set an offset for the special SHN_UNDEF section only when it is
+    // explicitly requested.
+    bool IsZeroUndefSection = SecNdx == 0;
----------------
I think this comment needs to make clear that the code block it accompanies sets the offset of all sections. How about "Set the offset for all sections, except the SHN_UNDEF section with index 0 when not explicitly requested."?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:453
+    // explicitly requested.
+    bool IsZeroUndefSection = SecNdx == 0;
+    if (!IsZeroUndefSection || Sec->Offset)
----------------
Perhaps `IsFirstUndefSection` would be a bit clearer.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:485
     } else if (auto S = dyn_cast<ELFYAML::NoBitsSection>(Sec)) {
+      // The SHT_NOBITS section does not have any content to write.
       SHeader.sh_entsize = 0;
----------------
"The SHT_NOBITS section does" -> "SHT_NOBITS sections do not"


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1027-1030
+      reportError("the 'Offset' value (0x" +
+                  Twine::utohexstr((uint64_t)*Offset) +
+                  ") is incompatible with the alignment requested (0x" +
+                  Twine::utohexstr(Align) + ")");
----------------
Should we really be reporting this error? I could imagine people wanting to create broken ELFs with a section at a specific offset and an alignment that doesn't match up.

(Also "requested alignment" is more correct - adjectives generally come before their nouns)


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1035-1036
+    if ((uint64_t)*Offset < CurrentOffset) {
+      reportError("the 'Offset' value (0x" +
+                  Twine::utohexstr((uint64_t)*Offset) + ") goes backward");
+      return CurrentOffset;
----------------
I can see why you've done this, but I think there's also a use-case for a section header table where the sections aren't in offset order, which I believe would run into this problem. You could use the SHOffset field instead, but that has its own problems (doesn't place data at the specified location, shouldn't be included in program header calculations etc). 


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:8
+# RUN: llvm-readelf --sections %t1 | FileCheck %s --check-prefix=DEFAULT
+# RUN: wc -c < %t1 | FileCheck %s --check-prefix=DEFAULT-SIZE
+
----------------
I'm not sure there's much point in this check?


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:38
+# RUN: llvm-readelf --sections %t2 | FileCheck %s --check-prefix=OFFSET
+# RUN: wc -c < %t2 | FileCheck %s --check-prefix=OFFSET-SIZE
+
----------------
I'm not sure this size check adds anything. The fact that the .strtab section's offset also increases is surely sufficient?


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:68-69
+# RUN: yaml2obj --docnum=2 %s -o %t3 -DOFFSET=0x48 -DALIGN=0x0
+# RUN: llvm-readelf --sections %t3 | FileCheck %s --check-prefix=DEFAULT
+# RUN: wc -c < %t3 | FileCheck %s --check-prefix=DEFAULT-SIZE
+
----------------
Replace with `cmp %t1 %t3`?


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:89
+# RUN: llvm-readelf --sections %t4 | FileCheck %s --check-prefix=BOTH-SAME
+# RUN: wc -c < %t4 | FileCheck %s --check-prefix=BOTH-SAME-SIZE
+
----------------
Rather than checking the file size, I think a clearer thing to do would be to check the offset of the section header table to show that it is/isn't impacted by the section's offset.


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

https://reviews.llvm.org/D78927