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

[Luis Machado <luis.machado@linaro.org>]

On 5/26/20 1:10 PM, Alan Hayward wrote:
> 
> 
>> 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.
> 

Thanks. Pushed now.