ARM: Tighten up disassembling of MSR mask for M-class

[petpav01]

Summary

Allow only disassembling of M-class MSR masks that the assembler knows how to assemble back.

Note: The current code in DecodeMSRMask() rejects the unpredictable A/R MSR mask '0000' with Fail. The code in the patch follows this style and rejects unpredictable M-class MSR masks also with Fail (instead of SoftFail). If SoftFail is preferred in this case then additional changes to ARMInstPrinter (to print non-symbolic masks) and ARMAsmParser (to parse non-symbolic masks) will be needed.

[jmolloy]

Hi Petr,

Thanks for working on this. As far as the softfail stuff goes, we're not too strict on that and there are bigger problems to solve before UNPREDICTABLE all works correctly, so let's not worry about that for the moment.

I do have a few specific comments, below.

Cheers,

James

lib/Target/ARM/Disassembler/ARMDisassembler.cpp

3999

Don't need the braces here.

4009

MSR only appears to have one encoding. What are you trying to achieve by checking the opcode here?

4010

Mask is two bits, so there should be a & 3 here.

4011

Several points here:

• Please add more documentation as to what this condition is supposed to check. I think I've reverse engineered it from the ARMARM but I shouldn't have to.
• The mask values 0b01 and 0b11 are predicated on the DSP extension in the ARMARM. I *believe* we do model this extension in LLVM, so you should check it here.

[petpav01]

Hi James,

Thank you for reviewing this. I am attaching a new patch to address your comments.

