Page MenuHomePhabricator

[llvm-objdump] Print target address with evaluateMemoryOperandAddress()
ClosedPublic

Authored by MaskRay on Apr 23 2020, 5:44 PM.

Details

Summary

D63847 added MCInstrAnalysis::evaluateMemoryOperandAddress(). This patch
leverages the feature to print the target addresses for evaluable instructions.

-400a: movl 4080(%rip), %eax
+400a: movl 4080(%rip), %eax  # 5000 <data1>

This patch also deletes MIA->isCall(Inst) || MIA->isUnconditionalBranch(Inst) || MIA->isConditionalBranch(Inst)
which is used to guard MCInstrAnalysis::evaluateBranch()

Diff Detail

Event Timeline

MaskRay created this revision.Apr 23 2020, 5:44 PM
seiya added a comment.Apr 25 2020, 6:46 AM

Nitpick in summary: Perhaps you might want to refer to D63847 instead of D63746?

llvm/tools/llvm-objdump/llvm-objdump.cpp
1521–1531

How about removing PrintedTarget?

bool PrintTarget =
    MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target);
MaskRay edited the summary of this revision. (Show Details)Apr 25 2020, 8:57 AM
MaskRay updated this revision to Diff 260112.Apr 25 2020, 9:11 AM
MaskRay marked an inline comment as done.

Delete PrintedTarget

skan added inline comments.Apr 25 2020, 9:28 PM
llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
268–276

I have two questions about this change.

  1. Is there any instruction that does not have any operands for ARM backend?
  2. If the only difference between the specialized evaluateBranch and the gerneral one was that

In ARM mode the PC is always off by 8 bytes.

could we use MCInstrAnalysis::evaluateMemoryOperandAddress(Inst, Addr, 8 , Target) here?

MaskRay marked an inline comment as done.Apr 25 2020, 9:56 PM
MaskRay added inline comments.
llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
268–276

Is there any instruction that does not have any operands for ARM backend?

Yes, e.g. nop

could we use MCInstrAnalysis::evaluateMemoryOperandAddress(Inst, Addr, 8 , Target) here?

No. The ARM backend does not derive from MCInstrAnalysis::evaluateMemoryOperandAddress.

skan added inline comments.Apr 25 2020, 10:15 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1515–1516

It would be nice if the comments updates along with the change that removes the limit of call/jcc/jmp.

skan added inline comments.Apr 25 2020, 10:29 PM
llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
268–276

sorry.. I meant

could we use MCInstrAnalysis::evaluateBranch(Inst, Addr, 8 , Target) here?

skan added inline comments.Apr 25 2020, 11:00 PM
llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
268–276

class ARMMCInstrAnalysis : public MCInstrAnalysis , ARMMCInstrAnalysis is derived from MCInstrAnalysis.

MaskRay marked 2 inline comments as done.Apr 25 2020, 11:43 PM
MaskRay added inline comments.
llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp
268–276

No. Actually the current MCInstrAnalysis::evaluateBranch implementation is X86 specific. See how it evaluates the target address.

skan accepted this revision.Apr 26 2020, 12:18 AM

LGTM

This revision is now accepted and ready to land.Apr 26 2020, 12:18 AM
jhenderson accepted this revision.Apr 27 2020, 1:20 AM

Looks reasonable to me, with the tweak to the comment @skan suggested. Having the extra information would certainly be useful.

llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbol-references.yaml
10–11

Do we need both cases here? Does having both provide additional coverage above the previous two cases?

MaskRay updated this revision to Diff 260359.Apr 27 2020, 9:43 AM
MaskRay marked 5 inline comments as done.

Address comments

MaskRay added inline comments.Apr 27 2020, 9:47 AM
llvm/test/tools/llvm-objdump/X86/elf-disassemble-symbol-references.yaml
10–11

I was undecided. Thanks for giving your input and I'll delete the second case.

This revision was automatically updated to reflect the committed changes.