[PATCH] D70680: [ARM][Thumb2] Fix ADD/SUB invalid writes to SP

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:930
+    let Inst{31-27} = 0b11110;
+    let Inst{26}    = imm{11};
+    let Inst{25-24} = 0b01;
----------------
"let Inst{26}    = imm{11};"

This looks suspicious.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:5611
+    if (!Check(S, DecoderGPRRegisterClass(Inst, Rd, Address, Decoder)))
+      return MCDisassembler::Fail;
+  }
----------------
dnsampaio wrote:
> efriedma wrote:
> > I assume this is supposed to reject `adr sp, #label` etc.  Is this new behavior?  If it is, can you split it into a separate patch? 
> > 
> > Not sure what the `Inst.getNumOperands()` check is supposed to be doing.
> About the `Inst.getNumOperands()`, I was confused if `Inst` was empty here or not, as some case statements create new instructions instead of using this one. I replaced it by an assert that `Inst` should be empty.
> 
> Yes indeed it is a change of behavior. It will fail to accept `sp` to `thumbv7`.
> 
> Currently we have the same warning for both `thumbv7` and `thumbv8` when doing:
> llvm-mc-9 --disassemble -triple=thumbv8 -show-encoding <<< "0x0f,0xf2,0x08,0x0d"
> we obtain:
> ```
> <stdin>:1:1: warning: potentially undefined instruction encoding
> 0x0f,0xf2,0x08,0x0d
> ^
>         addw    sp, pc, #8              @ encoding: [0x0f,0xf2,0x08,0x0d]
> ```
> 
> After the patch, it will stop warning for `thumbv8` and will hard-fail for `thumbv7`. (indeed, it should not accept the softfail given by `DecoderGPRRegisterClass` ).
> 
> I can't move this changes to a distinct patch, as this code is not even executed currently. When I create the `spImm` variants in table-gen is when this decoder is actually used. Before, the instructions are either decoded as `addw` or `subw`, never as `adr.w`.
> 
Okay, makes sense.

Not sure why you need to hard-fail instead of soft-fail here for thumbv7; does something else break?  (Please add a brief comment explaining.)


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:5609
+  const unsigned Rd = fieldFromInstruction(Insn, 8, 4);
+  assert((Inst.getNumOperands() == 0) && "We should receive an empty Inst");
+  if (MCDisassembler::Success !=
----------------
Extra parentheses.


================
Comment at: llvm/test/MC/Disassembler/ARM/thumb2-v8.txt:43
+# CHECK: adr.w   sp, #1024
\ No newline at end of file

----------------
Whitespace.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70680/new/

https://reviews.llvm.org/D70680