[llvm-commits] [PATCH] load/store shift fix

[Greg Fitzgerald]

I've reviewed your patch and it looks very good.  Thanks for adding
all those unit tests, especially the adversarial tests, which we don't
see nearly enough of.

One small concern however, is that your patch catches shifts of #32 in
the parser and not during encoding.

+    if (Imm == 32)
+      Imm = 0;

This hides a subtle bug ARMMCCodeEmitter that can't be detected from
llvm-mc once your patch is in place.  LLVM's instruction selection
doesn't use the ASCII gateway to creating MCInst objects, and instead
creates them directly.  If it were to create one with #32, which
requires 6 bits instead of 5 available to encode it, then the code
emitter will foolishly go out of bounds and write over the adjacent
field.

In your patch, can you remove those lines and instead, in
ARMMCCodeEmitter::getLdStSORegOpValue():

- Binary |= ShImm << 7;
+ Binary |= (ShImm & 0x1f) << 7;

This bug, and the proposed patch, is discussed here:

http://llvm.org/bugs/show_bug.cgi?id=13240

Thanks,
Greg


On Mon, Sep 17, 2012 at 6:23 AM, Tim Northover <t.p.northover at gmail.com> wrote:
>> It fixes a problem where some load/store instructions had the “register
>> shift immediate” operand being disassembled, parsed and printed incorrectly.
>
> You can see examples of this with:
>
> $ echo "ldr r0, [r1, r2, lsr #0]" | bin/llvm-mc -arch=arm
>
> which should be a (non-canonical) alias for "ldr r0, [r1, r2, lsl #0]"
> but gets a different encoding. There are also issues around the "ror
> #0" ~ rrx case.
>
> With this patch, our exhaustive MC Hammer testing doesn't seem to find
> any more problems with this memory-based operand class (excluding the
> usual "unpredictable" issues). We're working on the similar case with
> arithmetic instructions now.
>
> Tim.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits