[objdump][ARM] Use correct offset when printing ARM/Thumb branch targets

[ostannard]

Summary

llvm-objdump only uses one MCInstrAnalysis object, so if ARM and Thumb
code is mixed in one object, or if an object is disassembled without
explicitly setting the triple to match the ISA used, then branch and
call targets will be printed incorrectly.

This could be fixed by creating two MCInstrAnalysis objects in
llvm-objdump, like we currently do for SubtargetInfo. However, I don't
think there's any reason we need two separate sub-classes of
MCInstrAnalysis, so instead these can be merged into one, and the ISA
determined by checking the opcode of the instruction.

[ostannard]

• Test with both ARM and Thumb triples
• Fix failing lld test, where this bug was causing the wrong symbols to be displayed

[simon_tatham]

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

423

Is it possible to find out from the MCInstrDesc whether something's an Arm or a Thumb instruction? If so, you could set Offset based on that, and then you wouldn't have to get it right in each individual switch case, including future updates.

It looks as if (Desc.TSFlags & FormMask) == ThumbFrm ought to do that, but that is based on quite a quick look, so I might be wrong about what it means.

[MaskRay]

llvm/test/tools/llvm-objdump/ELF/ARM/branch-symbols.s

2

Tip: --no-show-raw-insn to omit hex bytes. They are separately tested by test/MC/ARM encoding tests, so no need to duplicate in llvm-objdump tests.

11

Another pain is that arm prints relative offsets, instead of absolute addresses.
Is anyone going to claim the work?

[ostannard]

llvm/test/tools/llvm-objdump/ELF/ARM/branch-symbols.s

11

I'm not planning on changing that, and I'm not sure that would be the right thing to do given that the assembler interprets immediates as relative offsets.

[ostannard]

• Use TSFlags to check for thumb instructions, and walk operands to find the immediate, to avoid needing to keep the switch statement up to date
• Don't include instruction encodings in test

[simon_tatham]

Very nice – it hadn't even occurred to me that you might be able to find the branch-target operand automatically as well as identifying Arm/Thumb.

llvm/lib/Target/ARM/ARMInstrInfo.td

 def BXJ : ABI<0b0001, (outs), (ins GPR:$func), NoItinerary, "bxj", "\t$func",
               [/* pattern left blank */]>, Sched<[WriteBr]> {
   bits<4> func;
   let Inst{23-20} = 0b0010;
   let Inst{19-8} = 0xfff;
   let Inst{7-4} = 0b0010;
   let Inst{3-0} = func;
   let isBranch = 1;
+  let isIndirectBranch = 1;
 }
 
 // Tail calls.
 
 let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [SP] in {
   def TCRETURNdi : PseudoInst<(outs), (ins i32imm:$dst), IIC_Br, []>,
                    Sched<[WriteBr]>;
 

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

public:
 
   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 (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;
+    uint64_t Offset;
+
     switch (Inst.getOpcode()) {
-    default:
+    case ARM::B:
+    case ARM::Bcc:
+    case ARM::BL:
+    case ARM::BLXi:
+    case ARM::BL_pred:
       OpId = 0;
-      if (Inst.getNumOperands() == 0)
-        return false;
+      Offset = 8;
+      break;
+    case ARM::tB:
+    case ARM::tBcc:
+    case ARM::t2B:
+    case ARM::t2Bcc:
+    case ARM::t2LE:
+      OpId = 0;
+      Offset = 4;
+      break;
+    case ARM::tCBZ:
+    case ARM::tCBNZ:
+      OpId = 1;
+      Offset = 4;
       break;
+    case ARM::tBL:
+    case ARM::tBLXi:
+    case ARM::MVE_LETP:
     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:
       OpId = 2;
+      Offset = 4;
       break;
-    case ARM::t2LE:
-      OpId = 1;
-      break;
+    default:
+      // The above should handle all direct branches.
+      const MCInstrDesc &Desc = Info->get(Inst.getOpcode());
+      assert((!Desc.isBranch() || Desc.isIndirectBranch()) &&
+             "Unhandled direct branch");
+      return false;
     }
 
     // We only handle PCRel branches for now.
     if (Info->get(Inst.getOpcode()).OpInfo[OpId].OperandType !=
         MCOI::OPERAND_PCREL)
       return false;
 
-    // In Thumb mode the PC is always off by 4 bytes.
-    Target = Addr + Inst.getOperand(OpId).getImm() + 4;
+    Target = Addr + Inst.getOperand(OpId).getImm() + Offset;
     return true;
   }
 };
 
 }
 
 static MCInstrAnalysis *createARMMCInstrAnalysis(const MCInstrInfo *Info) {
   return new ARMMCInstrAnalysis(Info);
 }
 
-static MCInstrAnalysis *createThumbMCInstrAnalysis(const MCInstrInfo *Info) {
-  return new ThumbMCInstrAnalysis(Info);
-}
-
 bool ARM::isCDECoproc(size_t Coproc, const MCSubtargetInfo &STI) {
   // Unfortunately we don't have ARMTargetInfo in the disassembler, so we have
   // to rely on feature bits.
   if (Coproc >= 8)
     return false;
   return STI.getFeatureBits()[ARM::FeatureCoprocCDE0 + Coproc];
 }
 
