Re: [PATCH][gdb/testsuite] Fix printing of 0x10000000000000000

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

> Hi,
>
> When printing large literal constants we have an unexpected result for
> 0x10000000000000000:
> ...
> $ gdb -q -batch \
>     -ex "p /x  0xffffffffffffffff" \
>     -ex "p /x 0x10000000000000000" \
>     -ex "p /x 0x10000000000000001"
> $1 = 0xffffffffffffffff
> $2 = 0x0
> Numeric constant too large.
> ...
>
> Fix this by fixing overflow detecting in parse_number, such that we have:
> ...
> $1 = 0xffffffffffffffff
> Numeric constant too large.
> Numeric constant too large.
> ...
>
> For rust, I've used as overflow string: "Integer literal is too large", based
> on what I found at
> <rust-lang/rust>/src/test/ui/parser/int-literal-too-large-span.rs
> but perhaps someone has a better idea.
>
> Don't fix this for modula-2, it makes more sense to do this as part of a
> parse_number rewrite to use ULONGEST, skip for now.
>
> Tested on x86_64-linux, with a build with --enable-targets=all.
>
> Any comments?

This all looks good to me, just one minor comment...

>
> Thanks,
> - Tom
>
> [gdb/testsuite] Fix printing of 0x10000000000000000
>
> ---
>  gdb/c-exp.y                             | 18 ++++++------------
>  gdb/f-exp.y                             | 15 +++++----------
>  gdb/go-exp.y                            | 16 +++++-----------
>  gdb/p-exp.y                             | 16 +++++-----------
>  gdb/rust-parse.c                        |  5 ++++-
>  gdb/testsuite/gdb.base/parse_number.exp | 17 +++++++++++++++++
>  6 files changed, 42 insertions(+), 45 deletions(-)
>
> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
> index 72f8dd32d93..84bc0568e8f 100644
> --- a/gdb/c-exp.y
> +++ b/gdb/c-exp.y
> @@ -2095,18 +2095,12 @@ parse_number (struct parser_state *par_state,
>        if (i >= base)
>  	return ERROR;		/* Invalid digit in this base */
>  
> -      /* Portably test for overflow (only works for nonzero values, so make
> -	 a second check for zero).  FIXME: Can't we just make n and prevn
> -	 unsigned and avoid this?  */
> -      if (c != 'l' && c != 'u' && c != 'i' && (prevn >= n) && n != 0)
> -	unsigned_p = 1;		/* Try something unsigned */
> -
> -      /* Portably test for unsigned overflow.
> -	 FIXME: This check is wrong; for example it doesn't find overflow
> -	 on 0x123456789 when LONGEST is 32 bits.  */
> -      if (c != 'l' && c != 'u' && c != 'i' && n != 0)
> -	{	
> -	  if (unsigned_p && prevn >= n)
> +      if (c != 'l' && c != 'u' && c != 'i')
> +	{
> +	  /* Test for overflow.  */
> +	  if (prevn == 0 && n == 0)
> +	    ;
> +	  else if (prevn >= n)
>  	    error (_("Numeric constant too large."));
>  	}
>        prevn = n;
> diff --git a/gdb/f-exp.y b/gdb/f-exp.y
> index 90cc2c65c7b..124dfcb1b8f 100644
> --- a/gdb/f-exp.y
> +++ b/gdb/f-exp.y
> @@ -1076,16 +1076,11 @@ parse_number (struct parser_state *par_state,
>  	  n *= base;
>  	  n += i;
>  	}
> -      /* Portably test for overflow (only works for nonzero values, so make
> -	 a second check for zero).  */
> -      if ((prevn >= n) && n != 0)
> -	unsigned_p=1;		/* Try something unsigned */
> -      /* If range checking enabled, portably test for unsigned overflow.  */
> -      if (RANGE_CHECK && n != 0)
> -	{
> -	  if ((unsigned_p && prevn >= n))
> -	    range_error (_("Overflow on numeric constant."));
> -	}
> +      /* Test for overflow.  */
> +      if (prevn == 0 && n == 0)
> +	;
> +      else if (RANGE_CHECK && prevn >= n)
> +	range_error (_("Overflow on numeric constant."));
>        prevn = n;
>      }
>    
> diff --git a/gdb/go-exp.y b/gdb/go-exp.y
> index 1b9b6ea7e90..8c9a558262f 100644
> --- a/gdb/go-exp.y
> +++ b/gdb/go-exp.y
> @@ -777,18 +777,12 @@ parse_number (struct parser_state *par_state,
>        if (i >= base)
>  	return ERROR;		/* Invalid digit in this base.  */
>  
> -      /* Portably test for overflow (only works for nonzero values, so make
> -	 a second check for zero).  FIXME: Can't we just make n and prevn
> -	 unsigned and avoid this?  */
> -      if (c != 'l' && c != 'u' && (prevn >= n) && n != 0)
> -	unsigned_p = 1;		/* Try something unsigned.  */
> -
> -      /* Portably test for unsigned overflow.
> -	 FIXME: This check is wrong; for example it doesn't find overflow
> -	 on 0x123456789 when LONGEST is 32 bits.  */
> -      if (c != 'l' && c != 'u' && n != 0)
> +      if (c != 'l' && c != 'u')
>  	{
> -	  if ((unsigned_p && prevn >= n))
> +	  /* Test for overflow.  */
> +	  if (n == 0 && prevn == 0)
> +	    ;
> +	  else if (prevn >= n)
>  	    error (_("Numeric constant too large."));
>  	}
>        prevn = n;
> diff --git a/gdb/p-exp.y b/gdb/p-exp.y
> index 7c88df65e69..7e119963759 100644
> --- a/gdb/p-exp.y
> +++ b/gdb/p-exp.y
> @@ -919,18 +919,12 @@ parse_number (struct parser_state *par_state,
>        if (i >= base)
>  	return ERROR;		/* Invalid digit in this base.  */
>  
> -      /* Portably test for overflow (only works for nonzero values, so make
> -	 a second check for zero).  FIXME: Can't we just make n and prevn
> -	 unsigned and avoid this?  */
> -      if (c != 'l' && c != 'u' && (prevn >= n) && n != 0)
> -	unsigned_p = 1;		/* Try something unsigned.  */
> -
> -      /* Portably test for unsigned overflow.
> -	 FIXME: This check is wrong; for example it doesn't find overflow
> -	 on 0x123456789 when LONGEST is 32 bits.  */
> -      if (c != 'l' && c != 'u' && n != 0)
> +      if (c != 'l' && c != 'u')
>  	{
> -	  if (unsigned_p && prevn >= n)
> +	  /* Test for overflow.  */
> +	  if (prevn == 0 && n == 0)
> +	    ;
> +	  else if (prevn >= n)
>  	    error (_("Numeric constant too large."));
>  	}
>        prevn = n;
> diff --git a/gdb/rust-parse.c b/gdb/rust-parse.c
> index 7d7d882872c..836f49108f8 100644
> --- a/gdb/rust-parse.c
> +++ b/gdb/rust-parse.c
> @@ -1024,7 +1024,10 @@ rust_parser::lex_number ()
>  	    }
>  	}
>  
> -      value = strtoulst (number.c_str () + offset, NULL, radix);
> +      const char *trailer;
> +      value = strtoulst (number.c_str () + offset, &trailer, radix);
> +      if (*trailer != '\0')
> +	error ("Integer literal is too large");
>        if (implicit_i32 && value >= ((uint64_t) 1) << 31)
>  	type = get_type ("i64");
>  
> diff --git a/gdb/testsuite/gdb.base/parse_number.exp b/gdb/testsuite/gdb.base/parse_number.exp
> index 444f5d0534b..60526f5612a 100644
> --- a/gdb/testsuite/gdb.base/parse_number.exp
> +++ b/gdb/testsuite/gdb.base/parse_number.exp
> @@ -68,6 +68,16 @@ proc test_parse_numbers {arch} {
>  
>  	gdb_test_no_output "set language $lang"
>  
> +	if { $lang == "modula-2" || $lang == "fortran" } {
> +	    set re_overflow "Overflow on numeric constant\\."
> +	} elseif { $lang == "ada" } {
> +	    set re_overflow "Integer literal out of range"
> +	} elseif { $lang == "rust" } {
> +	    set re_overflow "Integer literal is too large"
> +	} else {
> +	    set re_overflow "Numeric constant too large\\."
> +	}
> +
>  	set val "0xffffffffffffffff"
>  	set val [hex_for_lang $lang $val]
>  	if {$lang == "fortran"} {
> @@ -98,6 +108,13 @@ proc test_parse_numbers {arch} {
>  		gdb_test "ptype $val" " = $8B_type"
>  	    }
>  	}
> +
> +	if { $lang == "modula-2" } {
> +	    continue
> +	}

Maybe you're working on modula-2 right now, in which case, ignore this.
But, if you're not, then it might be better to raise a bug for the
module-2 issues - even if it's just "parse_number needs a rewrite", and
then use setup_xfail here instead of continue.

But otherwise, this all looks OK to me.

Thanks,
Andrew



> +
> +	set val [hex_for_lang $lang "10000000000000000"]
> +	gdb_test "p/x $val" $re_overflow
>      }
>  }
>