[PATCH] gdb: fix -Wtautological-overlap-compare warning in mips-linux-tdep.c

[PATCH] gdb: fix -Wtautological-overlap-compare warning in mips-linux-tdep.c

[Simon Marchi]

When building with clang 11, I get:

  CXX    mips-linux-tdep.o
/home/smarchi/src/binutils-gdb/gdb/mips-linux-tdep.c:643:30: error: overlapping comparisons always evaluate to true [-Werror,-Wtautological-overlap-compare]
      if (insn != 0x03e07821 || insn != 0x03e07825)
          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/mips-linux-tdep.c:636:30: error: overlapping comparisons always evaluate to true [-Werror,-Wtautological-overlap-compare]
      if (insn != 0x03e0782d || insn != 0x03e07825)
          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~

Indeed, given two different values, `insn` will always be different to
one of them, and these conditions always be true.  I suppose that the
original intent was "if insn is equal to this instruction or equal to
that instruction".  Therefore the `!=` should be changed to `==`.

gdb/ChangeLog:

	* mips-linux-tdep.c (mips_linux_in_dynsym_stub): Fix condition.
---
 gdb/mips-linux-tdep.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index aa7b8d11f3fb..ae067c19dfa7 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -633,16 +633,14 @@ mips_linux_in_dynsym_stub (CORE_ADDR pc)
   if (n64)
     {
       /* 'daddu t7,ra' or 'or t7, ra, zero'*/
-      if (insn != 0x03e0782d || insn != 0x03e07825)
+      if (insn == 0x03e0782d || insn == 0x03e07825)
 	return 0;
-
     }
   else
     {
       /* 'addu t7,ra'  or 'or t7, ra, zero'*/
-      if (insn != 0x03e07821 || insn != 0x03e07825)
+      if (insn == 0x03e07821 || insn == 0x03e07825)
 	return 0;
-
     }
 
   insn = extract_unsigned_integer (p + 8, 4, byte_order);
-- 
2.26.2

Re: [PATCH] gdb: fix -Wtautological-overlap-compare warning in mips-linux-tdep.c

[Andreas Schwab]

On Mai 15 2020, Simon Marchi via Gdb-patches wrote:

> Indeed, given two different values, `insn` will always be different to
> one of them, and these conditions always be true.  I suppose that the
> original intent was "if insn is equal to this instruction or equal to
> that instruction".  Therefore the `!=` should be changed to `==`.

I don't think that makes sense.  Rather || should be fixed to &&.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] gdb: fix -Wtautological-overlap-compare warning in mips-linux-tdep.c

[Maciej W. Rozycki]

On Fri, 15 May 2020, Simon Marchi wrote:

> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
> index aa7b8d11f3fb..ae067c19dfa7 100644
> --- a/gdb/mips-linux-tdep.c
> +++ b/gdb/mips-linux-tdep.c
> @@ -633,16 +633,14 @@ mips_linux_in_dynsym_stub (CORE_ADDR pc)
>    if (n64)
>      {
>        /* 'daddu t7,ra' or 'or t7, ra, zero'*/
> -      if (insn != 0x03e0782d || insn != 0x03e07825)
> +      if (insn == 0x03e0782d || insn == 0x03e07825)

 Nope, this ought to be:

	if (insn != 0x03e0782d && insn != 0x03e07825)

>  	return 0;
> -
>      }
>    else
>      {
>        /* 'addu t7,ra'  or 'or t7, ra, zero'*/
> -      if (insn != 0x03e07821 || insn != 0x03e07825)
> +      if (insn == 0x03e07821 || insn == 0x03e07825)

 Likewise:

	if (insn != 0x03e07821 && insn != 0x03e07825)

 Thanks for looking into it (and for cc-ing me)!

  Maciej

Re: [PATCH] gdb: fix -Wtautological-overlap-compare warning in mips-linux-tdep.c

[Simon Marchi]

On 2020-05-16 9:29 a.m., Maciej W. Rozycki wrote:
> On Fri, 15 May 2020, Simon Marchi wrote:
> 
>> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
>> index aa7b8d11f3fb..ae067c19dfa7 100644
>> --- a/gdb/mips-linux-tdep.c
>> +++ b/gdb/mips-linux-tdep.c
>> @@ -633,16 +633,14 @@ mips_linux_in_dynsym_stub (CORE_ADDR pc)
>>    if (n64)
>>      {
>>        /* 'daddu t7,ra' or 'or t7, ra, zero'*/
>> -      if (insn != 0x03e0782d || insn != 0x03e07825)
>> +      if (insn == 0x03e0782d || insn == 0x03e07825)
> 
>  Nope, this ought to be:
> 
> 	if (insn != 0x03e0782d && insn != 0x03e07825)
> 
>>  	return 0;
>> -
>>      }
>>    else
>>      {
>>        /* 'addu t7,ra'  or 'or t7, ra, zero'*/
>> -      if (insn != 0x03e07821 || insn != 0x03e07825)
>> +      if (insn == 0x03e07821 || insn == 0x03e07825)
> 
>  Likewise:
> 
> 	if (insn != 0x03e07821 && insn != 0x03e07825)
> 
>  Thanks for looking into it (and for cc-ing me)!
> 
>   Maciej
> 

Ah so I misinterpreted the intent.  Thanks to you and Andreas for pointing
it out!

I modified the patch and pushed it as below.

Simon


From 59f7bd8d2b855162db6784c9724ead9e2377f32c Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Sat, 16 May 2020 11:21:41 -0400
Subject: [PATCH] gdb: fix -Wtautological-overlap-compare warning in
 mips-linux-tdep.c

When building with clang 11, I get:

  CXX    mips-linux-tdep.o
/home/smarchi/src/binutils-gdb/gdb/mips-linux-tdep.c:643:30: error: overlapping comparisons always evaluate to true [-Werror,-Wtautological-overlap-compare]
      if (insn != 0x03e07821 || insn != 0x03e07825)
          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
/home/smarchi/src/binutils-gdb/gdb/mips-linux-tdep.c:636:30: error: overlapping comparisons always evaluate to true [-Werror,-Wtautological-overlap-compare]
      if (insn != 0x03e0782d || insn != 0x03e07825)
          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~

Indeed, given two different values, `insn` will always be different to
one of them, and these conditions always be true.

This code is meant to return if `insn` isn't one of these two values, so
the `||` should be replaced with `&&`.

gdb/ChangeLog:

	* mips-linux-tdep.c (mips_linux_in_dynsym_stub): Fix condition.
---
 gdb/ChangeLog         | 4 ++++
 gdb/mips-linux-tdep.c | 6 ++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1c4dc5c94c2a..8d6901efe67c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2020-05-16  Simon Marchi  <simon.marchi@efficios.com>
+
+	* mips-linux-tdep.c (mips_linux_in_dynsym_stub): Fix condition.
+
 2020-05-16  Pedro Alves  <palves@redhat.com>

 	* ia64-linux-nat.c
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index aa7b8d11f3fb..3ffd53db9ead 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -633,16 +633,14 @@ mips_linux_in_dynsym_stub (CORE_ADDR pc)
   if (n64)
     {
       /* 'daddu t7,ra' or 'or t7, ra, zero'*/
-      if (insn != 0x03e0782d || insn != 0x03e07825)
+      if (insn != 0x03e0782d && insn != 0x03e07825)
 	return 0;
-
     }
   else
     {
       /* 'addu t7,ra'  or 'or t7, ra, zero'*/
-      if (insn != 0x03e07821 || insn != 0x03e07825)
+      if (insn != 0x03e07821 && insn != 0x03e07825)
 	return 0;
-
     }

   insn = extract_unsigned_integer (p + 8, 4, byte_order);
-- 
2.26.2