Page MenuHomePhabricator

[llvm-objdump] Fix 'llvm-objdump -dr' for executables with relocations
ClosedPublic

Authored by maksfb on Aug 31 2021, 12:15 PM.

Details

Summary

Print relocations interleaved with disassembled instructions for
executables with relocatable sections, e.g. those built with "-Wl,-q".

pr/51684

Diff Detail

Event Timeline

maksfb created this revision.Aug 31 2021, 12:15 PM
maksfb requested review of this revision.Aug 31 2021, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 12:15 PM
nickdesaulniers accepted this revision.Aug 31 2021, 2:12 PM

Thanks @maksfb , this fixes the issue reported in https://bugs.llvm.org/show_bug.cgi?id=51684. Perhaps you can add pr/51684 to the description+commit message just as a cross reference?

Perhaps it would be good to wait for an additional code review from folks more knowledgeable than myself about this code before merging?

This revision is now accepted and ready to land.Aug 31 2021, 2:12 PM
MaskRay requested changes to this revision.Aug 31 2021, 2:36 PM
MaskRay added inline comments.
llvm/test/tools/llvm-objdump/ELF/exec-relocations.test
54 ↗(On Diff #369761)

Use just puts

Versioned symbols are not encoded this way.

57 ↗(On Diff #369761)

This is not needed

This revision now requires changes to proceed.Aug 31 2021, 2:36 PM
MaskRay added inline comments.Aug 31 2021, 2:38 PM
llvm/test/tools/llvm-objdump/ELF/exec-relocations.test
41 ↗(On Diff #369761)

The content should be simplified. Having one byte should be sufficient.

Every long stream of opaque bytes if a potential future maintenance burden.

maksfb updated this revision to Diff 369803.Aug 31 2021, 2:52 PM
maksfb edited the summary of this revision. (Show Details)

Address comments.

maksfb marked 3 inline comments as done.Aug 31 2021, 2:52 PM
MaskRay accepted this revision.Aug 31 2021, 6:44 PM

It would be good to wait for @jhenderson as well.

llvm/test/tools/llvm-objdump/ELF/exec-relocations.test
65 ↗(On Diff #369803)

remove unused main

llvm/tools/llvm-objdump/llvm-objdump.cpp
1294

// In executable and shared objects, r_offset holds a virtual address. Subtract SectionAddr from the r_offset field of a relocation to get the section offset.

This revision is now accepted and ready to land.Aug 31 2021, 6:44 PM
jhenderson added inline comments.Sep 1 2021, 12:59 AM
llvm/test/tools/llvm-objdump/ELF/exec-relocations.test
1 ↗(On Diff #369803)

I think this test needs moving into llvm-objdump/X86, as it disassembles X86 and therefore requires that target. You can maybe rename it elf-disassemble-relocs-exec.test to mirror the existing elf-disassemble-relocs.test found therein.

17 ↗(On Diff #369803)

You don't need a specific entry point to be defined for this test. This can be omitted.

19 ↗(On Diff #369803)

You don't need this program header. It shouldn't directly impact the disassembly + relocations.

28 ↗(On Diff #369803)

There's no need for this alignment value. Just omit it.

33 ↗(On Diff #369803)

If memory serves me rightly, yaml2obj automatically sets the Address values, based on the program header address value the section is in. I might be misremembering though, so maybe best do some experimentation where one or both sections are missing the address, and see what the output section addresses look like.

35 ↗(On Diff #369803)

You should be able to simplify this content, so only a small amount is needed (specifically for the two instructions being checked, plus maybe a simple nop instruction before and after, to show relocations aren't printed in the wrong place).

45 ↗(On Diff #369803)

You can omit this. yaml2obj sets relocation section Link fields to .symtab by default.

54 ↗(On Diff #369803)

No need to quote the name.

62 ↗(On Diff #369803)

You don't need to quote the name.

maksfb updated this revision to Diff 370455.Sep 2 2021, 5:24 PM

Address comments.

maksfb marked 11 inline comments as done.Sep 2 2021, 5:26 PM

One small suggestion, otherwise this looks fine.

llvm/test/tools/llvm-objdump/X86/elf-disassemble-relocs-exec.test
31

Consider adding a leading nop too. You need to show that a relocation isn't printed before its correct location (in other words, the leading nop would have no associated relocation).

maksfb updated this revision to Diff 371129.Sep 7 2021, 11:01 AM

Add a leading nop for the test case.

maksfb marked an inline comment as done.Sep 7 2021, 11:01 AM
This revision was landed with ongoing or failed builds.Sep 7 2021, 11:29 AM
This revision was automatically updated to reflect the committed changes.

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

maksfb added a comment.Sep 7 2021, 4:11 PM

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

No problem. Good point, ideally it should be cherry-picked. Do you know what's the logistics for that?

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

No problem. Good point, ideally it should be cherry-picked. Do you know what's the logistics for that?

I think https://llvm.org/docs/HowToReleaseLLVM.html#merge-requests is the official documentation, although I wouldn't be surprised if it's somewhat out-of-date, given it seems to be using SVN revision numbers! The essence though is that the release manager needs to be advised of which commit to cherry-pick.

maksfb added a comment.Sep 8 2021, 1:51 PM

thanks for the quick fix; does this still need to be cherry-picked to release/13.x to close out https://bugs.llvm.org/show_bug.cgi?id=51684?

No problem. Good point, ideally it should be cherry-picked. Do you know what's the logistics for that?

I think https://llvm.org/docs/HowToReleaseLLVM.html#merge-requests is the official documentation, although I wouldn't be surprised if it's somewhat out-of-date, given it seems to be using SVN revision numbers! The essence though is that the release manager needs to be advised of which commit to cherry-pick.

Thanks. I see the commit was cherry-picked. Likely because the bug was marked as blocking the release.