Re: -Wtautological-bitwise-compare error in arm-tdep.c

[Alan Hayward <Alan.Hayward@arm.com>]

> On 26 May 2020, at 13:17, Luis Machado <luis.machado@linaro.org> wrote:
> 
> On 5/26/20 6:45 AM, Alan Hayward wrote:
>>> On 25 May 2020, at 14:51, Luis Machado <luis.machado@linaro.org> wrote:
>>> 
>>> On 5/25/20 10:49 AM, Simon Marchi wrote:
>>>> On 2020-05-25 9:08 a.m., Luis Machado wrote:
>>>>> This fixes an instruction mask typo. We should be matching only
>>>>> ldrd (immediate) and not any other of its variants. As is, it never matches
>>>>> anything.
>>>> And moreover, within the `ldrd (immediate)` instruction, it only matches the
>>>> `Offset variant` variant, right?
>>> 
>>> That's right. We don't want to handle anything that changes the SP here. And the post-indexed and pre-indexed variants do so.
>>> 
>>>>> 
>>>>> With the patch, the instruction mask also allows matching of ldrd (literal),
>>>>> but the check for SP discards this particular instruction pattern, as it has
>>>>> a hardcoded PC register.
>>>> I don't feel the most qualified to approve this patch.  Alan, could you please
>>>> take a look?
>>>> Simon
>> The maths looks good now.
>> However, Binutils uses a slightly different mask, 0xff50:
>>   {ARM_FEATURE_CORE_LOW (ARM_EXT_V6T2),
>>     0xe9500000, 0xff500000,
>>     "ldrd%c\t%12-15r, %8-11r, [%16-19r, #%23`-%0-7W]%21'!%L”},
>> It does use 0xff70 for a different variation of ldrd:
>>   {ARM_FEATURE_CORE_LOW (ARM_EXT_V6T2),
>>     0xe8600000, 0xff700000,
>>     "strd%c\t%12-15r, %8-11r, [%16-19r], #%23`-%0-7W%L"},
>>   {ARM_FEATURE_CORE_LOW (ARM_EXT_V6T2),
>>     0xe8700000, 0xff700000,
>>     "ldrd%c\t%12-15r, %8-11r, [%16-19r], #%23`-%0-7W%L”},
>> That’s in binutils-gdb/opcodes/arm-dis.c.
>> All that code was added at the same time in 2015.
>> 0xff50 is going to allow more matches than 0xff70.
>> And given that the thing we care about is matching the opcode,
>> then 0xff50 is safer.
>> Before I go off and start looking at instruction decodings,
>> Luis - where did you get 0xff70 from?
> 
> From the manual, a concatenation of 7 bits from the opcode plus 4 bits of op0 and 1 bit for Load/Store.
> 
> Mask 0xfe00 filters the opcode (1110100).
> Mask 0x160 filters op0 (bits 0, 1 and 3 - 1011).
> Mask 0x10 filters the load/store bit (load is 1).
> 
> We want to match the insn pattern 0xe950, which is ldrd (immediate).
> 
> Compared to 0xff70, with mask 0xff50 we would ignore op<0>. Ignoring op<0> would allow matching ldrd (immediate, pre-indexed) as well.

...and we don’t want to match those.

Ok, I’m happy with the patch then :)


Alan.