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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/ELF/custom-null-section.yaml:205-206
+
+## Check that we can set an offset for the SHT_NULL section explicitly using the "Offset" key.
+## Check it affects the section header table offset.
+# RUN: yaml2obj --docnum=10 %s -DOFFSET=0x100 -o %t10
----------------
jhenderson wrote:
> The offset of a SHT_NULL section probably shouldn't affect that of other sections, since it is supposed to be "inactive". Not sure if it should be part of this change, but we should have a test case for it.
> The offset of a SHT_NULL section probably shouldn't affect that of other sections, since it is supposed to be "inactive"

Not sure I understood this point right, but I see no reason to make an exception for it:
It is special and that is why we "do not write any content for special SHN_UNDEF section". We are currently able to override its `sh_size`, `sh_info` and `sh_entsize` though. But the offset is not a property of a section. I.e. it has no field like `sh_offset`.
If a person adds `Offset: xx` in a YAML for any section, it should have an effect and following sections should have offsets greater or equal to it anyways (unless `ShOffset` is used to override it).


================
Comment at: llvm/test/tools/yaml2obj/ELF/custom-null-section.yaml:207-210
+# RUN: yaml2obj --docnum=10 %s -DOFFSET=0x100 -o %t10
+# RUN: llvm-readelf --headers --sections %t10 | FileCheck %s --check-prefix=EXPLICIT-OFFSET-A
+# RUN: yaml2obj --docnum=10 %s -DOFFSET=0x200 -o %t11
+# RUN: llvm-readelf --headers --sections %t11 | FileCheck %s --check-prefix=EXPLICIT-OFFSET-B
----------------
jhenderson wrote:
> Any particular reason these aren't one test, with two SHT_NULL sections?
yaml2obj normally creates a `SHT_NULL` section at index 0 implicitly:

```
  // Insert SHT_NULL section implicitly when it is not defined in YAML.
  if (Sections.empty() || Sections.front()->Type != ELF::SHT_NULL)
    Doc.Chunks.insert(
        Doc.Chunks.begin(),
        std::make_unique<ELFYAML::Section>(
            ELFYAML::Chunk::ChunkKind::RawContent, /*IsImplicit=*/true));
```

This patch adds the following code:

```
    if (!IsFirstUndefSection || Sec->Offset)
      SHeader.sh_offset = alignToOffset(CBA, SHeader.sh_addralign, Sec->Offset);
```

I.e. the first `SHT_NULL` section is special, and I've added the code to handle the "Offset" properly.
Others `SHT_NULL` are not special. And I am testing here that
setting an `Offset` for such sections affects the start of the section headers. For that I need to set
different offsets for it and test the result.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-offset.yaml:4
+## Show how the 'Offset' field key can be used.
+## Show that it might affect the file size.
+
----------------
jhenderson wrote:
> This comment needs updating, since you're checking the section header table offset now.
Actually what I think we want to show is that the `Offset` key might affect the file size.
It is the main difference with the `ShOffset` property which never can affect it.
Currently (since you asked to remove the file size test previously) it is tested implicitly:
the `Offset` changes physical offsets of the following sections, but still worth to mention I think.

Changing of the section header table offset is a size effect. If it was placed before sections and not after,
it would not change.


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

https://reviews.llvm.org/D78927