[PATCH] D70680: [ARM][Thumb2] Fix ADD/SUB invalid writes to SP
Diogo N. Sampaio via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 9 01:49:34 PST 2020
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
More information about the llvm-commits
mailing list