This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix the x87 FP register printout by “info float”.
- From: Pedro Alves <palves at redhat dot com>
- To: Vladimir Kargov <kargov at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 19 Apr 2013 15:24:52 +0100
- Subject: Re: [PATCH] Fix the x87 FP register printout by “info float”.
- References: <CAHLdKi=qPX-x7Eb0gwqTaOsWp0gK0=3WQFLmc6_HBKGUZ7RzfQ at mail dot gmail dot com> <516FC7B3 dot 4050502 at redhat dot com> <CAHLdKinQnJHBayS1va0aqrspfcwfO4iVJwNxuaof5VsHjObtuQ at mail dot gmail dot com>
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 .*"