for (Target *T : {&getTheARMLETarget(), &getTheARMBETarget(),
     // Register the MCInstPrinter.
     TargetRegistry::RegisterMCInstPrinter(*T, createARMMCInstPrinter);
 
     // Register the MC relocation info.
     TargetRegistry::RegisterMCRelocationInfo(*T, createARMMCRelocationInfo);
   }
 
   // Register the MC instruction analyzer.
-  for (Target *T : {&getTheARMLETarget(), &getTheARMBETarget()})
+  for (Target *T : {&getTheARMLETarget(), &getTheARMBETarget(),
+                    &getTheThumbLETarget(), &getTheThumbBETarget()})
     TargetRegistry::RegisterMCInstrAnalysis(*T, createARMMCInstrAnalysis);
-  for (Target *T : {&getTheThumbLETarget(), &getTheThumbBETarget()})
-    TargetRegistry::RegisterMCInstrAnalysis(*T, createThumbMCInstrAnalysis);
 
   for (Target *T : {&getTheARMLETarget(), &getTheThumbLETarget()}) {
     TargetRegistry::RegisterMCCodeEmitter(*T, createARMLEMCCodeEmitter);
     TargetRegistry::RegisterMCAsmBackend(*T, createARMLEAsmBackend);
   }
   for (Target *T : {&getTheARMBETarget(), &getTheThumbBETarget()}) {
     TargetRegistry::RegisterMCCodeEmitter(*T, createARMBEMCCodeEmitter);
     TargetRegistry::RegisterMCAsmBackend(*T, createARMBEAsmBackend);
   }
 }

llvm/test/tools/llvm-objdump/ELF/ARM/branch-symbols.s

+@ RUN: llvm-mc < %s --triple=armv8a -mattr=+mve,+lob -filetype=obj | llvm-objdump -dr - --triple armv8a --mattr=+mve,+lob | FileCheck %s
+
+foo:
+
+  // Branches
+  .arm
+  b foo
+  ble foo
+@ CHECK:       0: fe ff ff ea   b       #-8 <foo>
+@ CHECK:       4: fd ff ff da   ble     #-12 <foo>
+
+  .thumb
+  b foo
+  b.w foo
+  ble foo
+  ble.w foo
+  le foo
+  le lr, foo
+  cbz r0, bar
+  cbnz r0, bar
+@ CHECK:       8: fa e7         b       #-12 <foo>
+@ CHECK:       a: ff f7 f9 bf   b.w     #-14 <foo>
+@ CHECK:       e: f7 dd         ble     #-18 <foo>
+@ CHECK:      10: 7f f7 f6 af   ble.w   #-20 <foo>
+@ CHECK:      14: 2f f0 0d c0   le      #-24 <foo>
+@ CHECK:      18: 0f f0 0f c0   le      lr, #-28 <foo>
+@ CHECK:      1c: a0 b1         cbz     r0, #40 <bar>
+@ CHECK:      1e: 98 b9         cbnz    r0, #38 <bar>
+
+  // Calls without relocations (these offsets al correspond to label foo).
+  .arm
+  bl #-40
+  blx #-44
+  bleq #-48
+@ CHECK:      20: f6 ff ff eb   bl      #-40 <foo>
+@ CHECK:      24: f5 ff ff fa   blx     #-44 <foo>
+@ CHECK:      28: f4 ff ff 0b   bleq    #-48 <foo>
+
+  .thumb
+  bl #-48
+  blx #-52
+@ CHECK:      2c: ff f7 e8 ff   bl      #-48 <foo>
+@ CHECK:      30: ff f7 e6 ef   blx     #-52 <foo>
+
+  // Calls with relocations. These currently emit a reference to their own
+  // location, because we don't take relocations into account when printing
+  // branch targets.
+  .arm
+  bl baz
+  blx baz
+  bleq baz
+@ CHECK:      34: fe ff ff eb   bl      #-8 <$a.4>
+@ CHECK:                        00000034:  R_ARM_CALL   baz
+@ CHECK:      38: fe ff ff fa   blx     #-8 <$a.4+0x4>
+@ CHECK:                        00000038:  R_ARM_CALL   baz
+@ CHECK:      3c: fe ff ff 0b   bleq    #-8 <$a.4+0x8>
+@ CHECK:                        0000003c:  R_ARM_JUMP24 baz
+
+  .thumb
+  bl baz
+  blx baz
+@ CHECK:      40: ff f7 fe ff   bl      #-4 <$t.5>
+@ CHECK:                        00000040:  R_ARM_THM_CALL       baz
+@ CHECK:      44: ff f7 fe ef   blx     #-4 <$t.5+0x4>
+@ CHECK:                        00000044:  R_ARM_THM_CALL       baz
+
+bar:
+
+