This is an archive of the discontinued LLVM Phabricator instance.

Change ELF tools to allow multiple sections per file.
ClosedPublic

Authored by pcc on May 23 2019, 2:53 PM.

Details

Summary

This is how multi-partition combined output files are going to look. If we
see multiple sections, the tools will just read the first one.

Event Timeline

pcc created this revision.May 23 2019, 2:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 2:53 PM
Herald added a subscriber: rupprecht. · View Herald Transcript

I think you can add a test using yaml2obj probably. Can you?
Also, does it really pass existent test cases? (I would expect to see some of them testing "Multiple SHT_*" message)

And ideal behavior will be to dump all the sections, right?
Perhaps it worth to report a bug then. It is probably possible to work on that with use of yaml2obj tool
for creating the tests.

I think you can add a test using yaml2obj probably. Can you?
Also, does it really pass existent test cases? (I would expect to see some of them testing "Multiple SHT_*" message)

And ideal behavior will be to dump all the sections, right?
Perhaps it worth to report a bug then. It is probably possible to work on that with use of yaml2obj tool
for creating the tests.

You might want to enhance yaml2obj to allow multiple sections with the same name, although I don't think it's strictly needed. On taking the first one, do GNU tools do this? Or do they take the last one/emit an error/print all? I see no reason to emit an error here, so the change in behaviour is good, but if it picks the last, we should probably do the same. As @grimar mentions, if GNU dumps all, we should file bugs.

On the LLVM-specific sections, I think it would make more sense to just do what we do with the normal sections.

llvm/include/llvm/Object/ELFObjectFile.h
948–949

What about this case?

llvm/tools/llvm-readobj/ELFDumper.cpp
1429–1430

Ditto.

1434–1435

Ditto.

MaskRay accepted this revision.May 24 2019, 2:27 AM

LGTM. I suppose you'll add the tests in D62350, but please wait for others' opinions.

This revision is now accepted and ready to land.May 24 2019, 2:27 AM
MaskRay added inline comments.May 24 2019, 2:29 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
1429–1430

See D62350. Only dynamic sections are duplicated for partitions.

Static symbol table, etc, are still singleton.

jhenderson added inline comments.May 24 2019, 2:38 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
1429–1430

The principle is more general though. For most of these tools, there's no reason why there can't be multiple sections of these other types.

pcc marked an inline comment as done.May 24 2019, 3:27 PM

I think some of the GNU tools dump the first one, while others dump all of them. For example, nm dumps first, readelf dumps all. I agree with @grimar that regardless of what the GNU tools do, I think the end goal should be to dump all of them in all tools somehow. I filed https://bugs.llvm.org/show_bug.cgi?id=42013 to track this behaviour. But to start with, dumping the first one is better than erroring.

I'll see if yaml2obj will let me write tests for this. It looks like the current behaviour is untested, which might mean that things are harder than I'm expecting.

llvm/tools/llvm-readobj/ELFDumper.cpp
1429–1430

My initial inclination was to do only what was required for partitions, but after thinking about this for a bit, I think it's fine to do this for all sections. Although it means that we'll be ignoring some sections in some cases, that's simply a limitation of our tools and there's nothing wrong with the files themselves.

pcc updated this revision to Diff 201372.May 24 2019, 5:32 PM
pcc retitled this revision from Change ELF tools to allow multiple dynsym, versym, verdef and verneed sections per file. to Change ELF tools to allow multiple sections per file..
pcc removed a reviewer: jhenderson.

Address review comments

grimar added inline comments.May 27 2019, 2:53 AM
llvm/test/Object/multiple-sections.yaml
2 ↗(On Diff #201372)

I would suggest to use
llvm-readobj -a --elf-cg-profile --addrsig

(llvm-readelf is a GNU style and seems does not implement dumpers for versioning sections and SHT_LLVM_* sections yet.
Also, -a or --headers does not trigger --elf-cg-profile/--addrsig it seems).

And then perhaps I would check that at least something was dumped:

# CHECK:      Version symbols
# CHECK-NEXT:   Section Name: .versym

# CHECK: SHT_GNU_verdef
# CHECK: SHT_GNU_verneed
# CHECK: CGProfile
# CHECK: Addrsig
4 ↗(On Diff #201372)

Please add a short description of the test in comment.
(i.e. "here we are testing that multiple sections in a object does not trigger error", or something like that)

21 ↗(On Diff #201372)

You do not need AddressAlign field here and below I think.

78 ↗(On Diff #201372)

If I correctly understand you need this symbol to trigger creation of the first .symtab.
But does not seem you need .text section, can it probably be local?

Symbols:         
  - Name:            f
llvm/tools/llvm-readobj/ELFDumper.cpp
1411

I think the following is more correct:

if (!DynSymRegion.Size) {
  DynSymRegion = createDRIFrom(&Sec);
  // This is only used (if Elf_Shdr present)for naming section in GNU style
  DynSymtabName = unwrapOrError(Obj->getSectionName(&Sec));
  DynamicStringTable = unwrapOrError(Obj->getStringTableForSymtab(Sec));
}
ruiu accepted this revision.May 27 2019, 3:04 AM

LGTM

This revision was automatically updated to reflect the committed changes.
pcc marked 5 inline comments as done.