[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:
> 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?



================
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;
----------------
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.


================
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
+
----------------
jhenderson wrote:
> I'm not sure there's much point in this check?
Removed.


================
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
+
----------------
jhenderson wrote:
> I'm not sure this size check adds anything. The fact that the .strtab section's offset also increases is surely sufficient?
> The fact that the .strtab section's offset also increases is surely sufficient?

If we assume that is not broken/overridden, then - yes.


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

https://reviews.llvm.org/D78927