Re: [RFA 1/2] Fix two regressions in scalar printing

[Pedro Alves <palves@redhat.com>]

On 07/13/2017 01:33 PM, Tom Tromey wrote:
> PR gdb/21675 points out a few regressions in scalar printing.
> 
> One type of regression is due to not carrying over the old handling of
> floating point printing -- where a format like "/x" causes a floating
> point number to first be cast to integer.  While this behavior does not
> seem very useful to me, apparently at least one person is testing for
> it, and we did agree in the earlier thread to preserve this.  So, this
> patch extends this behavior to the 'd' and 'u' formats.

I'm open to changing the behavior, if we can come up with something
that is useful and generally makes more sense.

The manual does say, for "/x":

  Regard the bits of the value as an integer, and print the integer in
  hexadecimal.

and for "/f":

  Regard the bits of the value as a floating point number and print
  using typical floating point syntax.

which seems to suggest the intention was to reinterpret the
variable's raw contents as integer.  However, I suspect this was written
with the "x" command in mind though, where you're reinterpreting raw
memory disregarding anything about the types of the objects that may
exist on the examined memory (and also where you can also request a
specific width).  A question like "how to print a float type
object in hexadecimal format" just doesn't really leave much
doubt for the "x" command.  There, it's clearly raw bits
interpretation you want, so you can do things like:

 float global_f = 3.14f;

 (gdb) x /4xb &global_f
 0x601038 <global_f>:    0xc3    0xf5    0x48    0x40
 (gdb) x /fw &global_f
 0x601038 <global_f>:    3.1400001

"print" is different because it's working with objects with
size and type, and can print aggregates/structs with subobjects
of different types, even.

Playing devil's advocate, I could see some justification for the
converting behavior if you look at /x /d /u as behaving like a
printf %x/%d/%u format strings with an implicit cast.  That view
doesn't work with the current implementation of "%f" though, which
reinterprets with print, instead of converting, though with such
a view, that would be seen as a bug...  What a mess.  :-/

Meanwhile, it's good to avoid behavior changes until we have a
clear plan.

> The other regression is a longstanding bug in print_octal_chars: one of
> the constants was wrong.  This patch fixes the constant and adds static
> asserts to help catch this sort of error.
> 


> @@ -427,11 +429,14 @@ print_scalar_formatted (const gdb_byte *valaddr, struct type *type,
>      case 'o':
>        print_octal_chars (stream, valaddr, len, byte_order);
>        break;
> +    case 'd':
>      case 'u':
> -      print_decimal_chars (stream, valaddr, len, false, byte_order);
> +      {
> +	bool is_signed = options->format != 'u' || !TYPE_UNSIGNED (type);

I think you meant "&&" instead of "||".

(
Maybe checking against 'd' would be clearer:
	bool is_signed = options->format == 'd' && !TYPE_UNSIGNED (type);
or even separate case switches.
)

As is, you'll get this:

 (gdb) p /u -1
 $1 = -1

instead of current master's output:

 (gdb) p /u -1
 $1 = 4294967295

which doesn't look right.

Also, I'm also not quite sure about the TYPE_UNSIGNED check.
The manual says:

 @item d
 Print as integer in signed decimal.

 @item u
 Print as integer in unsigned decimal.

And I see this with a gdb from before the recent print_scalar_formatted
changes:

 (gdb) p /d (unsigned long long) -1
 $1 = -1

while we see this with either current master, or your patch:

 (gdb) p /d (unsigned long long) -1
 $1 = 18446744073709551615

which also doesn't look right to me.

And here:

 (gdb) p /d (unsigned) -1
 $2 = 4294967295

I'd expect "-1", but we don't get it with any gdb version (before
original print_scalar_formatted changes, or current master, or
your current patch), which also looks like a bug to me.

I think this would all be fixed by simply having separate
'u'/'d' cases with fixed signness:

    case 'u':
      print_decimal_chars (stream, valaddr, len, false, byte_order);
      break;
    case 'd':
      print_decimal_chars (stream, valaddr, len, true, byte_order);
      break;

> +	print_decimal_chars (stream, valaddr, len, is_signed, byte_order);

Thanks,
Pedro Alves