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

  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.

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.

2017-07-13  Tom Tromey  <tom@tromey.com>

	PR gdb/21675
	* valprint.c (LOW_ZERO): Change value to 034.
	(print_octal_chars): Add static_asserts for octal constants.
	* printcmd.c (print_scalar_formatted): Add 'd' and 'u' to special
	cases for float types.

2017-07-13  Tom Tromey  <tom@tromey.com>

	PR gdb/21675:
	* gdb.base/printcmds.exp (test_radices): New function.
---
 gdb/ChangeLog                        |  8 ++++++++
 gdb/printcmd.c                       | 11 ++++++++---
 gdb/testsuite/ChangeLog              |  5 +++++
 gdb/testsuite/gdb.base/printcmds.exp |  8 ++++++++
 gdb/valprint.c                       |  8 +++++++-
 5 files changed, 36 insertions(+), 4 deletions(-)
  

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
  

On 13 July 2017 at 13:33, Tom Tromey <tom@tromey.com> 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.

While Eclipse CDT is testing for this format, speaking as an Eclipse
CDT committer, the Eclipse CDT community should be happy to change the
behaviour if the GDB community is. The tests purpose in Eclipse CDT is
to ensure that there is proper communication between CDT and GDB over
MI. Personally I would prefer changes like this to be on major version
number changes, but I am not intimately familiar with what GDB version
numbers are intending to convey to users.

> 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.

In filing gdb/21675 I seem to have combined two issues that I thought
were the same cause (due to having both been triggered by the same GDB
change). This part of the bug is the problematic one.
  

> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 14 Jul 2017 15:56:27 +0100
> 
> 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 I agree: we should produce what the format says, not what the
cast says.
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

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

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

I'm testing this.  Thanks for the feedback.

One early note is that this changes the expected output for this test:

FAIL: gdb.dwarf2/var-access.exp: verify re-initialized s2

Now it says:

print/d s2
$9 = {a = -65, b = 73, c = -25, d = 123}

but the test wants:

gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
    "verify re-initialized s2"

Tom
  

On 07/14/2017 05:49 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I think this would all be fixed by simply having separate
> Pedro> 'u'/'d' cases with fixed signness:
> 
> Pedro>     case 'u':
> Pedro>       print_decimal_chars (stream, valaddr, len, false, byte_order);
> Pedro>       break;
> Pedro>     case 'd':
> Pedro>       print_decimal_chars (stream, valaddr, len, true, byte_order);
> Pedro>       break;
> 
> I'm testing this.  Thanks for the feedback.
> 
> One early note is that this changes the expected output for this test:
> 
> FAIL: gdb.dwarf2/var-access.exp: verify re-initialized s2
> 
> Now it says:
> 
> print/d s2
> $9 = {a = -65, b = 73, c = -25, d = 123}
> 
> but the test wants:
> 
> gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
>     "verify re-initialized s2"
> 

Yeah, that seems OK to me GDB-output-wise.  "You get what you ask for".

Now, the test is for "# Byte-aligned register- and memory pieces.", and
is treating the bytes as raw bytes, even though the fields are
defined as "char".

We see just above that a test setting the fields using values over 0x7f:

 gdb_test_no_output "set var s2 = {191, 73, 231, 123}" \
     "re-initialize s2"
 gdb_test "print/d s2"  " = \\{a = 191, b = 73, c = 231, d = 123\\}" \
     "verify re-initialized s2"

The /d was surely as a convenience to avoid printing the bytes
in character format.  I'd just change it to /u.  Same for the related
tests just above.

Thanks,
Pedro Alves
  

Hi Tom,

As raised in [1], this patch creates a regression on arm targets in:

gdb.base/sizeof.exp: check valueof "'\377'"

I've taken the liberty of creating a bugzilla ticket [2] where I added some more 
info of things I've tried.

[1] https://sourceware.org/ml/gdb-testers/2017-q3/msg01944.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=22010

Please let me know if I can be of any help in fixing that bug.

Best regards,

Thomas


