Re: -Werror,-Wtautological-overlap-compare error in h8300-tdep.c

[Yoshinori Sato <ysato@users.sourceforge.jp>]

On Wed, 20 May 2020 02:35:12 +0900,
Simon Marchi wrote:
> 
> On 2020-05-18 8:51 a.m., Yoshinori Sato wrote:
> > Yes.
> > register number 6 (er6) is frame pointer in h8300 ABI.
> > Since we are analyzing the code that accesses the stack frame,
> > we will only look at the frame pointer.
> 
> Ah ok, thanks for the explanation.  Here's what I pushed.  Note that I also
> changed the read_memory_integer to read_memory_unsigned_integer.
>

Sorry too late reply.
It looks good. Thanks.

> 
> From 1d6ce4d31257e1ea49b3a1b257055bf8f7ff16af Mon Sep 17 00:00:00 2001
> From: Yoshinori Sato <ysato@users.sourceforge.jp>
> Date: Tue, 19 May 2020 13:33:08 -0400
> Subject: [PATCH] gdb: fix -Wtautological-overlap-compare error in h8300-tdep.c
> 
> Compiling with clang 11 gives us:
> 
>       CXX    h8300-tdep.o
>     /home/smarchi/src/binutils-gdb/gdb/h8300-tdep.c:225:21: error: overlapping comparisons always evaluate to false [-Werror,-Wtautological-overlap-compare]
>                   if (disp < 0 && disp > 0xffffff)
>                       ~~~~~~~~~^~~~~~~~~~~~~~~~~~
>     /home/smarchi/src/binutils-gdb/gdb/h8300-tdep.c:203:17: error: overlapping comparisons always evaluate to false [-Werror,-Wtautological-overlap-compare]
>               if (disp < 0 && disp > 0xffffff)
>                   ~~~~~~~~~^~~~~~~~~~~~~~~~~~
>     /home/smarchi/src/binutils-gdb/gdb/h8300-tdep.c:184:17: error: overlapping comparisons always evaluate to false [-Werror,-Wtautological-overlap-compare]
>               if (disp < 0 && disp > 0xffffff)
>                   ~~~~~~~~~^~~~~~~~~~~~~~~~~~
> 
> Indeed, disp (of type LONGEST) can't be less than 0 and greater than
> 0xffffff.
> 
> Fix it by changing the way we check if disp is negative.  Check the sign
> bit of disp, which is a 24-bit number.
> 
> gdb/ChangeLog:
> 
> 	* h8300-tdep.c (h8300_is_argument_spill): Change how we check
> 	whether disp is negative.
> ---
>  gdb/ChangeLog    |  5 +++++
>  gdb/h8300-tdep.c | 13 +++++++------
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index f086b5cf908c..c4393182dd95 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2020-05-19  Yoshinori Sato  <ysato@users.sourceforge.jp>
> +
> +	* h8300-tdep.c (h8300_is_argument_spill): Change how we check
> +	whether disp is negative.
> +
>  2020-05-19  Simon Marchi  <simon.marchi@efficios.com>
> 
>  	* symfile.h (struct symfile_segment_data)
> diff --git a/gdb/h8300-tdep.c b/gdb/h8300-tdep.c
> index 3c2f502a124d..b569a23f2631 100644
> --- a/gdb/h8300-tdep.c
> +++ b/gdb/h8300-tdep.c
> @@ -178,10 +178,10 @@ h8300_is_argument_spill (struct gdbarch *gdbarch, CORE_ADDR pc)
>        if (IS_MOVB_Rn24_SP (read_memory_unsigned_integer (pc + 2,
>  							 2, byte_order)))
>  	{
> -	  LONGEST disp = read_memory_integer (pc + 4, 4, byte_order);
> +	  ULONGEST disp = read_memory_unsigned_integer (pc + 4, 4, byte_order);
> 
>  	  /* ... and d:24 is negative.  */
> -	  if (disp < 0 && disp > 0xffffff)
> +	  if ((disp & 0x00800000) != 0)
>  	    return 8;
>  	}
>      }
> @@ -197,10 +197,10 @@ h8300_is_argument_spill (struct gdbarch *gdbarch, CORE_ADDR pc)
>        if (IS_MOVW_Rn24_SP (read_memory_unsigned_integer (pc + 2,
>  							 2, byte_order)))
>  	{
> -	  LONGEST disp = read_memory_integer (pc + 4, 4, byte_order);
> +	  ULONGEST disp = read_memory_unsigned_integer (pc + 4, 4, byte_order);
> 
>  	  /* ... and d:24 is negative.  */
> -	  if (disp < 0 && disp > 0xffffff)
> +	  if ((disp & 0x00800000) != 0)
>  	    return 8;
>  	}
>      }
> @@ -219,10 +219,11 @@ h8300_is_argument_spill (struct gdbarch *gdbarch, CORE_ADDR pc)
>  	{
>  	  if (IS_MOVL_Rn24_SP (read_memory_integer (pc + 4, 2, byte_order)))
>  	    {
> -	      LONGEST disp = read_memory_integer (pc + 6, 4, byte_order);
> +	      ULONGEST disp = read_memory_unsigned_integer (pc + 6, 4,
> +							    byte_order);
> 
>  	      /* ... and d:24 is negative.  */
> -	      if (disp < 0 && disp > 0xffffff)
> +	      if ((disp & 0x00800000) != 0)
>  		return 10;
>  	    }
>  	}
> -- 
> 2.26.2
> 

-- 
Yosinori Sato