[PATCH] Fix the x87 FP register printout by “info float”.

[PATCH] Fix the x87 FP register printout by “info float”.

[Vladimir Kargov]

Hello,

Please find attached a patch that fixes the x87 FP register printout
when issuing the “info float” command.

Consider the following simple program:

.globl  _start
.text
_start:
      fldt    val
.data
      val: .byte 0x00,0x00,0x45,0x07,0x11,0x19,0x22,0xe9,0xfe,0xbf

With the current gdb revision(7.6.50.20130417-cvs) on my x86-64 target
machine after the moment the fldt command has been executed the
register st(0) looks like this, according to the “info regs” output
(TOP=7):

R7: Valid   0xffffffbffffffffeffffffe922191107450000 -0.910676542908976927

which is clearly wrong (just count its length). The problem is due to
the printf() statement (see patch) printing a promoted integer value
of a char argument "raw[i]", and, since char is signed on my machine,
the erroneous “ffffff” are printed for the first three bytes which
turn out to be "negative". The fix adds the char length modifier that
forces fprintf to treat the value as a char instead of int. After the
fix the value will be printed correctly:

R7: Valid   0xbffee922191107450000 -0.910676542908976927

Another way to fix this would be to replace the type in the definition
of variable "raw" from "char" to "gdb_char" which is currently defined
as "unsigned char", but I couldn't find any signs in the code that
this typedef wouldn't be changed in the future to something else.

2013-04-18  Vladimir Kargov <kargov@gmail.com>
	* i387-tdep.c (i387_print_float_info): Add the "hh" length modifier
	to the format string of fprintf_filtered().

Index: gdb/i387-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i387-tdep.c,v
retrieving revision 1.76
diff -u -p -r1.76 i387-tdep.c
--- gdb/i387-tdep.c     1 Jan 2013 06:32:45 -0000       1.76
+++ gdb/i387-tdep.c     18 Apr 2013 00:23:27 -0000
@@ -302,7 +302,7 @@ i387_print_float_info (struct gdbarch *g

              fputs_filtered ("0x", file);
              for (i = 9; i >= 0; i--)
-               fprintf_filtered (file, "%02x", raw[i]);
+               fprintf_filtered (file, "%02hhx", raw[i]);

              if (tag != -1 && tag != 3)
                print_i387_ext (gdbarch, raw, file);

Best regards,
--
Vladimir

Re: [PATCH] Fix the x87 FP register printout by “info float”.

[Pedro Alves]

On 04/18/2013 01:51 AM, Vladimir Kargov wrote:

> Another way to fix this would be to replace the type in the definition
> of variable "raw" from "char" to "gdb_char" which is currently defined
> as "unsigned char", 

You must have meant gdb_byte.  Actually, this (not checked in yet)
series does that:

 http://sourceware.org/ml/gdb-patches/2013-04/msg00327.html

Specifically this patch:

 http://sourceware.org/ml/gdb-patches/2013-04/msg00328.html

> but I couldn't find any signs in the code that
> this typedef wouldn't be changed in the future to something else.

Could you write a test case?  That'd prevent a regression here
going unnoticed.

-- 
Pedro Alves

Re: [PATCH] Fix the x87 FP register printout by “info float”.

[Vladimir Kargov]

On 18 April 2013 14:15, Pedro Alves <palves@redhat.com> wrote:
> On 04/18/2013 01:51 AM, Vladimir Kargov wrote:
>
>> Another way to fix this would be to replace the type in the definition
>> of variable "raw" from "char" to "gdb_char" which is currently defined
>> as "unsigned char",
>
> You must have meant gdb_byte.
Yes, sure.

> Actually, this (not checked in yet) series does that:
Oh, I did not notice it. It should solve the problem, although I think
it is still reasonable to add the length modifier to fprintf_filtered
in case the definition of gdb_byte ever changes.

>> but I couldn't find any signs in the code that
>> this typedef wouldn't be changed in the future to something else.
>
> Could you write a test case?  That'd prevent a regression here
> going unnoticed.
A testcase for the proper FP register printout? I'm attaching one.
(may have missed some formatting/guidelines...)

2013-04-19  Vladimir Kargov <kargov@gmail.com>
        * gdb.arch/i386-float.S: New file.
        * gdb.arch/i386-float.exp: New file.

diff --git a/gdb/testsuite/gdb.arch/i386-float.S
b/gdb/testsuite/gdb.arch/i386-float.S
new file mode 100644
index 0000000..f7c87f8
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-float.S
@@ -0,0 +1,24 @@
+/* Copyright 2009-2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.  */
+
+	.text
+	.globl main
+main:
+	fninit
+	fldt    val
+.data
+	val: .byte 0x00,0x00,0x45,0x07,0x11,0x19,0x22,0xe9,0xfe,0xbf
diff --git a/gdb/testsuite/gdb.arch/i386-float.exp
b/gdb/testsuite/gdb.arch/i386-float.exp
new file mode 100644
index 0000000..a50f608
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-float.exp
@@ -0,0 +1,51 @@
+# Copyright (C) 2009-2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# This file is part of the gdb testsuite.
+
+# Test the x87 floating point information printout.
+
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    verbose "Skipping i386 test for proper x87 FP support."
+    return
+}
+
+set testfile "i386-float"
+set srcfile ${testfile}.S
+set binfile ${objdir}/${subdir}/${testfile}
+
+# some targets have leading underscores on assembly symbols.
+set additional_flags [gdb_target_symbol_prefix_flags]
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
executable [list debug $additional_flags]] != "" } {
+    untested i386-float.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+send_gdb "stepi\n"
+gdb_test "info float" ".*R7: Empty   0x00000000000000000000\r\n.*"
+send_gdb "stepi\n"
+gdb_test "info float" ".*=>R7: Valid   0xbffee922191107450000 .*"


-- 
Vladimir

Re: [PATCH] Fix the x87 FP register printout by “info float”.

[Pedro Alves]

On 04/19/2013 04:22 AM, Vladimir Kargov wrote:
> On 18 April 2013 14:15, Pedro Alves <palves@redhat.com> wrote:

>> > Actually, this (not checked in yet) series does that:
> Oh, I did not notice it. It should solve the problem, although I think
> it is still reasonable to add the length modifier to fprintf_filtered

...

> in case the definition of gdb_byte ever changes.

I think hell will freeze over before that happening.  ;-)