On 13/07/17 13:33, 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.
> 
> 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.
> 
> 2017-07-13  Tom Tromey  <tom@tromey.com>
> 
> 	PR gdb/21675
> 	* valprint.c (LOW_ZERO): Change value to 034.
> 	(print_octal_chars): Add static_asserts for octal constants.
> 	* printcmd.c (print_scalar_formatted): Add 'd' and 'u' to special
> 	cases for float types.
> 
> 2017-07-13  Tom Tromey  <tom@tromey.com>
> 
> 	PR gdb/21675:
> 	* gdb.base/printcmds.exp (test_radices): New function.
> ---
>   gdb/ChangeLog                        |  8 ++++++++
>   gdb/printcmd.c                       | 11 ++++++++---
>   gdb/testsuite/ChangeLog              |  5 +++++
>   gdb/testsuite/gdb.base/printcmds.exp |  8 ++++++++
>   gdb/valprint.c                       |  8 +++++++-
>   5 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 012b3e4..5ac5bab 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,11 @@
> +2017-07-13  Tom Tromey  <tom@tromey.com>
> +
> +	PR gdb/21675
> +	* valprint.c (LOW_ZERO): Change value to 034.
> +	(print_octal_chars): Add static_asserts for octal constants.
> +	* printcmd.c (print_scalar_formatted): Add 'd' and 'u' to special
> +	cases for float types.
> +
>   2017-07-11  John Baldwin  <jhb@FreeBSD.org>
>   
>   	* amd64-bsd-nat.c (amd64bsd_fetch_inferior_registers): Use
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index a8cc052..cd615ec 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -413,7 +413,9 @@ print_scalar_formatted (const gdb_byte *valaddr, struct type *type,
>         && (options->format == 'o'
>   	  || options->format == 'x'
>   	  || options->format == 't'
> -	  || options->format == 'z'))
> +	  || options->format == 'z'
> +	  || options->format == 'd'
> +	  || options->format == 'u'))
>       {
>         LONGEST val_long = unpack_long (type, valaddr);
>         converted_float_bytes.resize (TYPE_LENGTH (type));
> @@ -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);
> +	print_decimal_chars (stream, valaddr, len, is_signed, byte_order);
> +      }
>         break;
>       case 0:
> -    case 'd':
>         if (TYPE_CODE (type) != TYPE_CODE_FLT)
>   	{
>   	  print_decimal_chars (stream, valaddr, len, !TYPE_UNSIGNED (type),
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index aa3dee3..0c8481b 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2017-07-13  Tom Tromey  <tom@tromey.com>
> +
> +	PR gdb/21675:
> +	* gdb.base/printcmds.exp (test_radices): New function.
> +
>   2017-07-11  Iain Buclaw  <ibuclaw@gdcproject.org>
>   
>   	* gdb.dlang/demangle.exp: Update for demangling changes.
> diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
> index 323ca73..03275c3 100644
> --- a/gdb/testsuite/gdb.base/printcmds.exp
> +++ b/gdb/testsuite/gdb.base/printcmds.exp
> @@ -155,6 +155,13 @@ proc test_float_rejected {} {
>       test_print_reject "p 1.1ll"
>   }
>   
> +# 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\[^.\]"
> +}
> +
>   proc test_print_all_chars {} {
>       global gdb_prompt
>   
> @@ -981,3 +988,4 @@ test_printf
>   test_printf_with_dfp
>   test_print_symbol
>   test_repeat_bytes
> +test_radices
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index 1667882..9e216cf 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -1593,15 +1593,21 @@ print_octal_chars (struct ui_file *stream, const gdb_byte *valaddr,
>      */
>   #define BITS_IN_OCTAL 3
>   #define HIGH_ZERO     0340
> -#define LOW_ZERO      0016
> +#define LOW_ZERO      0034
>   #define CARRY_ZERO    0003
> +  static_assert (HIGH_ZERO + LOW_ZERO + CARRY_ZERO == 0xff,
> +		 "cycle zero constants are wrong");
>   #define HIGH_ONE      0200
>   #define MID_ONE       0160
>   #define LOW_ONE       0016
>   #define CARRY_ONE     0001
> +  static_assert (HIGH_ONE + MID_ONE + LOW_ONE + CARRY_ONE == 0xff,
> +		 "cycle one constants are wrong");
>   #define HIGH_TWO      0300
>   #define MID_TWO       0070
>   #define LOW_TWO       0007
> +  static_assert (HIGH_TWO + MID_TWO + LOW_TWO == 0xff,
> +		 "cycle two constants are wrong");
>   
>     /* For 32 we start in cycle 2, with two bits and one bit carry;
>        for 64 in cycle in cycle 1, with one bit and a two bit carry.  */
>