[ARM][disassembler] Fix incorrect number of operands MCInst generated by the disassembler

[myhsu]

Summary

Try to fix bug 49974.

This patch fixes two issues:

1. BL does not use predicate (BL_pred is the predicate version of BL), so we shouldn't add related operands in DecodeBranchImmInstruction.
2. Inside DecodeT2AddSubSPImm, we shouldn't add predicate operands into the MCInst because ARMDisassembler::AddThumbPredicate will do that for us. However, we should handle CC-out operand for t2SUBspImm and t2AddspImm.

I can't think of a way to test this patch because all the existing disassembler tests are based on disassembled assembly code.

[myhsu]

ping.
Any comment on this fix?

[dnsampaio]

Sorry but I'm not the right person for this.

[dmgreen]

Can you add a test using llvm-mc -show-inst ?

[myhsu]

• Add test cases

[dmgreen]

Thanks, LGTM.

llvm/test/MC/Disassembler/ARM/bug49974.arm.txt

1 ↗

(On Diff #339328)

I would name these tests after the instruction they are testing, not the bug number.

llvm/test/MC/Disassembler/ARM/bug49974.thumb2.txt

15 ↗

(On Diff #339328)

May be worth adding quick tests for t2SUBspImm with and without S, to make sure they are still doing OK too. They seem fine but it can be good to have the test.

[myhsu]

Update test cases

• Use tested instructions as the file name.
• Add tests for t2SUBspImm, which is also affected by this patch

llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp

if (!tryAddingSymbolicOperand(Address, Address + SignExtend32<26>(imm) + 8,
                                   true, 4, Inst, Decoder))
     Inst.addOperand(MCOperand::createImm(SignExtend32<26>(imm)));
     return S;
   }
 
   if (!tryAddingSymbolicOperand(Address, Address + SignExtend32<26>(imm) + 8,
                                 true, 4, Inst, Decoder))
     Inst.addOperand(MCOperand::createImm(SignExtend32<26>(imm)));
+
+  // We already have BL_pred for BL w/ predicate, no need to add addition
+  // predicate opreands for BL
+  if (Inst.getOpcode() != ARM::BL)
     if (!Check(S, DecodePredicateOperand(Inst, pred, Address, Decoder)))
       return MCDisassembler::Fail;
 
   return S;
 }
 
 static DecodeStatus DecodeAddrMode6Operand(MCInst &Inst, unsigned Val,
                                    uint64_t Address, const void *Decoder) {
   DecodeStatus S = MCDisassembler::Success;
 
static DecodeStatus DecodeT2AddSubSPImm(MCInst &Inst, unsigned Insn,
   // T3 does a zext of imm12, where T2 does a ThumbExpandImm (T2SOImm)
   DecodeStatus DS = MCDisassembler::Success;
   if ((!Check(DS,
               DecodeGPRspRegisterClass(Inst, Rd, Address, Decoder))) || // dst
       (!Check(DS, DecodeGPRspRegisterClass(Inst, Rn, Address, Decoder))))
     return MCDisassembler::Fail;
   if (TypeT3) {
     Inst.setOpcode(sign1 ? ARM::t2SUBspImm12 : ARM::t2ADDspImm12);
-    S = 0;
     Inst.addOperand(MCOperand::createImm(Imm12)); // zext imm12
   } else {
     Inst.setOpcode(sign1 ? ARM::t2SUBspImm : ARM::t2ADDspImm);
     if (!Check(DS, DecodeT2SOImm(Inst, Imm12, Address, Decoder))) // imm12
       return MCDisassembler::Fail;
-  }
     if (!Check(DS, DecodeCCOutOperand(Inst, S, Address, Decoder))) // cc_out
       return MCDisassembler::Fail;
-
-  Inst.addOperand(MCOperand::createReg(0)); // pred
+  }
 
   return DS;
 }

llvm/test/MC/Disassembler/ARM/bl-arm.txt

+# RUN: llvm-mc -triple=arm -disassemble -show-inst < %s | FileCheck %s
+
+# https://bugs.llvm.org/show_bug.cgi?id=49974
+# Redundant (predicate) operands were inserted to the
+# disassembled MCInst.
+
+# CHECK:      bl  #152
+# CHECK-SAME: <MCInst #{{[0-9]+}} BL
+# CHECK-NEXT: <MCOperand Imm:152>>
+
+0x26 0x00 0x00 0xeb

llvm/test/MC/Disassembler/ARM/sub-sp-imm-thumb2.txt

+# RUN: llvm-mc -triple=thumbv7 -disassemble -show-inst < %s | FileCheck %s
+
+# https://bugs.llvm.org/show_bug.cgi?id=49974
+# Incorrect number of predicate operands were inserted to the
+# disassembled MCInst.
+
+# CHECK:      subw sp, sp, #1148
+# CHECK-SAME: <MCInst #{{[0-9]+}} t2SUBspImm12
+# CHECK-NEXT: <MCOperand Reg:15>
+# CHECK-NEXT: <MCOperand Reg:15>
+# CHECK-NEXT: <MCOperand Imm:1148>
+# CHECK-NEXT: <MCOperand Imm:14>
+# CHECK-NEXT: <MCOperand Reg:0>>
+
+0xad 0xf2 0x7c 0x4d
+
+# CHECK:      sub.w   sp, sp, #1024
+# CHECK-SAME: <MCInst #{{[0-9]+}} t2SUBspImm
+# CHECK-NEXT: <MCOperand Reg:15>
+# CHECK-NEXT: <MCOperand Reg:15>
+# CHECK-NEXT: <MCOperand Imm:1024>
+# CHECK-NEXT: <MCOperand Imm:14>
+# CHECK-NEXT: <MCOperand Reg:0>
+# CHECK-NEXT: <MCOperand Reg:0>>
+
+0xad,0xf5,0x80,0x6d
+
+# CHECK:      subs.w  sp, sp, #1024
+# CHECK-SAME: <MCInst #{{[0-9]+}} t2SUBspImm
+# CHECK-NEXT: <MCOperand Reg:15>
+# CHECK-NEXT: <MCOperand Reg:15>
+# CHECK-NEXT: <MCOperand Imm:1024>
+# CHECK-NEXT: <MCOperand Imm:14>
+# CHECK-NEXT: <MCOperand Reg:0>
+# CHECK-NEXT: <MCOperand Reg:3>>
+
+0xbd,0xf5,0x80,0x6d