Page MenuHomePhabricator

[llvm-readelf/llvm-objdump] - Improve/refactor the implementation of SHT_LLVM_ADDRSIG section dumping.
ClosedPublic

Authored by grimar on Oct 3 2019, 4:58 AM.

Details

Summary

This patch:

  • Adds a llvm-readobj/llvm-readelf test file for SHT_LLVM_ADDRSIG sections. (we do not have any)
  • Enables dumping of SHT_LLVM_ADDRSIG with --all.
  • Changes the logic to report a warning instead of an error when something goes wrong during dumping (allows to continue dumping SHT_LLVM_ADDRSIG and other sections on error).
  • Refactors a piece of logic to a new toULEB128Array helper which might be used for GNU-style dumping implementation.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Oct 3 2019, 4:58 AM
jhenderson added inline comments.Oct 3 2019, 6:11 AM
test/tools/llvm-readobj/elf-addrsig.test
1 ↗(On Diff #222987)

'##'

FWIW, it looks like there's some coverage in the MC tests, but I think it makes sense to have a dedicated test here.

30 ↗(On Diff #222987)

mailformed -> malformed

33 ↗(On Diff #222987)

MAILFORMED -> MALFORMED

80 ↗(On Diff #222987)

llvm-readelf does not dump it, and should not dump it either with --all (see also the discussion about .stack_sizes dumping), so this comment should be updated to say so.

82 ↗(On Diff #222987)

Why rebuild?

grimar marked an inline comment as done.Oct 3 2019, 6:19 AM
grimar added inline comments.
test/tools/llvm-readobj/elf-addrsig.test
80 ↗(On Diff #222987)

Yes. What I meant is that when it will start dumping it with --addrsig, it will be possible to replace --implicit-check-not=warning --implicit-check-not=error with something better like --implicit-check-not=addrsig_header_blah_blah

I'll update the comment.

grimar updated this revision to Diff 223175.Oct 4 2019, 2:08 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
  • Updated --all test case.
test/tools/llvm-readobj/elf-addrsig.test
82 ↗(On Diff #222987)

No need to rebuild + I've moved this test right below the 1st test.

grimar edited the summary of this revision. (Show Details)Oct 4 2019, 2:09 AM
jhenderson added inline comments.Oct 4 2019, 2:37 AM
test/tools/llvm-readobj/elf-addrsig.test
30 ↗(On Diff #223175)

dumps any SHT_LLVM_ADDRSIG section

31 ↗(On Diff #223175)

This is a little confusing to read to me (I originally thought "--all dumps it but llvm-readelf doesn't - but llvm-readelf is not an option of llvm-readobj"). Perhaps replacing "when --all..." with "when --all is specified for LLVM style, but not for GNU style" would be better?

32 ↗(On Diff #223175)

be implemented -> is implemented

grimar updated this revision to Diff 223204.Oct 4 2019, 6:29 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Oct 4 2019, 8:37 AM

LGTM.

test/tools/llvm-readobj/elf-addrsig.test
31 ↗(On Diff #223204)

Nit: missing trailing full stop.

This revision is now accepted and ready to land.Oct 4 2019, 8:37 AM
MaskRay accepted this revision.Oct 5 2019, 10:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 10:08 PM