Re: [PATCH][gdb/testsuite] Fix gdb.ada/literals.exp with aarch64

[Joel Brobecker via Gdb-patches <gdb-patches@sourceware.org>]

Hi Tom,

On Thu, Jul 28, 2022 at 04:21:35PM +0200, Tom de Vries wrote:
> On 7/28/22 15:10, Joel Brobecker wrote:
> > > > Thanks for the patch.
> > > > 
> > > > My question is whether it actually makes sense that -1 be
> > > > a valid output for the print command above. I tried the addition
> > > > of ...
> > > > 
> > > >       | /* Note: Interprets ULLONG_MAX as -1.  */
> > > >       | yylval.typed_val.type = type_long_long (par_state);
> > > > 
> > > > ... to a patch of yours:
> > > > 
> > > >       | commit ac3afe36d73c84096685fece885d70b28bc9629f
> > > >       | Author: Tom de Vries <tdevries@suse.de>
> > > >       | Date:   Sat Jun 4 13:17:33 2022 +0200
> > > >       | Subject: [gdb/ada] Fix literal truncation
> > > >       |
> > > >       | Make sure we error out on overflow instead of truncating in all cases.
> > > >       |
> > > >       | Tested on x86_64-linux, with a build with --enable-targets=all.
> > > > 
> > > > Do you remember why we do this? Intuitively, you'd expect that GDB
> > > > would behave the same regardless of whether it selects type "long"
> > > > or "long long" for its processing, as long as both types are 64-bit.
> > > > 
> > > 
> > > I have no idea.  AFAICT it's been in gdb since the "Add base ada language
> > > files" commit from 2002 that we avoid "unsigned long long".
> > > 
> > > I've taken care to preserve the behaviour in the commit you refer to (and
> > > this patch), since I don't have the knowledge to decide that things should
> > > be different.
> > > 
> > > > With the above, we can take this patch as an intermediate remedy,
> > > > but I think we might need to dig deeper into why we use a signed
> > > > type in the case of long long but not long, particularly when both
> > > > types are the same size.
> > > > 
> > > > WDYT?
> > > > 
> > > 
> > > I'd be happy to write a patch to change the behaviour of gdb.  But somebody
> > > knowledgeable in Ada needs to specify what that needs to be.
> > 
> > We're really outside of the Ada language. Would you mind checking
> > what the C language does? I think it would make sense to do the same,
> > here. E.g., on x86_64-linux, I get:
> > 
> >      (gdb) p 0xffffffffffffffff
> >      $1 = 18446744073709551615
> > 
> 
> Ack, that's what the C language does.
> 
> This patch changes gdb to print 18446744073709551615 for ada as well.
> 
> So, pick your favorite patch ;)

TomT kindly tested your patch on a number of platforms for us,
and everything looked good on those. I propose we go with your
second patch which changes the behavior to match what we do
in C (I looked at the patch, and it looks good to me, so approved).

Thank you!

> 
> Thanks,
> - Tom

> [gdb/testsuite] Fix gdb.ada/literals.exp with aarch64
> 
> On aarch64-linux, I run into:
> ...
> (gdb) print 16#ffffffffffffffff#^M
> $7 = 18446744073709551615^M
> (gdb) FAIL: gdb.ada/literals.exp: print 16#ffffffffffffffff#
> ...
> while on x86_64-linux instead, I get:
> ...
> (gdb) print 16#ffffffffffffffff#^M
> $7 = -1^M
> (gdb) PASS: gdb.ada/literals.exp: print 16#ffffffffffffffff#
> ...
> 
> We can easily reproduce this on x86_64-linux using:
> ...
> $ gdb -q -batch -ex "set lang ada" -ex "set arch i386" \
>   -ex "print 16#ffffffffffffffff#"
> $1 = -1
> $ gdb -q -batch -ex "set lang ada" -ex "set arch aarch64" \
>   -ex "print 16#ffffffffffffffff#"
> $1 = 18446744073709551615
> ...
> 
> With i386, we have:
> ...
> (gdb) p int_bits
> $3 = 32
> (gdb) p long_bits
> $4 = 32
> (gdb) p long_long_bits
> $5 = 64
> ...
> and so in processInt we hit the fits-in-unsigned-long-long case where we use
> as type long long:
> ...
>       /* Note: Interprets ULLONG_MAX as -1.  */
>       yylval.typed_val.type = type_long_long (par_state);
> ...
> 
> With aarch64, we have instead:
> ...
> (gdb) p int_bits
> $1 = 32
> (gdb) p long_bits
> $2 = 64
> (gdb) p long_long_bits
> $3 = 64
> ...
> and so in processInt we hit the fits-in-unsigned-long case where we use
> as type unsigned long:
> ...
>       yylval.typed_val.type
>         = builtin_type (par_state->gdbarch ())->builtin_unsigned_long;
> ...
> 
> It's not clear why for ada we're using long long for the
> fits-in-unsigned-long-long case.
> 
> Fix this by using unsigned long long for the fits-in-unsigned-long-long case,
> meaning the new reference output is 18446744073709551615 instead of -1.
> 
> Tested on x86_64-linux.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29416
> 
> ---
>  gdb/ada-lex.l                      | 4 ++--
>  gdb/testsuite/gdb.ada/literals.exp | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/ada-lex.l b/gdb/ada-lex.l
> index 002eb811e41..ed88d502e9f 100644
> --- a/gdb/ada-lex.l
> +++ b/gdb/ada-lex.l
> @@ -498,8 +498,8 @@ processInt (struct parser_state *par_state, const char *base0,
>      yylval.typed_val.type = type_long_long (par_state);
>    else if (fits_in_type (1, value, long_long_bits, false))
>      {
> -      /* Note: Interprets ULLONG_MAX as -1.  */
> -      yylval.typed_val.type = type_long_long (par_state);
> +      yylval.typed_val.type
> +	= builtin_type (par_state->gdbarch ())->builtin_unsigned_long_long;
>        /* See unsigned long case above.  */
>        if (value & LONGEST_SIGN)
>  	yylval.typed_val.val =
> diff --git a/gdb/testsuite/gdb.ada/literals.exp b/gdb/testsuite/gdb.ada/literals.exp
> index a6ac89b540f..6badc857292 100644
> --- a/gdb/testsuite/gdb.ada/literals.exp
> +++ b/gdb/testsuite/gdb.ada/literals.exp
> @@ -36,4 +36,4 @@ gdb_test "print 16#f#e1" " = 240"
>  gdb_test "print 16#1#e10" " = 1099511627776"
>  
>  gdb_test "print/x 16#7fffffffffffffff#" " = 0x7fffffffffffffff"
> -gdb_test "print 16#ffffffffffffffff#" " = -1"
> +gdb_test "print 16#ffffffffffffffff#" " = 18446744073709551615"


-- 
Joel