This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Fix scm-ports.exp regression
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