* [RFA] Fix scm-ports.exp regression
@ 2018-01-03 18:20 Tom Tromey
2018-01-05 17:01 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2018-01-03 18:20 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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(-)
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!,
--
2.13.6
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
2018-01-03 18:20 [RFA] Fix scm-ports.exp regression Tom Tromey
@ 2018-01-05 17:01 ` Pedro Alves
2018-01-09 18:27 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-01-05 17:01 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
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!,
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
2018-01-05 17:01 ` Pedro Alves
@ 2018-01-09 18:27 ` Tom Tromey
2018-01-15 16:51 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2018-01-09 18:27 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
2018-01-09 18:27 ` Tom Tromey
@ 2018-01-15 16:51 ` Pedro Alves
2018-01-15 18:17 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-01-15 16:51 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
2018-01-15 16:51 ` Pedro Alves
@ 2018-01-15 18:17 ` Tom Tromey
2018-01-15 20:59 ` Sergio Durigan Junior
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2018-01-15 18:17 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
2018-01-15 18:17 ` Tom Tromey
@ 2018-01-15 20:59 ` Sergio Durigan Junior
2018-01-15 22:10 ` Tom Tromey
0 siblings, 1 reply; 9+ messages in thread
From: Sergio Durigan Junior @ 2018-01-15 20:59 UTC (permalink / raw)
To: Tom Tromey; +Cc: Pedro Alves, gdb-patches
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
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
2018-01-15 20:59 ` Sergio Durigan Junior
@ 2018-01-15 22:10 ` Tom Tromey
2018-01-16 7:39 ` Joel Brobecker
0 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2018-01-15 22:10 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Tom Tromey, Pedro Alves, gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
2018-01-15 22:10 ` Tom Tromey
@ 2018-01-16 7:39 ` Joel Brobecker
0 siblings, 0 replies; 9+ messages in thread
From: Joel Brobecker @ 2018-01-16 7:39 UTC (permalink / raw)
To: Tom Tromey; +Cc: Sergio Durigan Junior, Pedro Alves, gdb-patches
> 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.
--
Joel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Fix scm-ports.exp regression
@ 2018-01-05 17:59 Doug Evans via gdb-patches
0 siblings, 0 replies; 9+ messages in thread
From: Doug Evans via gdb-patches @ 2018-01-05 17:59 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
Tom Tromey writes:
> 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.
LGTM
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-01-16 7:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-03 18:20 [RFA] Fix scm-ports.exp regression Tom Tromey
2018-01-05 17:01 ` Pedro Alves
2018-01-09 18:27 ` Tom Tromey
2018-01-15 16:51 ` Pedro Alves
2018-01-15 18:17 ` Tom Tromey
2018-01-15 20:59 ` Sergio Durigan Junior
2018-01-15 22:10 ` Tom Tromey
2018-01-16 7:39 ` Joel Brobecker
2018-01-05 17:59 Doug Evans via gdb-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).