From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27676 invoked by alias); 19 Apr 2013 14:24:58 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 27665 invoked by uid 89); 19 Apr 2013 14:24:58 -0000 X-Spam-SWARE-Status: No, score=-7.4 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 19 Apr 2013 14:24:56 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3JEOsTk009168 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 19 Apr 2013 10:24:54 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3JEOq1k025423; Fri, 19 Apr 2013 10:24:53 -0400 Message-ID: <517153B4.1060605@redhat.com> Date: Fri, 19 Apr 2013 14:33:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Vladimir Kargov CC: gdb-patches@sourceware.org Subject: =?windows-1252?Q?Re=3A_=5BPATCH=5D_Fix_the_x87_FP_re?= =?windows-1252?Q?gister_printout_by_=93info_float=94=2E?= References: <516FC7B3.4050502@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-SW-Source: 2013-04/txt/msg00582.txt.bz2 On 04/19/2013 04:22 AM, Vladimir Kargov wrote: > On 18 April 2013 14:15, Pedro Alves 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 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 Pedro Alves * i387-tdep.c (i387_print_float_info): Use gdb_byte for pointer to value contents. gdb/testsuite/ 2013-04-19 Vladimir Kargov Pedro Alves * 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 . + + 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 . + + +# 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 .*"