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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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) + ")");
----------------
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`).


================
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;
----------------
grimar wrote:
> jhenderson wrote:
> > 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). 
> yaml2obj does not support writing section out of order currently. This patch has no intention to change this.
> We always write sections in the same order they are listed in a YAML.
> 
> To implement what you want perhaps we might want just to introduce another field, like "SectionHeaderIndex".
> Or may be expand YAML format to it allow to describe section header table (and may be it's location) explicitly.
`alignToOffset(sec)` can be called according to the section offset order in `Doc.Chunks` in `ELFState<ELFT>::initSectionHeaders` may be sufficient, but I haven't tried.


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

https://reviews.llvm.org/D78927