I believe printing a negative value with an unsigned format
specifier is actually undefined.  Then, %hhx is not portable
enough for GDB.  E.g., Microsoft's C runtime doesn't support it, so
MinGW, when using the default printf implementation, doesn't either.
(Before anyone asks, no, I don't think that alone justifies pulling
in printf from gnulib.)
It's just simpler to pass down an unsigned value and be done with it.
If gdb_byte ever changes, or something else unexpectedly, then the
new test case will catch it.

>> > Could you write a test case?  That'd prevent a regression here
>> > going unnoticed.
> A testcase for the proper FP register printout? I'm attaching one.
> (may have missed some formatting/guidelines...)

Many thanks.  That helps a lot.  I've fixed a few things, and
I'm applying the patch further below.  I'll just point out
what needed fixing, FYI.

> 2013-04-19  Vladimir Kargov <kargov@gmail.com>

Double space after your name.

>         * gdb.arch/i386-float.S: New file.
>         * gdb.arch/i386-float.exp: New file.
> 
> diff --git a/gdb/testsuite/gdb.arch/i386-float.S
> b/gdb/testsuite/gdb.arch/i386-float.S
> new file mode 100644
> index 0000000..f7c87f8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-float.S
> @@ -0,0 +1,24 @@

> +	.text
> +	.globl main
> +main:

This should make use of the SYMBOL_PREFIX, setup by:

  +# some targets have leading underscores on assembly symbols.
  +set additional_flags [gdb_target_symbol_prefix_flags]


> +	fninit
> +	fldt    val

Might as well add a ret here.


> +set testfile "i386-float"
> +set srcfile ${testfile}.S
> +set binfile ${objdir}/${subdir}/${testfile}

We use standard_testfile nowadays for this (paves the
way for moving binaries elsewhere, like into subdirs
for finer testsuite parallelization).

> +
> +# some targets have leading underscores on assembly symbols.
> +set additional_flags [gdb_target_symbol_prefix_flags]
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}"
> executable [list debug $additional_flags]] != "" } {
> +    untested i386-float.exp
> +    return -1
> +}
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}
> +
> +if ![runto_main] then {
> +    fail "Can't run to main"
> +    return 0
> +}

prepare_for_testing does all this.

> +
> +send_gdb "stepi\n"

We always need to expect the prompt...

> +gdb_test "info float" ".*R7: Empty   0x00000000000000000000\r\n.*"

...otherwise the prompt regex matching inside this gdb_test
might match the prompt gdb prints after stepi, and FAIL.  This
class of bug is a frequent cause of racy tests.
The fix is to use gdb_test instead of send_gdb.

> +send_gdb "stepi\n"
> +gdb_test "info float" ".*=>R7: Valid   0xbffee922191107450000 .*"

Although not a regression, the fix is super safe so I put
it in the 7.6 branch as well.

Thanks a lot.

------
Subject: Fix the x87 FP register printout when issuing the “info float” command.

