Page MenuHomePhabricator

[yaml2obj] - Implement the "SectionHeaderTable" tag.
Needs ReviewPublic

Authored by grimar on Fri, May 15, 5:43 AM.

Details

Summary

With the "SectionHeaderTable" it is now possible to reorder
entries in the section header table.

It also allows to stop emitting the table.

Depends on D79984, D79985.

Diff Detail

Event Timeline

grimar created this revision.Fri, May 15, 5:43 AM
Herald added a project: Restricted Project. · View Herald Transcript

This is a good idea.

llvm/lib/ObjectYAML/ELFEmitter.cpp
1467

try_emplace can remove {}

1473

Use ArrayRef::slice to skip SHT_NULL.

1478

section name should be present in the Sections list

1482

auto &

llvm/test/tools/yaml2obj/ELF/section-headers.yaml
32

Fill not indented correctly.

40

Can Sections be omitted?

grimar marked an inline comment as done.Sun, May 17, 3:40 AM
grimar added inline comments.
llvm/test/tools/yaml2obj/ELF/section-headers.yaml
40

It was done intentionally. I am thinking about adding a one more "Location"/"Offset" tag here in a follow-up
which should allow placing the table before/after sections or, perhaps, to a particular location either:

SectionHeaderTable:
  Offset: AutoBefore
  Sections:
  ...

(Something like was mentioned here: https://reviews.llvm.org/D78927#inline-732176)

Mostly looks good, but a number of small comments from me.

llvm/include/llvm/ObjectYAML/ELFYAML.h
89

If there's nothing stopping it, I'd prefer to make this SectionHeader (in commong with FileHeader and ProgramHeader). Variable names could then be changed to SecHdr where necessary.

516

SectionHeaders

llvm/lib/ObjectYAML/ELFEmitter.cpp
323

I'm not convinced we should do this. This field doesn't change according to the ELF gABI when there are no section headers (unlike e_shnum and e_shoff).

328

Maybe change this to else if (Doc.SectionHeader && Doc.SectionHeader->sections.empty()) and then get rid of the (!Doc.SectionHeader) clause, since it has the same effect as the final else.

336

I take it Doc.getSections() includes a null section header already?

340

I'm wondering whether we should make the null section explicit in the SectionHeaderTable block. This allows us to omit that section header entirely, or to have just a section header table containing only the null shdr.

llvm/test/tools/yaml2obj/ELF/section-headers.yaml
62

I'm not sure what this message is trying to say, but I'm assuming it's "omits any section that exists." which I think would be a clearer way of phrasing.

63

Rather than calling the section "undefined", which implies something to do with SHN_UNDEF, let's go with "non-existent".

64–65

I don't feel strongly about this, so if you prefer this way, that's fine, but I personally prefer the first line to end with | \ and the second line to be indented slightly, i.e.

# RUN: not yaml2obj ... | \
# RUN:   FileCheck ...

The reasoning is 1) the '|' shows the command is done, and 2), the indentation shows it's a continuation implicitly.

106

"section header" -> "section header table" (or possibly just "section headers")

grimar updated this revision to Diff 266005.Mon, May 25, 5:13 AM
grimar marked 20 inline comments as done.
  • Rebased.
  • Addressed review comments.
MaskRay added inline comments.Mon, May 25, 9:46 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
340

Making the SHT_NULL section explicit in SectionHeaderTable seems fine, but how do we encode it?

The SHT_NULL section in Sections remains on-demand implicit.

grimar added inline comments.Tue, May 26, 6:31 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
323

OK. I've reverted this piece.

336

Yes. It is either defined explicitly in a YAML or we implicilty add it much earlier:

template <class ELFT>
ELFState<ELFT>::ELFState(ELFYAML::Object &D, yaml::ErrorHandler EH)
    : Doc(D), ErrHandler(EH) {
  std::vector<ELFYAML::Section *> Sections = Doc.getSections();
  // 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));
...
340

This allows us to omit that section header entirely, or to have just a section header table containing only the null shdr.

Is it useful? (I am not sure I can imagine a use case except creating of a specific broken object,
though it should be possible to create such using SHOff even right now. I.e. SHOff could point to a hand crafted table. Or just to the first header directly, to skip the null one. I agree that is not that straightforward though).

The current implementation is that SectionHeaderTable must list all sections (null section is an exception). I think it is a nice propery,
because it seems can be easy to forget to mention a section and omit it by mistake. E.g. in this patch I've to add implicit sections like ".shstrtab" and ".strtab" manually.

Perhaps, we might introduce a new explicit syntax to describe omitted sections if you want to have such feature. E.g:

SectionHeaderTable:
  Sections:
  ...
  Excluded:
    - foo
    - bar

I'm wondering whether we should make the null section explicit

My concern is that we'll have an unnecessary noise in most of the test cases that use SectionHeaderTable then. Usually
test cases do not need to do anything with the SHT_NULL section. Perhaps, the new explicit syntax might help here too.
Like:

SectionHeaderTable:
  CreateImplicitNullSection: false
1467

Nice!

1473

getSections() returns std::vector..

llvm/test/tools/yaml2obj/ELF/section-headers.yaml
64–65

I usually also do in this way I think. Not sure why did differently that time. Fixed.

grimar added a comment.EditedTue, May 26, 6:32 AM

(For some reason seems I also forgot to push "submit" for my comments previously. Sorry!).