Re: [PATCH] Change how "print/x" displays floating-point value

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

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

> Currently, "print/x" will display a floating-point value by first
> casting it to an integer type.  This yields weird results like:
>
>     (gdb) print/x 1.5
>     $1 = 0x1
>
> This has confused users multiple times -- see PR gdb/16242, where
> there are several dups.  I've also seen some confusion from this
> internally at AdaCore.
>
> The manual says:
>
>     'x'
> 	 Regard the bits of the value as an integer, and print the integer
> 	 in hexadecimal.
>
> ... which seems more useful.  So, perhaps what happened is that this
> was incorrectly implemented (or maybe correctly implemented and then
> regressed, as there don't seem to be any tests).
>
> This patch fixes the bug.
>
> There was a previous discussion where we agreed to preserve the old
> behavior:
>
>     https://sourceware.org/legacy-ml/gdb-patches/2017-06/msg00314.html
>
> However, I think it makes more sense to follow the manual.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=16242
> ---
>  gdb/printcmd.c                            | 9 ++-------
>  gdb/testsuite/gdb.base/printcmds.exp      | 7 +++++--
>  gdb/testsuite/gdb.base/return-nodebug.exp | 7 ++++++-
>  3 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 6f9be820b0c..30de1927d39 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -426,19 +426,14 @@ print_scalar_formatted (const gdb_byte *valaddr, struct type *type,
>        len = newlen;
>      }
>  
> -  /* Historically gdb has printed floats by first casting them to a
> -     long, and then printing the long.  PR cli/16242 suggests changing
> -     this to using C-style hex float format.
> -
> -     Biased range types and sub-word scalar types must also be handled
> +  /* Biased range types and sub-word scalar types must be handled
>       here; the value is correctly computed by unpack_long.  */
>    gdb::byte_vector converted_bytes;
>    /* Some cases below will unpack the value again.  In the biased
>       range case, we want to avoid this, so we store the unpacked value
>       here for possible use later.  */
>    gdb::optional<LONGEST> val_long;
> -  if (((type->code () == TYPE_CODE_FLT
> -	|| is_fixed_point_type (type))
> +  if ((is_fixed_point_type (type)
>         && (options->format == 'o'
>  	   || options->format == 'x'
>  	   || options->format == 't'
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index 37632985a07..2d0aafd6d8b 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -158,8 +158,11 @@ proc test_float_rejected {} {
>  # Regression test for PR gdb/21675
>  proc test_radices {} {
>      gdb_test "print/o 16777211" " = 077777773"
> -    gdb_test "print/d 1.5" " = 1"
> -    gdb_test "print/u 1.5" " = 1"
> +
> +    # See PR gdb/16242 for this.
> +    gdb_test "print/d 1.5f" " = 1069547520"
> +    gdb_test "print/u 1.5f" " = 1069547520"
> +    gdb_test "print/x 1.5f" " = 0x3fc00000"

I noticed the reference to PR gdb/21675 at the top of this hunk, which
was an interesting read.

Would it be possible to increase the testing to cover printing doubles
as well as floats, and also to test /o and /t format?

FWIW, I think this change makes sense.  Though I wonder if it is worth
having a NEWS entry as this might look like a bug to someone who is used
to the old behaviour?

Also, maybe I'm just overly paranoid, but I wonder if it is worth really
stressing the point in the manual (@node Output Format) that the value
is reinterpreted, rather than cast.  Especially for /d /u /o, etc the
manual is rather vague on how a float will be handled, e.g.:

  @item d
  Print as integer in signed decimal.

Which doesn't really tell me much...

Thanks,
Andrew




>  
>      gdb_test "print/u (char) -1" " = 255"
>      gdb_test "print/d (unsigned char) -1" " = -1"
> diff --git a/gdb/testsuite/gdb.base/return-nodebug.exp b/gdb/testsuite/gdb.base/return-nodebug.exp
> index 6fd41bee884..1cce09d2fc4 100644
> --- a/gdb/testsuite/gdb.base/return-nodebug.exp
> +++ b/gdb/testsuite/gdb.base/return-nodebug.exp
> @@ -38,7 +38,12 @@ proc do_test {type} {
>  		"advance to marker"
>  
>  	    # And if it returned the full width of the result.
> -	    gdb_test "print /d t" " = -1" "full width of the returned result"
> +	    if {$type == "float" || $type == "double"} {
> +		set flag ""
> +	    } else {
> +		set flag "/d"
> +	    }
> +	    gdb_test "print $flag t" " = -1" "full width of the returned result"
>  	}
>      }
>  }
> -- 
> 2.31.1