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

[Greg Fitzgerald]

> The InstPrinter and the MCCodeEmitter should probably have assert()s in
> there to validate the range of the immediate operand to help catch such
> bugs, but I suspect they don't.

Thank you Jim.

If ldr (with shifted reg), str, pld, pli are not all currently used by
instruction selection, we should add those assert's.  While the code
rarely does this, this particular case is especially error-prone in
that 32 is a valid input, but if written as 32, it'll go out of bounds
and set the instruction's 'isAdd' bit.

So the only thing really needed is in ARMMCCodeEmitter::getLdStSORegOpValue():

assert(ShImm & ~0x1f == 0);

Jim, if you don't mind adding that, I think this patch is ready to go.

Thanks,
Greg

On Thu, Sep 20, 2012 at 5:45 PM, Jim Grosbach <grosbach at apple.com> wrote:
>
> On Sep 20, 2012, at 5:03 PM, Greg Fitzgerald <garious at gmail.com> wrote:
>
>>> + "lsr #32" gives an MCInst immediate of 0. Encoder and Decoder can be
>>> as naive as you like. Printer, parser and selection have to be
>>> special.
>>>
>>> I think the second is more in line with the goals of the MC layer.
>>
>
> This is correct.
>
>
>> That'd be fine, but what are you doing to prevent the MCOperand from
>> being constructed with #32?  Your patch guards llvm-mc from doing it
>> in the AsmParser, but what about llc?
>
> That would be a bug in ISel.
>
> The InstPrinter and the MCCodeEmitter should probably have assert()s in there to validate the range of the immediate operand to help catch such bugs, but I suspect they don't.
>
> -Jim
>
>>
>> -Greg
>>
>>
>> On Thu, Sep 20, 2012 at 1:40 PM, Tim Northover <t.p.northover at gmail.com> wrote:
>>> Hi Greg,
>>>
>>> I'm afraid I disagree with this issue too.
>>>
>>>> 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.
>>>
>>> This all comes down to what we want the canonical MCInst for these
>>> instructions to be. Our two options are:
>>>
>>> + "lsr #32" (etc) gives an MCInst immediate of 32. Encoder and decoder
>>> are special (if the decoder isn't special, MCInsts are no longer
>>> canonical since "lsr #32" will be decoded to an immediate of 0).
>>> Printer and Parser can forget about it. Selection equally special
>>> here: "lsr #0" needs special handling to be converted to an MCOperand
>>> of 0.
>>> + "lsr #32" gives an MCInst immediate of 0. Encoder and Decoder can be
>>> as naive as you like. Printer, parser and selection have to be
>>> special.
>>>
>>> I think the second is more in line with the goals of the MC layer.
>>>
>>> Tim.
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>