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

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

[Simon Marchi]

We get this error in arm-tdep.c when building with clang 11:

  CXX    arm-tdep.o
/home/smarchi/src/binutils-gdb/gdb/arm-tdep.c:934:29: error: bitwise comparison always evaluates to false [-Werror,-Wtautological-bitwise-compare]
          else if ((insn & 0xffb0) == 0xe950    /* ldrd Rt, Rt2,
                   ~~~~~~~~~~~~~~~~^~~~~~~~~

Indeed, this can never evaluate to true.  I tried to look at what the condition
should be, but it's a bit too deep into ARM instruction encoding for me.  I am
pretty sure that the corresponding section in the ARM Architecture Reference
Manual [1] is F3.1.15, if somebody wants to take a stab at it.

Simon

[1] https://static.docs.arm.com/ddi0487/fb/DDI0487F_b_armv8_arm.pdf

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

[Luis Machado]

On 5/15/20 5:32 PM, Simon Marchi via Gdb-patches wrote:
> We get this error in arm-tdep.c when building with clang 11:
> 
>    CXX    arm-tdep.o
> /home/smarchi/src/binutils-gdb/gdb/arm-tdep.c:934:29: error: bitwise comparison always evaluates to false [-Werror,-Wtautological-bitwise-compare]
>            else if ((insn & 0xffb0) == 0xe950    /* ldrd Rt, Rt2,
>                     ~~~~~~~~~~~~~~~~^~~~~~~~~
> 
> Indeed, this can never evaluate to true.  I tried to look at what the condition
> should be, but it's a bit too deep into ARM instruction encoding for me.  I am
> pretty sure that the corresponding section in the ARM Architecture Reference
> Manual [1] is F3.1.15, if somebody wants to take a stab at it.
> 
> Simon
> 
> [1] https://static.docs.arm.com/ddi0487/fb/DDI0487F_b_armv8_arm.pdf
> 

Thanks for reporting this. I'll look into it. It may be a typo of some kind.

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

[Simon Marchi]

On 2020-05-15 4:41 p.m., Luis Machado wrote:
> On 5/15/20 5:32 PM, Simon Marchi via Gdb-patches wrote:
>> We get this error in arm-tdep.c when building with clang 11:
>>
>>    CXX    arm-tdep.o
>> /home/smarchi/src/binutils-gdb/gdb/arm-tdep.c:934:29: error: bitwise comparison always evaluates to false [-Werror,-Wtautological-bitwise-compare]
>>            else if ((insn & 0xffb0) == 0xe950    /* ldrd Rt, Rt2,
>>                     ~~~~~~~~~~~~~~~~^~~~~~~~~
>>
>> Indeed, this can never evaluate to true.  I tried to look at what the condition
>> should be, but it's a bit too deep into ARM instruction encoding for me.  I am
>> pretty sure that the corresponding section in the ARM Architecture Reference
>> Manual [1] is F3.1.15, if somebody wants to take a stab at it.
>>
>> Simon
>>
>> [1] https://static.docs.arm.com/ddi0487/fb/DDI0487F_b_armv8_arm.pdf
>>
> 
> Thanks for reporting this. I'll look into it. It may be a typo of some kind.

Yes, I suppose, it's just not a trivial typo to fix!

Simon

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

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

Hi,

On 5/15/20 5:32 PM, Simon Marchi via Gdb-patches wrote:
> We get this error in arm-tdep.c when building with clang 11:
> 
>    CXX    arm-tdep.o
> /home/smarchi/src/binutils-gdb/gdb/arm-tdep.c:934:29: error: bitwise comparison always evaluates to false [-Werror,-Wtautological-bitwise-compare]
>            else if ((insn & 0xffb0) == 0xe950    /* ldrd Rt, Rt2,
>                     ~~~~~~~~~~~~~~~~^~~~~~~~~
> 
> Indeed, this can never evaluate to true.  I tried to look at what the condition
> should be, but it's a bit too deep into ARM instruction encoding for me.  I am
> pretty sure that the corresponding section in the ARM Architecture Reference
> Manual [1] is F3.1.15, if somebody wants to take a stab at it.
> 
> Simon
> 
> [1] https://static.docs.arm.com/ddi0487/fb/DDI0487F_b_armv8_arm.pdf
> 

The following patch should fix clang's error and make the code do the 
right thing.

[-- Attachment #2: 0001-ARM-Fix-PR-26000-logical-bitwise-error-prologue-anal.patch --]
[-- Type: text/x-patch, Size: 1328 bytes --]

From dbaf5e7b1f35c7e97bf23aac846509b33eea491b Mon Sep 17 00:00:00 2001
From: Luis Machado <luis.machado@linaro.org>
Date: Fri, 15 May 2020 23:06:52 -0300
Subject: [PATCH] [ARM] Fix PR 26000, logical bitwise error / prologue analyzer

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.

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.

gdb/ChangeLog:

2020-05-25  Luis Machado  <luis.machado@linaro.org>

	PR tdep/26000
	* arm-tdep.c (thumb_analyze_prologue): Fix instruction matching
	for ldrd (immediate).
---
 gdb/arm-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 40bffbb2dd..9a73b85899 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -931,7 +931,7 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 	       parameters from memory.  */
 	    ;
 
-	  else if ((insn & 0xffb0) == 0xe950	/* ldrd Rt, Rt2,
+	  else if ((insn & 0xff70) == 0xe950    /* ldrd Rt, Rt2,
 						   [Rn, #+/-imm] */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
 	    /* Similarly ignore dual loads from the stack.  */
-- 
2.17.1

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

[Simon Marchi]

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?

>
> 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

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

[Luis Machado]

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
> 

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

[Alan Hayward]

> 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?


Alan.

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

[Luis Machado]

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.

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

[Alan Hayward]

> 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.

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

[Luis Machado]

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.

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

[Simon Marchi]

On 2020-05-27 8:43 a.m., Luis Machado wrote:
> Thanks. Pushed now.

Thanks for fixing this!