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

[MaskRay]

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()

[seiya]

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

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

1519

How about removing PrintedTarget?

bool PrintTarget =
    MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target);

[MaskRay]

Delete PrintedTarget

[skan]

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]

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]

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

1514–1517

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

[skan]

llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

268–276

sorry.. I meant

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

[skan]

llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

268–276

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

[MaskRay]

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]

LGTM

[jhenderson]

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]

Address comments

[MaskRay]

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.

llvm/lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

bool isConditionalBranch(const MCInst &Inst) const override {
     // BCCs with the "always" predicate are unconditional branches.
     if (Inst.getOpcode() == ARM::Bcc && Inst.getOperand(1).getImm()==ARMCC::AL)
       return false;
     return MCInstrAnalysis::isConditionalBranch(Inst);
   }
 
   bool evaluateBranch(const MCInst &Inst, uint64_t Addr,
                       uint64_t Size, uint64_t &Target) const override {
     // We only handle PCRel branches for now.
-    if (Info->get(Inst.getOpcode()).OpInfo[0].OperandType!=MCOI::OPERAND_PCREL)
+    if (Inst.getNumOperands() == 0 ||
+        Info->get(Inst.getOpcode()).OpInfo[0].OperandType !=
+            MCOI::OPERAND_PCREL)
       return false;
 
     int64_t Imm = Inst.getOperand(0).getImm();
     Target = Addr+Imm+8; // In ARM mode the PC is always off by 8 bytes.
     return true;
   }
 };
 
 class ThumbMCInstrAnalysis : public ARMMCInstrAnalysis {
 public:
   ThumbMCInstrAnalysis(const MCInstrInfo *Info) : ARMMCInstrAnalysis(Info) {}
 
   bool evaluateBranch(const MCInst &Inst, uint64_t Addr, uint64_t Size,
                       uint64_t &Target) const override {
     unsigned OpId;
     switch (Inst.getOpcode()) {
     default:
       OpId = 0;
+      if (Inst.getNumOperands() == 0)
+        return false;
       break;
     case ARM::MVE_WLSTP_8:
     case ARM::MVE_WLSTP_16:
     case ARM::MVE_WLSTP_32:
     case ARM::MVE_WLSTP_64:
     case ARM::t2WLS:
     case ARM::MVE_LETP:
     case ARM::t2LEUpdate:

llvm/test/MC/X86/tlsdesc-64.s

 # RUN: llvm-mc -triple x86_64-pc-linux-musl %s | FileCheck --check-prefix=PRINT %s
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-musl %s -o %t
 # RUN: llvm-readelf -s %t | FileCheck --check-prefix=SYM %s
 # RUN: llvm-objdump -d -r --no-show-raw-insn %t | FileCheck --match-full-lines %s
 
 # PRINT:      leaq a@tlsdesc(%rip), %rax
 # PRINT-NEXT: callq *a@tlscall(%rax)
 
 # SYM: TLS GLOBAL DEFAULT UND a
 
-# CHECK:      0: leaq (%rip), %rax
+# CHECK:      0: leaq (%rip), %rax  # 7 <{{.*}}>
 # CHECK-NEXT:   0000000000000003: R_X86_64_GOTPC32_TLSDESC a-0x4
 # CHECK-NEXT: 7: callq *(%rax)
 # CHECK-NEXT:   0000000000000007: R_X86_64_TLSDESC_CALL a
 
 leaq a@tlsdesc(%rip), %rax
 call *a@tlscall(%rax)
 addq %fs:0, %rax

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

 ## Show that references in disassembly are labelled with the correct symbol.
 # RUN: yaml2obj %s --docnum=1 -o %t
 # RUN: llvm-objdump %t -d | FileCheck %s --check-prefix=EXEC
 
-# EXEC: Disassembly of section .text1:
-# EXEC:     4000: e8 00 00 00 00                callq   0x4005 <third>
-# EXEC: Disassembly of section .text2:
-# EXEC:     4005: e8 12 34 56 78                callq   0x7856741c <fourth+0x78563412>
+# EXEC-LABEL: <first>:
+# EXEC-NEXT:   4000: e8 00 00 00 00                callq   0x4005 <third>
+# EXEC-LABEL: <third>:
+# EXEC-NEXT:   4005: e8 12 34 56 78                callq   0x7856741c <data1+0x7856241c>
+# EXEC-LABEL: <fourth>:
+# EXEC-NEXT:   400a: 8b 05 f0 0f 00 00             movl    4080(%rip), %eax  # 5000 <data1>
+# EXEC-NEXT:   4010: 8b 05 12 34 56 78             movl    2018915346(%rip), %eax  # 78567428 <data1+0x78562428>
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name:    .text1
     Type:    SHT_PROGBITS
     Address: 0x4000
     Flags:   [SHF_ALLOC, SHF_EXECINSTR]
     Content: 'e800000000' # Case 1: Referencing an address with a symbol.
   - Name:    .text2
     Type:    SHT_PROGBITS
     Flags:   [SHF_ALLOC, SHF_EXECINSTR]
     Address: 0x4005
     Content: 'e812345678' # Case 2: Referencing an address without a symbol.
   - Name:    .text3
     Type:    SHT_PROGBITS
     Flags:   [SHF_ALLOC, SHF_EXECINSTR]
     Address: 0x400A
+    Content: '8b05f00f00008b0512345678' # Case 3: Memory operands
+  - Name:    .data
+    Type:    SHT_PROGBITS
+    Flags:   [SHF_ALLOC, SHF_WRITE]
+    Address: 0x5000
 Symbols:
   - Name:    first
     Section: .text1
     Value:   0x4000
   - Name:    second
     Section: .text1
     Value:   0x4005
   - Name:    third
     Section: .text2
     Value:   0x4005
   - Name:    fourth
     Section: .text3
     Value:   0x400A
+  - Name:    data1
+    Section: .data
+    Value:   0x5000
 
 # RUN: yaml2obj %s --docnum=2 -o %t.o
 # RUN: llvm-objdump %t.o -d | FileCheck %s --check-prefix=REL
 
 # REL:      Disassembly of section .text1:
 # REL-EMPTY:
 # REL-NEXT: 0000000000000000 <.text1>:
 # REL-NEXT:        0: e8 00 00 00 00                callq   0x5 <.text1+0x5>

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

for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
 
         PIP.printInst(*IP, Disassembled ? &Inst : nullptr,
                       Bytes.slice(Index, Size),
                       {SectionAddr + Index + VMAAdjustment, Section.getIndex()},
                       outs(), "", *STI, &SP, Obj->getFileName(), &Rels);
         outs() << CommentStream.str();
         Comments.clear();
 
         // If disassembly has failed, avoid analysing invalid/incomplete
         // instruction information. Otherwise, try to resolve the target of a
         // call, tail call, etc. to a specific symbol.
-        if (Disassembled && MIA &&
-            (MIA->isCall(Inst) || MIA->isUnconditionalBranch(Inst) ||
-             MIA->isConditionalBranch(Inst))) {
+        if (Disassembled && MIA) {
           uint64_t Target;
-          if (MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target)) {
+          bool PrintedTarget =
+              MIA->evaluateBranch(Inst, SectionAddr + Index, Size, Target);
+          bool PrintTarget = PrintedTarget;
+          if (!PrintTarget)
+            if (Optional<uint64_t> MaybeTarget =
+                    MIA->evaluateMemoryOperandAddress(Inst, SectionAddr + Index,
+                                                      Size)) {
+              Target = *MaybeTarget;
+              PrintTarget = true;
+              outs() << "  # " << Twine::utohexstr(Target);
+            }
+          if (PrintTarget) {
             // In a relocatable object, the target's section must reside in
             // the same section as the call instruction or it is accessed
             // through a relocation.
             //
             // In a non-relocatable object, the target may be in any section.
             // In that case, locate the section(s) containing the target address
             // and find the symbol in one of those, if possible.
             //