[llvm-objdump] Print target address instead of call/j/br pc-rel immediate

[sperezglz]

Summary

binutils:
400436: 74 02 je 40043a <_init+0x12>

llvm-objdump currently :
400436: 74 02 je 0x2 <_init+0x12>

Even though the symbol + offset is printed with llvm-objdump I believe it is more intuitive to see the target address as well.

[jhenderson]

I have no issues with attempting to improve GNU compatibility, but please see my inline comments, as you appear to have degraded some of the experience for one feature that is not present in GNU objdump. Also, I'm somewhat surprised that this issue doesn't affect other targets. Do they already do the right thing?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp

1341–1342

Rather than removing this comment, I think you should change it to say why you do nothing here.

1344

Rather than removing the syntax highlighting, I think it would be preferable to keep it, but perhaps in a different colour? @seiya added this as part of the GSOC work last summer, so he may be best placed to suggest an alternative.

Same comment applies elsewhere.

[MaskRay]

This is basically the largest blocker that prevents me from using llvm-objdump regularly / or why I am still using powerpc-linux-gnu-objdump -d, objdump -d, etc... I thought about fixing this, but it would require a lot of test update.

If lld is in your -DLLVM_ENABLE_PROJECTS= list, try ninja check-lld, there should be lots of failures. There is an llvm-objdump option --print-imm-hex which is also used to decide whether an immediate should be printed in the hexadecimal form, but the target InstrPrinter code may not consistently respect it. (I've updated many lld tests to use --print-imm-hex, but I believe there are still lots of remaining issues... After --print-imm-hex becomes the default, we should probably do another clean-up to remove the option)

[sperezglz]

Thanks for the feedback @jhenderson @MaskRay I will do as suggested and first try to scope the effort of making this consistant for all the remaining targets and check failing tests in other tools. I will also investigate if it is possible to achieve this without touching the target's InstPrinters and maybe adding a clo, or any approach that would cause less disruption and/or less effort for fixing potentially impacted tests. Get back to you as soon as I have something.

[sperezglz]

I used a different approach in this update, adding a command line option and leaving the target's instruction printers untouched, now unless the CLO is used the output is the same as standard, that is friendlier with existing tests. The other thing I had to do was to create an "indirect call" property, only added it to x86 , this was needed because a target address shouldn't be calculated (or printed) for indirect calls and I needed a way to identify them.

[jhenderson]

llvm/include/llvm/MC/MCInstrDesc.h

280 ↗

(On Diff #235949)

Nit: missing trailing full stop.

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

341

"tgt" is not an obvious abbreviation for "target" to me. I'd recommend just "print-target-addr" or "print-target-address".

674

This change seems unnecessary for the rest of the change?

1484

LLVM style is to use upper-case for variable names (i.e. splitInstStream -> SplitInstStream). It's also not immediately obvious what auto represents here, so consider replacing it with the full type.

That all being said, I'm not especially keen on this approach, as it seems to tie the code here to how the instructions are printed inside the corresponding instruction printer. It feels like something that could break very easily to me. That being said, I don't have a clear suggestion for an alternative at this point. @MaskRay might do.

1485–1489

You can reduce some of the duplication here by adding the comment and symbol details outside this if:

if (PrintTargetAddress) {
  auto splitInstStream = InstPrintStream.str().rsplit('\t');
  outs() << splitInstStream.first << '\t' << Twine::utohexstr(Target);
} else {
  outs() << InstPrintStream.str();
}
outs() << CommentStream.str() << SymbolDetailsStream.str();

1486

Same comment as above. InstPrinterBuffer is not going to be used again, so why clear it?

Note that the Comments string and stream are constructed outside the loop, so need to be cleared in order to be reused. You may wish to do the same thing with the other streams for performance reasons (though I don't know for sure).

1491

What's the point of this line? It's about to go out of scope.

[sperezglz]

llvm/include/llvm/MC/MCInstrDesc.h

280 ↗

(On Diff #235949)

Sure , I'll add it.

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

341

Yes that makes sense.

674

This was necessary because this function is used in the Hexagon prettyPrinter and it was printing directly to "outs()" by default while I'm sending everything to a temporary buffer so I had to add a parameter to make it flexible, this came out on a couple of Hexagon tests failing, but with this change those work fine.

1484

Yes the variable name was a typo I'll change it and add explicit type to the variable.

About the approach, in my opinion the main advantage is that the core of changes is centralized to one file and does not involve changing every target Inst Printer, the hard requirement is that there is a "\t" between instruction and immediate.

I'll fix the stuff pointed out by the comments and leave it for discussion and reference in case other suggestions come up.

1485–1489

You are right , I'll clean it up.

1486

Yeah sorry about those two, I tried to keep it safe by clearing but lost track of the scope where they live.

[MaskRay]

MIPS beq $9, $6, 1332
RISCV: beqz x10, 512
PPC: bl .+offset [1]

These are examples that a direct call/jump instruction may take more than one operands, or have a custom target address printing method. They demonstrate that llvm-objdump just lacks target-specific knowledge to make the dump look good. Instead, I think we should

• Teach MCInstPrinter::printInst and MCInstrPrinter::printInstruction to take an extra parameter Address (D72172 D72180)
• Migrate targets's *InstrInfo.td files to use a different PrintMethod, say, printBranchOperand, instead of the default printOperand.
• In printInstruction (generated by tablegen), pass Address to printBranchOperand.

  case 1:
    // PseudoFLD, PseudoFLW, PseudoFSD, PseudoFSW, PseudoSB, PseudoSD, Pseudo...
-    printBranchOperand(MI, 1, STI, O);
+    printBranchOperand(MI, 1, Address, STI, O);
    return;
    break;

[1]: PPC currently prints bl .+offset

void PPCInstPrinter::printBranchOperand(const MCInst *MI, unsigned OpNo,
                                        raw_ostream &O) {
  if (!MI->getOperand(OpNo).isImm())
    return printOperand(MI, OpNo, O);

  // Branches can take an immediate operand.  This is used by the branch
  // selection pass to print .+8, an eight byte displacement from the PC.
  O << ".";
  int32_t Imm = SignExtend32<32>((unsigned)MI->getOperand(OpNo).getImm() << 2);
  if (Imm >= 0)
    O << "+";
  O << Imm;
}

[sperezglz]

This comment was removed by sperezglz.

[sperezglz]

Oh ok, I was not aware of such examples, those cases make this approach invalid. Abandoning this, thanks for the reviews.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64InstPrinter.cpp

void AArch64InstPrinter::printVectorIndex(const MCInst *MI, unsigned OpNum,
   O << "[" << MI->getOperand(OpNum).getImm() << "]";
 }
 
 void AArch64InstPrinter::printAlignedLabel(const MCInst *MI, unsigned OpNum,
                                            const MCSubtargetInfo &STI,
                                            raw_ostream &O) {
   const MCOperand &Op = MI->getOperand(OpNum);
 
-  // If the label has already been resolved to an immediate offset (say, when
-  // we're running the disassembler), just print the immediate.
   if (Op.isImm()) {
-    O << "#" << formatImm(Op.getImm() * 4);
     return;
   }
 
   // If the branch target is simply an address then print it in hex.
   const MCConstantExpr *BranchTarget =
       dyn_cast<MCConstantExpr>(MI->getOperand(OpNum).getExpr());
   int64_t Address;
   if (BranchTarget && BranchTarget->evaluateAsAbsolute(Address)) {

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

 }
 
 void ARMInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
                                   const MCSubtargetInfo &STI, raw_ostream &O) {
   const MCOperand &Op = MI->getOperand(OpNo);
   if (Op.isReg()) {
     unsigned Reg = Op.getReg();
     printRegName(O, Reg);
-  } else if (Op.isImm()) {
-    O << markup("<imm:") << '#' << formatImm(Op.getImm()) << markup(">");
-  } else {
+  } else if (!Op.isImm()) {
     assert(Op.isExpr() && "unknown operand kind in printOperand");
     const MCExpr *Expr = Op.getExpr();
     switch (Expr->getKind()) {
     case MCExpr::Binary:
       O << '#';
       Expr->print(O, &MAI);
       break;
     case MCExpr::Constant: {

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp

 
 /// printPCRelImm - This is used to print an immediate value that ends up
 /// being encoded as a pc-relative value (e.g. for jumps and calls).  In
 /// Intel-style these print slightly differently than normal immediates.
 /// for example, a $ is not emitted.
 void X86InstPrinterCommon::printPCRelImm(const MCInst *MI, unsigned OpNo,
                                          raw_ostream &O) {
   const MCOperand &Op = MI->getOperand(OpNo);
-  if (Op.isImm())
-    O << formatImm(Op.getImm());
-  else {
+  if (!Op.isImm()) {
     assert(Op.isExpr() && "unknown pcrel immediate operand");
     // If a symbolic branch target was added as a constant expression then print
     // that address in hex.
     const MCConstantExpr *BranchTarget = dyn_cast<MCConstantExpr>(Op.getExpr());
     int64_t Address;
     if (BranchTarget && BranchTarget->evaluateAsAbsolute(Address)) {
       O << formatHex((uint64_t)Address);
     } else {

llvm/test/tools/llvm-objdump/AArch64/pc-rel-targets.test

 // RUN: llvm-objdump -d %p/Inputs/kextbundle.macho-aarch64 | FileCheck %s
 
-CHECK:  4008:       03 00 00 94     bl      #12 <_bar.stub>
+CHECK:  4008:       03 00 00 94     bl      4014 <_bar.stub>

llvm/test/tools/llvm-objdump/ARM/invalid-instruction.s

 @RUN: llvm-mc -triple arm-unknown-linux -filetype=obj %s | llvm-objdump -d - | FileCheck %s
 
 .text
   b l0
   .inst 0xffffffff
 l0:
 
-@CHECK:            0: 00 00 00 ea   b #0 <l0>
+@CHECK:            0: 00 00 00 ea   b 8 <l0>
 @CHECK-NEXT:       4: ff ff ff ff  <unknown>

llvm/test/tools/llvm-objdump/X86/coff-disassemble-export.test

 // RUN: llvm-objdump -d %p/Inputs/disassemble.dll.coff-i386 | \
 // RUN:   FileCheck %s
 
 // CHECK-LABEL: g:
-// CHECK: calll 8 <f>
+// CHECK: calll 10001010 <f>
 
 // CHECK-LABEL: f:
-// CHECK: calll -24 <g>
+// CHECK: calll 10001000 <g>

llvm/test/tools/llvm-objdump/X86/demangle.s

 # RUN: llvm-mc %s -filetype=obj -triple=x86_64-pc-linux -o %t
 # RUN: llvm-objdump -t -r --demangle %t | FileCheck %s
 
 ## Check we demangle symbols when printing symbol table.
 # CHECK:      SYMBOL TABLE:
 # CHECK-NEXT: 0000000000000000 g     F .text           00000000 foo()
 
 ## Check we demangle symbols when printing relocations.
 # CHECK:      000000000000001 R_X86_64_PLT32 foo()-0x4
 
 ## Check the case when relocations are inlined into disassembly.
 # RUN: llvm-objdump -d -r --demangle %t | FileCheck %s --check-prefix=INLINE
 # INLINE:      foo():
-# INLINE-NEXT:  0: {{.*}}  callq   0 <_Z3foov+0x5>
+# INLINE-NEXT:  0: {{.*}}  callq   5 <_Z3foov+0x5>
 # INLINE-NEXT:  0000000000000001:  R_X86_64_PLT32 foo()-0x4
 
 .text
 .globl _Z3foov
 .type _Z3foov,@function
 _Z3foov:
  callq _Z3foov@PLT

llvm/test/tools/llvm-objdump/X86/elf-disassemble-no-symtab.test

 ## Show that llvm-objdump can handle a missing symbol table when printing
 ## references and labels.
 
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objcopy -R .symtab %t
 # RUN: llvm-objdump %t -d | FileCheck %s
 
 # CHECK:       Disassembly of section .text:
 # CHECK-EMPTY:
 # CHECK-NEXT:  0000000000004000 .text:
-# CHECK-NEXT:    4000: e8 42 00 00 00                callq   66 <.text+0x47>
+# CHECK-NEXT:    4000: e8 42 00 00 00                callq   4047 <.text+0x47>
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name:    .text
     Type:    SHT_PROGBITS
     Address: 0x4000
     Flags:   [SHF_ALLOC, SHF_EXECINSTR]
     Content: 'e842000000'

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

 ## Show that --disassemble + --reloc prints relocations inline and does not dump
 ## the relocation sections.
 
 # RUN: yaml2obj %s --docnum=1 -o %t1.o
 # RUN: llvm-objdump %t1.o -d -r | FileCheck %s --implicit-check-not="RELOCATION RECORDS"
 
-# CHECK:      0: e8 00 00 00 00                callq   0 <.text+0x5>
+# CHECK:      0: e8 00 00 00 00                callq   5 <.text+0x5>
 # CHECK-NEXT:          0000000000000001:  R_X86_64_PC32        foo-0x4
 # CHECK-NEXT:          0000000000000002:  R_X86_64_NONE        bar+0x8
-# CHECK-NEXT: 5: e8 00 00 00 00                callq   0 <.text+0xa>
+# CHECK-NEXT: 5: e8 00 00 00 00                callq   a <.text+0xa>
 # CHECK-NEXT:          0000000000000006:  R_X86_64_PLT32       foo+0x1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64

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   0 <third>
+# EXEC:     4000: e8 00 00 00 00                callq   4005 <third>
 # EXEC: Disassembly of section .text2:
-# EXEC:     4005: e8 12 34 56 78                callq   2018915346 <fourth+0x78563412>
+# EXEC:     4005: e8 12 34 56 78                callq   7856741c <fourth+0x78563412>
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
- Name:    fourth
     Value:   0x400A
 
 # 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   0 <.text1+0x5>
+# REL-NEXT:        0: e8 00 00 00 00                callq   5 <.text1+0x5>
 # REL-EMPTY:
 # REL-NEXT: Disassembly of section .text2:
 # REL-EMPTY:
 # REL-NEXT: 0000000000000000 .text2:
-# REL-NEXT:        0: e8 00 00 00 00                callq   0 <sym2>
+# REL-NEXT:        0: e8 00 00 00 00                callq   5 <sym2>
 # REL-EMPTY:
 # REL-NEXT: Disassembly of section .text3:
 # REL-EMPTY:
 # REL-NEXT: 0000000000000000 .text3:
-# REL-NEXT:        0: e8 00 00 00 00                callq   0 <.text3+0x5>
+# REL-NEXT:        0: e8 00 00 00 00                callq   5 <.text3+0x5>
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:

llvm/test/tools/llvm-objdump/X86/hex-displacement.test

 # Check that target displacement has '0x' prefix
 # to accent hex format of the number.
 
 # RUN: llvm-objdump -d %p/Inputs/hello.exe.macho-i386 | FileCheck %s
 
-# CHECK: 1f47:   e8 00 00 00 00  calll   0 <_main+0xc>
+# CHECK: 1f47:   e8 00 00 00 00  calll   1f4c <_main+0xc>

llvm/test/tools/llvm-objdump/X86/section-filter-relocs.test

 ## Test that --section works correctly for -d with -r.
 # RUN: yaml2obj %s -o %t.o
 
 ## Show that only the specified sections are disassembled.
 ## Also show that no relocation sections are dumped because none are
 ## specified.
 # RUN: llvm-objdump -d -r %t.o --section=.text --section=.rodata \
 # RUN:   | FileCheck %s --check-prefixes=DISASM,RELOC --implicit-check-not=.text2
 
 # DISASM:       Disassembly of section .text:
 # DISASM-EMPTY:
 # DISASM-NEXT:  0000000000000400 .text:
-# DISASM-NEXT:  400: e8 00 00 00 00                callq   0 <.text+0x5>
+# DISASM-NEXT:  400: e8 00 00 00 00                callq   405 <.text+0x5>
 # RELOC-NEXT:                      00000401:  R_X86_64_PC32        foo+0x1
 # RELOC-NEXT:                      00000401:  R_X86_64_GOT32       foo
 # DISASM:       Disassembly of section .rodata:
 # DISASM-EMPTY:
 # DISASM-NEXT:  0000000000000000 .rodata:
 # DISASM-NEXT:  0: 00 00                           addb    %al, (%rax)
 # RELOC-NEXT:              0000000000000000:  R_X86_64_NONE        foo
 # DISASM-NEXT:  2: 00 00                           addb    %al, (%rax)

llvm/test/tools/llvm-objdump/call-absolute-symbol-elf.test

 // REQUIRES: x86_64-linux
 // RUN: llvm-objdump --disassemble %p/Inputs/call-absolute-symbol.elf-x86_64 | FileCheck %s
-CHECK: 201000:  e8 fb f0 df ff  callq -2100997 <foo>
+CHECK: 201000:  e8 fb f0 df ff  callq 100 <foo>

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

static cl::opt<bool>
     Wide("wide", cl::desc("Ignored for compatibility with GNU objdump"),
          cl::cat(ObjdumpCat));
 static cl::alias WideShort("w", cl::Grouping, cl::aliasopt(Wide));
 
 static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 
 static StringSet<> DisasmFuncsSet;
 static StringSet<> FoundSectionSet;
 static StringRef ToolName;
 
 typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
 
 namespace {
 struct FilterResult {
   // True if the section should not be skipped.
   bool Keep;
static bool isArmElf(const ObjectFile *Obj) {
   return Elf && Elf->getEMachine() == ELF::EM_ARM;
 }
 
 static bool hasMappingSymbols(const ObjectFile *Obj) {
   return isArmElf(Obj) || isAArch64Elf(Obj);
 }
 
 static void printRelocation(StringRef FileName, const RelocationRef &Rel,
                             uint64_t Address, bool Is64Bits) {
   StringRef Fmt = Is64Bits ? "\t\t%016" PRIx64 ":  " : "\t\t\t%08" PRIx64 ":  ";
   SmallString<16> Name;
   SmallString<32> Val;
   Rel.getTypeName(Name);
   if (Error E = getRelocationValueString(Rel, Val))
     reportError(std::move(E), FileName);
   outs() << format(Fmt.data(), Address) << Name << "\t" << Val << "\n";
 }
#endif
                   [=](const std::tuple<uint64_t, StringRef, uint8_t> &O) {
                     return std::get<0>(O) <= Target;
                   });
             }
             if (TargetSym != TargetSectionSymbols->begin()) {
               --TargetSym;
               uint64_t TargetAddress = std::get<0>(*TargetSym);
               StringRef TargetName = std::get<1>(*TargetSym);
-              outs() << " <" << TargetName;
+              outs() << Twine::utohexstr(Target) << " <" << TargetName;
               uint64_t Disp = Target - TargetAddress;
               if (Disp)
                 outs() << "+0x" << Twine::utohexstr(Disp);
               outs() << '>';
             }
           }
         }
         outs() << "\n";
 
         // Hexagon does this in pretty printer
         if (Obj->getArch() != Triple::hexagon) {
           // Print relocation for instruction.
           while (RelCur != RelEnd) {
             uint64_t Offset = RelCur->getOffset();
             // If this relocation is hidden, skip it.
             if (getHidden(*RelCur) || SectionAddr + Offset < StartAddress) {
               ++RelCur;
               continue;
             }
 
             // Stop when RelCur's offset is past the current instruction.
             if (Offset >= Index + Size)
               break;