[RFA,1/2] Fix two regressions in scalar printing
Commit Message
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(-)
Comments
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. */
>
@@ -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
@@ -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),
@@ -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.
@@ -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
@@ -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. */