Consider the following simple program:

.globl  _start
.text
_start:
      fldt    val
.data
      val: .byte 0x00,0x00,0x45,0x07,0x11,0x19,0x22,0xe9,0xfe,0xbf

With current GDB on x86-64 GNU/Linux hosts, after the moment the fldt
command has been executed the register st(0) looks like this,
according to the “info regs” output (TOP=7):

  R7: Valid   0xffffffbffffffffeffffffe922191107450000 -0.910676542908976927

which is clearly wrong (just count its length).  The problem is due to
the printf statement (see patch) printing a promoted integer value of
a char argument "raw[i]", and, since char is signed on x86-64
GNU/Linux, the erroneous “ffffff” are printed for the first three
bytes which turn out to be "negative".  The fix is to use gdb_byte
instead which is unsigned (and is the type of value_contents(), the
type to be used for raw target bytes anyway).  After the fix the value
will be printed correctly:

  R7: Valid   0xbffee922191107450000 -0.910676542908976927

gdb/
2013-04-19  Vladimir Kargov <kargov@gmail.com>
	    Pedro Alves  <palves@redhat.com>

	* i387-tdep.c (i387_print_float_info): Use gdb_byte for pointer to
	value contents.

gdb/testsuite/
2013-04-19  Vladimir Kargov  <kargov@gmail.com>
	    Pedro Alves  <palves@redhat.com>

	* gdb.arch/i386-float.S: New file.
	* gdb.arch/i386-float.exp: New file.
---
 gdb/i387-tdep.c                       |    2 +-
 gdb/testsuite/gdb.arch/i386-float.S   |   34 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/i386-float.exp |   43 +++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.arch/i386-float.S
 create mode 100644 gdb/testsuite/gdb.arch/i386-float.exp

diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index 65bb103..48a00c3 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -298,7 +298,7 @@ i387_print_float_info (struct gdbarch *gdbarch, struct ui_file *file,

 	  if (value_entirely_available (regval))
 	    {
-	      const char *raw = value_contents (regval);
+	      const gdb_byte *raw = value_contents (regval);

 	      fputs_filtered ("0x", file);
 	      for (i = 9; i >= 0; i--)
diff --git a/gdb/testsuite/gdb.arch/i386-float.S b/gdb/testsuite/gdb.arch/i386-float.S
new file mode 100644
index 0000000..c4f6391
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-float.S
@@ -0,0 +1,34 @@
+/* Copyright 2009-2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.  */
+
+#define CONCAT1(a, b) CONCAT2(a, b)
+#define CONCAT2(a, b) a ## b
+
+#ifdef SYMBOL_PREFIX
+# define SYMBOL(str)     CONCAT1(SYMBOL_PREFIX, str)
+#else
+# define SYMBOL(str)     str
+#endif
+
+	.text
+	.globl SYMBOL(main)
+SYMBOL(main):
+	fninit
+	fldt    val
+	ret
+.data
+	val: .byte 0x00,0x00,0x45,0x07,0x11,0x19,0x22,0xe9,0xfe,0xbf
diff --git a/gdb/testsuite/gdb.arch/i386-float.exp b/gdb/testsuite/gdb.arch/i386-float.exp
new file mode 100644
index 0000000..c417532
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-float.exp
@@ -0,0 +1,43 @@
+# Copyright (C) 2009-2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+# This file is part of the gdb testsuite.
+
+# Test the x87 floating point information printout.
+
+if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } then {
+    verbose "Skipping i386 tests for x87 floating point support."
+    return
+}
+
+standard_testfile .S
+
+# some targets have leading underscores on assembly symbols.
+set additional_flags [gdb_target_symbol_prefix_flags]
+
+if { [prepare_for_testing break.exp $testfile $srcfile [list debug $additional_flags]] } {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "stepi" ".*fldt.*" "first stepi"
+gdb_test "info float" ".*R7: Empty   0x00000000000000000000\r\n.*"
+gdb_test "stepi" ".*ret.*" "second stepi"
+gdb_test "info float" ".*=>R7: Valid   0xbffee922191107450000 .*"

Re: [PATCH] Fix the x87 FP register printout by “info float”.

[Vladimir Kargov]

On 19 April 2013 18:24, Pedro Alves <palves@redhat.com> wrote:
> I believe printing a negative value with an unsigned format
> specifier is actually undefined.
I've just checked C99 and yes, it appears to be undefined after all (I
actually thought that the C standard mandated a uniform representation
of negative integer numbers which it didn't!), so my patch indeed
exploited compiler and hardware dependent behaviour.

> I'm applying the patch
> I'll just point out what needed fixing, FYI.
Thanks, and thank you for the review!

-- 
Vladimir