[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:1027-1030
+      reportError("the 'Offset' value (0x" +
+                  Twine::utohexstr((uint64_t)*Offset) +
+                  ") is incompatible with the alignment requested (0x" +
+                  Twine::utohexstr(Align) + ")");
----------------
MaskRay wrote:
> grimar wrote:
> > jhenderson wrote:
> > > 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)
> > I wasn't sure about this error, but using both "AddressAlign" and "Offset" in a YAML probably might lead to confusion:
> > 
> > ```
> > Sections:
> >   - Name:         .bar
> >     Type:         SHT_PROGBITS
> >     Size:         0x10
> >     Offset:       0x100
> >     AddressAlign: 0x1000
> > ```
> > 
> > Usually `AddressAlign` affects the offset. I.e. it is not a regular field that just sets a value.
> > And so it might raise a question: should an `Offset` be aligned by `AddressAlign`? Or should we use the `Offset` given and just ignore `AddressAlign`s usual logic? 
> > 
> > I am not sure we want to silently allow this combination (when an alignment doesn't match up).
> > 
> > Currently there is a way to achieve what you describe: it should be possible to use `Filler`(s) + `ShOffset` + `AddressAlign` to create any layout.
> > Perhaps it is sufficient?
> > 
> For SHT_NOBITS sections, sh_offset%sh_addralign!=0 is possible (sh_offset for SHT_NOBITS can generally be ignored). lld can create such output (`ELF/Writer.cpp:computeFileOffset`).
> I am not sure we want to silently allow this combination (when an alignment doesn't match up).

I'd probably suggest ignoring the alignment in this situation, since the Offset is more explicit - if I specified an Offset that was deliberately not aligned, I'd probably still expect it to get respected, and the alignment field just used to set the sh_addralign value.

You could add a warning if you wanted, but since those aren't likely to be seen, I'm not sure how useful that is.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:303
       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;
-  CBA.getOSAndAlignedOffset(SHOff, sizeof(typename ELFT::uint));
+  // Align the start of the section header and write the ELF header right after
+  // the ELF header and program headers.
----------------
section header -> section header table (or section headers)

The **ELF** header?

I thought someone made a change a while back to write the section header table at the end anyway? Or am I getting mixed up with a different thing?


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

section -> sections


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:110
+
+## The same offset as in the Case 1.
+# BOTH-B: Start of section headers: 288 (bytes into file)
----------------
in Case 1.


================
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
+
----------------
jhenderson wrote:
> Replace with `cmp %t1 %t3`?
This referred to the llvm-readelf line. We don't need to do llvm-readelf twice, if the two outputs should be binary identical. Use `cmp` instead.


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

https://reviews.llvm.org/D78927