Comment at: lib/Target/ARM/Disassembler/ARMDisassembler.cpp:4009
@@ +4008,3 @@
+ // Validate the 2-bit mask value in MSR for v7m.
+ if ((FeatureBits & ARM::HasV7Ops) && Inst.getOpcode() == ARM::t2MSR_M) {

+ unsigned Mask = Val >> 10;

MSR only appears to have one encoding. What are you trying to achieve by checking the opcode here?

This code path (function DecodeMSRMask()) is also used for the MRS instruction.
In that case, the 2-bit mask value will be always zero (because the MRS
instruction has no mask) and no mask validation should be done.

Thanks,
Petr

[petpav01]

r214505

lib/Target/ARM/Disassembler/ARMDisassembler.cpp

if (Val & ~0xf)
     return MCDisassembler::Fail;
 
   Inst.addOperand(MCOperand::CreateImm(Val));
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeMSRMask(MCInst &Inst, unsigned Val,
                           uint64_t Address, const void *Decoder) {
-  if (!Val) return MCDisassembler::Fail;
+  uint64_t FeatureBits = ((const MCDisassembler*)Decoder)->getSubtargetInfo()
+                                                          .getFeatureBits();
+  if (FeatureBits & ARM::FeatureMClass) {
+    unsigned ValLow = Val & 0xff;
+
+    // Validate the SYSm value first.
+    switch (ValLow) {
+    case  0: // apsr
+    case  1: // iapsr
+    case  2: // eapsr
+    case  3: // xpsr
+    case  5: // ipsr
+    case  6: // epsr
+    case  7: // iepsr
+    case  8: // msp
+    case  9: // psp
+    case 16: // primask
+    case 20: // control
+      break;
+    case 17: // basepri
+    case 18: // basepri_max
+    case 19: // faultmask
+      if (!(FeatureBits & ARM::HasV7Ops))
+        // Values basepri, basepri_max and faultmask are only valid for v7m.
+        return MCDisassembler::Fail;
+      break;
+    default:
+      return MCDisassembler::Fail;
+    }
+
+    // The ARMv7-M architecture has an additional 2-bit mask value in the MSR
+    // instruction (bits {11,10}). The mask is used only with apsr, iapsr,
+    // eapsr and xpsr, it has to be 0b10 in other cases. Bit mask{1} indicates
+    // if the NZCVQ bits should be moved by the instruction. Bit mask{0}
+    // indicates the move for the GE{3:0} bits, the mask{0} bit can be set
+    // only if the processor includes the DSP extension.
+    if ((FeatureBits & ARM::HasV7Ops) && Inst.getOpcode() == ARM::t2MSR_M) {
+      unsigned Mask = (Val >> 10) & 3;
+      if (Mask == 0 || (Mask != 2 && ValLow > 3) ||
+          (!(FeatureBits & ARM::FeatureDSPThumb2) && Mask == 1))
+        return MCDisassembler::Fail;
+    }
+  } else {
+    // A/R class
+    if (Val == 0)
+      return MCDisassembler::Fail;
+  }
   Inst.addOperand(MCOperand::CreateImm(Val));
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeDoubleRegLoad(MCInst &Inst, unsigned Insn,
                                         uint64_t Address, const void *Decoder) {
   DecodeStatus S = MCDisassembler::Success;
 

test/MC/Disassembler/ARM/invalid-thumb-MSR-MClass.txt

+# RUN: not llvm-mc -disassemble %s -triple=thumbv7-apple-darwin9 -mcpu cortex-m3 2>&1 | FileCheck %s
+
+#------------------------------------------------------------------------------
+# Undefined encodings for mrs
+#------------------------------------------------------------------------------
+
+# invalid SYSm
+# CHECK: warning: invalid instruction encoding
+# CHECK-NEXT: [0xef 0xf3 0x80 0x80]
+[0xef 0xf3 0x80 0x80]
+
+#------------------------------------------------------------------------------
+# Undefined encodings for msr
+#------------------------------------------------------------------------------
+
+# invalid mask = '00'
+# CHECK: warning: invalid instruction encoding
+# CHECK-NEXT: [0x80 0xf3 0x00 0x80]
+[0x80 0xf3 0x00 0x80]
+
+# invalid mask = '11' with SYSm not in {0..3}
+# CHECK: warning: invalid instruction encoding
+# CHECK-NEXT: [0x80 0xf3 0x04 0x8c]
+[0x80 0xf3 0x04 0x8c]
+
+# invalid mask = '01' (Cortex-M3 does not have the DSP extension)
+# CHECK: warning: invalid instruction encoding
+# CHECK-NEXT: [0x80 0xf3 0x00 0x84]
+[0x80 0xf3 0x00 0x84]
+
+# invalid SYSm
+# CHECK: warning: invalid instruction encoding
+# CHECK-NEXT: [0x80 0xf3 0x80 0x88]
+[0x80 0xf3 0x80 0x88]

test/MC/Disassembler/ARM/thumb-MSR-MClass.txt

-# RUN: llvm-mc --disassemble %s -triple=thumbv7-apple-darwin9 -mcpu cortex-m3 | FileCheck %s
+# RUN: llvm-mc --disassemble %s -triple=thumbv7-apple-darwin9 -mcpu cortex-m4 | FileCheck %s
 
-# CHECK: msr    primask, r0
-0x80 0xf3 0x10 0x80
+#------------------------------------------------------------------------------
+# MRS
+#------------------------------------------------------------------------------
 
-# CHECK: mrs    r0, primask
+# CHECK: mrs r0, apsr
+# CHECK: mrs r0, iapsr
+# CHECK: mrs r0, eapsr
+# CHECK: mrs r0, xpsr
+# CHECK: mrs r0, ipsr
+# CHECK: mrs r0, epsr
+# CHECK: mrs r0, iepsr
+# CHECK: mrs r0, msp
+# CHECK: mrs r0, psp
+# CHECK: mrs r0, primask
+# CHECK: mrs r0, basepri
+# CHECK: mrs r0, basepri_max
+# CHECK: mrs r0, faultmask
+# CHECK: mrs r0, control
+
+0xef 0xf3 0x00 0x80
+0xef 0xf3 0x01 0x80
+0xef 0xf3 0x02 0x80
+0xef 0xf3 0x03 0x80
+0xef 0xf3 0x05 0x80
+0xef 0xf3 0x06 0x80
+0xef 0xf3 0x07 0x80
+0xef 0xf3 0x08 0x80
+0xef 0xf3 0x09 0x80
 0xef 0xf3 0x10 0x80
+0xef 0xf3 0x11 0x80
+0xef 0xf3 0x12 0x80
+0xef 0xf3 0x13 0x80
+0xef 0xf3 0x14 0x80
+
+
+#------------------------------------------------------------------------------
+# MSR
+#------------------------------------------------------------------------------
+
+# CHECK: msr apsr, r0
+# CHECK: msr apsr_g, r0
+# CHECK: msr apsr_nzcvqg, r0
+
+0x80 0xf3 0x00 0x88
+0x80 0xf3 0x00 0x84
+0x80 0xf3 0x00 0x8c
+
+# CHECK: msr iapsr, r0
+# CHECK: msr iapsr_g, r0
+# CHECK: msr iapsr_nzcvqg, r0
+
+0x80 0xf3 0x01 0x88
+0x80 0xf3 0x01 0x84
+0x80 0xf3 0x01 0x8c
+
+# CHECK: msr eapsr, r0
+# CHECK: msr eapsr_g, r0
+# CHECK: msr eapsr_nzcvqg, r0
+
+0x80 0xf3 0x02 0x88
+0x80 0xf3 0x02 0x84
+0x80 0xf3 0x02 0x8c
+
+# CHECK: msr xpsr, r0
+# CHECK: msr xpsr_g, r0
+# CHECK: msr xpsr_nzcvqg, r0
+
+0x80 0xf3 0x03 0x88
+0x80 0xf3 0x03 0x84
+0x80 0xf3 0x03 0x8c
+
+# CHECK: msr ipsr, r0
+# CHECK: msr epsr, r0
+# CHECK: msr iepsr, r0
+# CHECK: msr msp, r0
+# CHECK: msr psp, r0
+# CHECK: msr primask, r0
+# CHECK: msr basepri, r0
+# CHECK: msr basepri_max, r0
+# CHECK: msr faultmask, r0
+# CHECK: msr control, r0
+
+0x80 0xf3 0x05 0x88
+0x80 0xf3 0x06 0x88
+0x80 0xf3 0x07 0x88
+0x80 0xf3 0x08 0x88
+0x80 0xf3 0x09 0x88
+0x80 0xf3 0x10 0x88
+0x80 0xf3 0x11 0x88
+0x80 0xf3 0x12 0x88
+0x80 0xf3 0x13 0x88
+0x80 0xf3 0x14 0x88