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

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio added inline comments.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:5611
+    if (!Check(S, DecoderGPRRegisterClass(Inst, Rd, Address, Decoder)))
+      return MCDisassembler::Fail;
+  }
----------------
efriedma wrote:
> 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.)
Indeed it won't break, is just that I didn't realize that the standard was to emit a warning, instead of an error. Fixing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70680