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

[George Rimar via Phabricator via llvm-commits]

grimar 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) + ")");
----------------
jhenderson wrote:
> 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.
OK. Done.


================
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.
----------------
jhenderson wrote:
> 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?
No, you are right, it is written to the end. I've broke this comment during rewriting somehow :/ Fixed.


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

https://reviews.llvm.org/D78927