Page MenuHomePhabricator

Implemented sane default for llvm-objdump's relocation Value format
ClosedPublic

Authored by dcederman on Aug 8 2017, 6:41 AM.

Details

Summary

"Unknown" for platforms that were not manually added into the switch
did not make sense at all. Now it prints Target + addend for all
elf-machines that were not explicitly mentioned.

Addresses PR21059 and PR25124.

Diff Detail

Event Timeline

fedor.sergeev created this revision.Aug 8 2017, 6:41 AM
fedor.sergeev edited the summary of this revision. (Show Details)EditedAug 8 2017, 6:44 AM

Initially this problem was mentioned to me by James during https://reviews.llvm.org/D35567 review.
I found at least 2 PRs that mention objdump and Unknown.
Perhaps there are more, as this deficiency is rather obvious in code.

jyknight added inline comments.Oct 17 2017, 7:58 AM
tools/llvm-objdump/llvm-objdump.cpp
693

And after the other changes, I think this can go back to an int64_t.

739

This "-P" suffix here doesn't seem particularly useful, I'd suggest just removing it.

The reloc name is printed, I don't think the fact that PC is subtracted needs to be printed as part of the output here as well. (Perhaps the intent was to print the "full formula", but if so, this list is woefully incomplete, and I don't think that's particularly worthwhile to spend time implementing).

745

I believe this list of case statements which override addend = None is bogus and should go away. Most of these have SHT_REL relocations, which put the addend in the instruction -- and thus don't get printed anyways (see also TODO note above -- although I'd note that GNU objdump also doesn't print SHT_REL addends either). And when they _do_ actually have addends, I see no reason why we would not want them printed.

Other than that, looks good.

espindola edited reviewers, added: espindola; removed: rafael.Mar 15 2018, 9:16 AM
dcederman commandeered this revision.May 31 2018, 3:30 AM
dcederman added a reviewer: fedor.sergeev.
dcederman added a subscriber: dcederman.

Hello,

I got referred to this revision by @jyknight after I created a patch to add Sparc to the list in getRelocationValueString. Changing the default does seem like a better approach, so I will update this revision following the comment. I hope that is ok.

dcederman updated this revision to Diff 149259.May 31 2018, 3:31 AM
jyknight accepted this revision.May 31 2018, 8:42 AM
This revision is now accepted and ready to land.May 31 2018, 8:42 AM
This revision was automatically updated to reflect the committed changes.