Page MenuHomePhabricator

[llvm-readelf] - Rework how we parse the .dynamic section.
ClosedPublic

Authored by grimar on May 15 2019, 3:30 AM.

Details

Summary

This is a result of what I found during my work on https://bugs.llvm.org/show_bug.cgi?id=41679.

Previously LLVM readelf took the information about .dynamic section from its PT_DYNAMIC segment only.
GNU tools have a bit different logic. They also use the information from the .dynamic section header if it is available.
This patch changes the code to improve the compatibility with the GNU Binutils.

I also think it should resolve the problems from PR mentioned.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.May 15 2019, 3:30 AM
jhenderson added inline comments.May 15 2019, 6:06 AM
test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test
2 ↗(On Diff #199572)

which is not in -> when it is not in a

8 ↗(On Diff #199572)

Does this warning come out with GNU? Same question with the other warnings elsewhere.

test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test
5 ↗(On Diff #199572)

either ". We check that we print a warning for this case" or ". We check that we warn about this case".

tools/llvm-readobj/ELFDumper.cpp
206–207 ↗(On Diff #199572)

I feel like a cleaner interface would be for findDynamicTable to return a start/end offset or even an ArrayRef of the contents, and pass that into parseDynamicTable.

1330–1332 ↗(On Diff #199572)

This feels like a weird behaviour. If it can dump the .dynamic section when it's not in a segment, why does it need the segment to exist?

1343 ↗(On Diff #199572)

has a priority under -> has priority over

tools/llvm-readobj/llvm-readobj.cpp
377 ↗(On Diff #199572)

Do GNU readelf warnings definitely get printed on stdout?

I have the same question as James with respect to requiring PT_DYNAMIC but otherwise the code looks good to me. I'm still looking at the tests.

grimar updated this revision to Diff 200250.May 20 2019, 4:26 AM
grimar marked 10 inline comments as done.
  • Addressed review comments.
  • Fixed one more test: test/Object/corrupt.test
test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test
8 ↗(On Diff #199572)

Yes, I think so:
(I rewrote it a bit, but idea of the message is the same).

umb@ubuntu:~/tests/9$ readelf --dynamic 1.o
readelf: Warning: the .dynamic section is not contained within the dynamic segment

Dynamic section at offset 0x1000 contains 2 entries:
  Tag        Type                         Name/Value
 0x0000000000000018 (BIND_NOW)           
 0x0000000000000000 (NULL)               0x0

umb@ubuntu:~/tests/9$ readelf -v
GNU readelf (GNU Binutils for Ubuntu) 2.31.1
test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test
11 ↗(On Diff #199572)

GNU shows:
readelf: Warning: the .dynamic section is not the first section in the dynamic segment.

I think my version is better because:

  1. I think the different start address does not mean there is a section there. It is a normal assumption, but still I have a fantazy to imagine there can be just a sequence of data before the .dynamic and no section in the section header table. Perhaps does not sound as a real case, but probably possible? Perhaps it is possible to create such test case with yaml2obj even.
  2. I can imagine that SHT_DYNAMIC has name different from .dynamic. GNU code just hardoces the name. Doesn't seem as a good choice.
tools/llvm-readobj/ELFDumper.cpp
206–207 ↗(On Diff #199572)

Hmmm. In the current design we store the tables and use them later, see:

DynRegionInfo DynRelRegion;
DynRegionInfo DynRelaRegion;
DynRegionInfo DynRelrRegion;
DynRegionInfo DynPLTRelRegion;
DynRegionInfo DynSymRegion;
DynRegionInfo DynamicTable;

When I wrote this place, I thought about the following scenario:

  1. Rename the method to findAndParseDynamicTable or alike.
  2. Call the parseDynamicTable from there.

(Honestly I did not try, but seems it should work).

What do you think?

1330–1332 ↗(On Diff #199572)

Indeed. But I am following GNU behavior here. We have a test case which is failing without that lines: elf-dynamic-no-pt-dynamic.test

I.e. GNU seems to be a bit strange here, but I thought it is important to follow it for the consistency here.
Should we just check the section header and then a dynamic header (if not found) to lookup the .dynamic table?
(I am not sure so I would suggest to land this and do such change in a follow up so that we could revert it easily if that will not work for some reason).

tools/llvm-readobj/llvm-readobj.cpp
377 ↗(On Diff #199572)

Hmm. No. Looks like I tested that it does not change the return code (and does not exit after warning), i.e.
like in my code and somehow tested where do it prints the output wrong.

After retesting I see it prints warnings to stderr, indeed. Fixed, thanks!

jhenderson added inline comments.May 20 2019, 4:55 AM
test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test
11 ↗(On Diff #199572)

I agree with your comments here. It is possible to create such a test case with yaml2obj, with something like this:

Sections:
  - Name: .dynamic
    Type: SHT_DYNAMIC
    Address: 0x1000
    AddressAlign: 0x1000
    Entries:
      # ...
ProgramHeaders:
 - Type: PT_DYNAMIC
    Offset: 0x900
    Sections:
      - Section: .dynamic

Maybe it would be good for the type and name of the section to be printed (e.g. "warning: the SHT_DYNAMIC section '.dynamic' ...")

tools/llvm-readobj/ELFDumper.cpp
206–207 ↗(On Diff #199572)

Okay. How about loadDynamicTable instead of findDynamicTable? That could parse as well, but I'm okay if it doesn't.

1330–1332 ↗(On Diff #199572)

I agree with doing it as a separate change, thanks. I think there is no logic in being able to dump a section not in a segment, but requiring the segment to be present. Of course, it is weird having a .dynamic section without a PT_DYNAMIC segment at all, but if we handle one case, we might as well handle the whole behaviour.

grimar updated this revision to Diff 200278.May 20 2019, 7:37 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test
11 ↗(On Diff #199572)

I updated the warnings to include the dynamic section name.

tools/llvm-readobj/ELFDumper.cpp
206–207 ↗(On Diff #199572)

Done. (I also moved parseDynamicTable. Not sure it is much better, but isolating it in one place is also perhaps can be reasonable)

jhenderson accepted this revision.May 20 2019, 7:57 AM

LGTM, with one minor additional comment.

test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test
1 ↗(On Diff #200278)

Not sure what happened, but this file is currently showing up twice in the diff!

tools/llvm-readobj/ELFDumper.cpp
1342 ↗(On Diff #200278)

Maybe worth extending this comment with. "This matches GNU's behavior."

This revision is now accepted and ready to land.May 20 2019, 7:57 AM
grimar marked an inline comment as done.May 20 2019, 8:08 AM

Thanks for the review, James!

test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test
1 ↗(On Diff #200278)

It is just a 2 different tests with a similar names:
(or I do not observe this bug)

A M test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test (47 lines)
A M test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test (92 lines)

jhenderson added inline comments.May 20 2019, 8:19 AM
test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test
1 ↗(On Diff #200278)

Oh, right, thanks!

test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test
1 ↗(On Diff #200278)

Perhaps worth naming this elf-non-dynamic-in-pt-dynamic.test

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 8:40 AM

I have the same question as James with respect to requiring PT_DYNAMIC but otherwise the code looks good to me. I'm still looking at the tests.

I suddenly realized that forgot about this comment when committed. Sorry, Jake, if you have any post-commit review comments about the tests I'll be happy to address them.
(I am working on the follow-up patch for this, btw, which was suggested in this comment: https://reviews.llvm.org/D61937?id=199572#inline-552013. Some tests will be changed there.)