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

Message ID 20200516020151.34919-1-simon.marchi@efficios.com
State New
Headers show
Series
  • gdb: fix -Wtautological-overlap-compare warning in mips-linux-tdep.c
Related show

Commit Message

Shahab Vahedi via Gdb-patches May 16, 2020, 2:01 a.m.
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(-)

-- 
2.26.2

Comments

Andreas Schwab May 16, 2020, 6:42 a.m. | #1
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."
Maciej W. Rozycki May 16, 2020, 1:29 p.m. | #2
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
Simon Marchi May 16, 2020, 3:23 p.m. | #3
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

Patch

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