Message ID | 20180103182048.8495-1-tom@tromey.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 3016 invoked by alias); 3 Jan 2018 18:20:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 2447 invoked by uid 89); 3 Jan 2018 18:20:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=H*MI:tom X-HELO: gateway23.websitewelcome.com Received: from gateway23.websitewelcome.com (HELO gateway23.websitewelcome.com) (192.185.49.219) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 Jan 2018 18:20:53 +0000 Received: from cm10.websitewelcome.com (cm10.websitewelcome.com [100.42.49.4]) by gateway23.websitewelcome.com (Postfix) with ESMTP id 9FA9712862 for <gdb-patches@sourceware.org>; Wed, 3 Jan 2018 12:20:51 -0600 (CST) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id WnelexXXFcGlpWnele6hJf; Wed, 03 Jan 2018 12:20:51 -0600 Received: from 71-218-90-63.hlrn.qwest.net ([71.218.90.63]:58072 helo=pokyo.Home) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89) (envelope-from <tom@tromey.com>) id 1eWnel-0012aQ-Be; Wed, 03 Jan 2018 12:20:51 -0600 From: Tom Tromey <tom@tromey.com> To: gdb-patches@sourceware.org Cc: Tom Tromey <tom@tromey.com> Subject: [RFA] Fix scm-ports.exp regression Date: Wed, 3 Jan 2018 11:20:48 -0700 Message-Id: <20180103182048.8495-1-tom@tromey.com> X-BWhitelist: no X-Source-L: No X-Exim-ID: 1eWnel-0012aQ-Be X-Source-Sender: 71-218-90-63.hlrn.qwest.net (pokyo.Home) [71.218.90.63]:58072 X-Source-Auth: tom+tromey.com X-Email-Count: 1 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes |
Commit Message
Tom Tromey
Jan. 3, 2018, 6:20 p.m. UTC
In https://sourceware.org/ml/gdb-patches/2017-12/msg00215.html, Jan pointed out that the scalar printing patches caused a regression in scm-ports.exp on x86. I think the simplest fix is to use "print/u" rather than "print/d" to get the value of sp_reg in the test case. Tested on x86-64 Fedora 26 using an ordinary build and also a -m32 build. 2018-01-03 Tom Tromey <tom@tromey.com> * gdb.guile/scm-ports.exp (test_mem_port_rw): Use get_valueof to compute sp_reg. --- gdb/testsuite/ChangeLog | 5 +++++ gdb/testsuite/gdb.guile/scm-ports.exp | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
Comments
On 01/03/2018 06:20 PM, Tom Tromey wrote: > In https://sourceware.org/ml/gdb-patches/2017-12/msg00215.html, Jan > pointed out that the scalar printing patches caused a regression in > scm-ports.exp on x86. > > I think the simplest fix is to use "print/u" rather than "print/d" to > get the value of sp_reg in the test case. Can you expand a bit on this rationale, please? There's: (parse-and-eval \"*(char*) \$sp\") in the context of the diff. Is that related? I ask because that "char" in there would look like something that could print as signed or unsigned depending on target. It'll probably be obvious with a bit more info. Thanks, Pedro Alves > > Tested on x86-64 Fedora 26 using an ordinary build and also a -m32 > build. > > 2018-01-03 Tom Tromey <tom@tromey.com> > > * gdb.guile/scm-ports.exp (test_mem_port_rw): Use get_valueof to > compute sp_reg. > --- > gdb/testsuite/ChangeLog | 5 +++++ > gdb/testsuite/gdb.guile/scm-ports.exp | 2 +- > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog > index 500dbddf1c..e3903cca6b 100644 > --- a/gdb/testsuite/ChangeLog > +++ b/gdb/testsuite/ChangeLog > @@ -1,3 +1,8 @@ > +2018-01-03 Tom Tromey <tom@tromey.com> > + > + * gdb.guile/scm-ports.exp (test_mem_port_rw): Use get_valueof to > + compute sp_reg. > + > 2018-01-03 Xavier Roirand <roirand@adacore.com> > > * gdb.ada/excep_handle.exp: New testcase. > diff --git a/gdb/testsuite/gdb.guile/scm-ports.exp b/gdb/testsuite/gdb.guile/scm-ports.exp > index 48af5e30e1..04170ef4b8 100644 > --- a/gdb/testsuite/gdb.guile/scm-ports.exp > +++ b/gdb/testsuite/gdb.guile/scm-ports.exp > @@ -104,7 +104,7 @@ proc test_mem_port_rw { kind } { > "get sp reg" > # Note: Only use $sp_reg for gdb_test result matching, don't use it in > # gdb commands. Otherwise transcript.N becomes unusable. > - set sp_reg [get_integer_valueof "\$sp" 0] > + set sp_reg [get_valueof /u "\$sp" 0] > gdb_test_no_output "guile (define byte-at-sp (parse-and-eval \"*(char*) \$sp\"))" \ > "save current value at sp" > # Pass the result of parse-and-eval through value-fetch-lazy!, >
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: >> I think the simplest fix is to use "print/u" rather than "print/d" to >> get the value of sp_reg in the test case. Pedro> Can you expand a bit on this rationale, please? Pedro> There's: Pedro> (parse-and-eval \"*(char*) \$sp\") Pedro> in the context of the diff. Is that related? I ask because Pedro> that "char" in there would look like something that could print Pedro> as signed or unsigned depending on target. I don't think that is related. That expression has a dereference. What happens is that on x86, this: set sp_reg [get_integer_valueof "\$sp" 0] ... ends up setting sp_reg to a negative value, because get_integer_valueof uses "print/d": print /d $sp $1 = -11496 Then later the test suite does: gdb_test "guile (print (seek rw-mem-port (value->integer sp-reg) SEEK_SET))" \ "= $sp_reg" \ "seek to \$sp" ... expecting this value to be identical to the saved $sp_reg value. However it gets: guile (print (seek rw-mem-port (value->integer sp-reg) SEEK_SET)) = 4294955800 "print" is just a wrapper for guile's format: gdb_test_no_output "guile (define (print x) (format #t \"= ~A\" x) (newline))" The seek function returns a scm_t_off, so I would think that this sort of printing is handled by guile, not by gdb. IIRC what happened is that "print/d" slightly changed in some cases during the scalar printing work, and what we're seeing is the result. Tom
Hi Tromey, Sorry for the delay. On 01/09/2018 06:26 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > >>> I think the simplest fix is to use "print/u" rather than "print/d" to >>> get the value of sp_reg in the test case. > > Pedro> Can you expand a bit on this rationale, please? > > Pedro> There's: > Pedro> (parse-and-eval \"*(char*) \$sp\") > Pedro> in the context of the diff. Is that related? I ask because > Pedro> that "char" in there would look like something that could print > Pedro> as signed or unsigned depending on target. > > I don't think that is related. That expression has a dereference. > > What happens is that on x86, this: > > set sp_reg [get_integer_valueof "\$sp" 0] > > ... ends up setting sp_reg to a negative value, because > get_integer_valueof uses "print/d": > > print /d $sp > $1 = -11496 > > Then later the test suite does: > > gdb_test "guile (print (seek rw-mem-port (value->integer sp-reg) SEEK_SET))" \ > "= $sp_reg" \ > "seek to \$sp" > > ... expecting this value to be identical to the saved $sp_reg value. > However it gets: > > guile (print (seek rw-mem-port (value->integer sp-reg) SEEK_SET)) > = 4294955800 > > "print" is just a wrapper for guile's format: > > gdb_test_no_output "guile (define (print x) (format #t \"= ~A\" x) (newline))" > > The seek function returns a scm_t_off, so I would think that this sort > of printing is handled by guile, not by gdb. I see. So seemingly this is printing a scm_t_off, which seems to be a signed 64-bit integer: /usr/include/guile/2.0/libguile/scmconfig-32.h:82:typedef int64_t scm_t_int64; /usr/include/guile/2.0/libguile/scmconfig-32.h:119:typedef scm_t_int64 scm_t_off; /usr/include/guile/2.0/libguile/scmconfig-64.h:82:typedef int64_t scm_t_int64; /usr/include/guile/2.0/libguile/scmconfig-64.h:119:typedef scm_t_int64 scm_t_off; while $sp is 32-bit, and we're extracting it as a 32-bit signed integer (into $sp_reg). Here: (seek rw-mem-port (value->integer sp-reg) SEEK_SET) "sp-reg" is a pointer, and value->integer takes us to gdbscm_value_to_integer [I think], which converts the pointer to an unsigned integer, AFAICT, and then probably that gets cast/converted to scm_t_off when passed to guile's "seek", somewhere. And then 'seek' returns the same offset out, as an scm_t_off, and then guile's 'format' prints that. So pedantically, doing: "print (scm_t_off) $sp" (or really "print (int64_t) $sp", or even "print (long long) $sp"...) to extract $sp_reg would be a little more to the point, I guess. But it looks like on 64-bit archs, the API can't access memory addresses with the high bit set anyway (?) (not sure how to get those; maybe debugging some bare metal/kernel code), so the difference doesn't really matter much in practice. The patch is fine with me as is. I just wish the commit log were a little clearer with details such as the above. > IIRC what happened is that "print/d" slightly changed in some cases > during the scalar printing work, and what we're seeing is the result. Yes, before the rework, "/d" would still print integers as unsigned in some cases. Now it always prints them as signed, as if it you wrote something like this: (gdb) print (std::make_signed<decltype(EXPR)>::type) EXPR instead of: (gdb) print /d EXPR with the difference that /d affects display only, unlike a cast which affects the actual value recorded in the value history. Thanks, Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> The patch is fine with me as is. I just wish the commit
Pedro> log were a little clearer with details such as the above.
I added my explanation to the commit message.
I'm going to push this to the 8.0 branch and trunk.
Tom
On Monday, January 15 2018, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > > Pedro> The patch is fine with me as is. I just wish the commit > Pedro> log were a little clearer with details such as the above. > > I added my explanation to the commit message. > I'm going to push this to the 8.0 branch and trunk. Hey Tom, Perhaps you meant to push this to the 8.1 branch? Thanks,
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
Sergio> Perhaps you meant to push this to the 8.1 branch?
Yeah, sorry about that.
I will do it now.
I can revert on the 8.0 branch if someone wants.
Tom
> Sergio> Perhaps you meant to push this to the 8.1 branch? > > Yeah, sorry about that. > I will do it now. > I can revert on the 8.0 branch if someone wants. I am guessing the patch is correct on the 8.0 branch, even if there wasn't a fail before. I think it's fine to leave it in.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 500dbddf1c..e3903cca6b 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2018-01-03 Tom Tromey <tom@tromey.com> + + * gdb.guile/scm-ports.exp (test_mem_port_rw): Use get_valueof to + compute sp_reg. + 2018-01-03 Xavier Roirand <roirand@adacore.com> * gdb.ada/excep_handle.exp: New testcase. diff --git a/gdb/testsuite/gdb.guile/scm-ports.exp b/gdb/testsuite/gdb.guile/scm-ports.exp index 48af5e30e1..04170ef4b8 100644 --- a/gdb/testsuite/gdb.guile/scm-ports.exp +++ b/gdb/testsuite/gdb.guile/scm-ports.exp @@ -104,7 +104,7 @@ proc test_mem_port_rw { kind } { "get sp reg" # Note: Only use $sp_reg for gdb_test result matching, don't use it in # gdb commands. Otherwise transcript.N becomes unusable. - set sp_reg [get_integer_valueof "\$sp" 0] + set sp_reg [get_valueof /u "\$sp" 0] gdb_test_no_output "guile (define byte-at-sp (parse-and-eval \"*(char*) \$sp\"))" \ "save current value at sp" # Pass the result of parse-and-eval through value-fetch-lazy!,