This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Fix BZ15121 -- x/a broken for addresses in shared libraries (try #2)
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Paul Pluzhnikov <ppluzhnikov at google dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>, Mark Kettenis <mark dot kettenis at xs4all dot nl>, Pedro Alves <palves at redhat dot com>
- Date: Tue, 11 Oct 2016 16:30:12 +0100
- Subject: Re: Fix BZ15121 -- x/a broken for addresses in shared libraries (try #2)
- Authentication-results: sourceware.org; auth=none
- References: <CALoOobM_kWs6Av5NoV8z+=c58A4RBP3bpsSjMo-jmBOimnqCGw@mail.gmail.com>
Hi Paul,
> Attached is my second attempt to fix BZ15121. Tested on Linux/x86_64;
> no new failures.
>
> Previous attempt:
> https://sourceware.org/ml/gdb-patches/2015-09/msg00074.html
>
> The change to long_long.exp needs to be made conditional (i.e. use old
> pattern on MIPS and SH; new pattern everywhere else). I am not sure how
> to achieve that. Should I make it conditional on 'istarget "mips*"' or ... ?
I think the way to go here would be to create a helper procedure say
`has_signed_addresses' and place the necessary `istarget' calls within.
This could go in `lib/gdb.exp' along with similar helpers there, so that
it's available for general use; there might be other test cases needing
it now or in the future.
You can use it then to construct patterns where necessary, although it
does not appear to me you want to change the `gdb_test_ptr' tests in
`gdb.base/long_long.exp', and likely don't need the helper either; see
below.
> 2016-10-10 Paul Pluzhnikov <ppluzhnikov@google.com>
>
> [BZ #15121]
GDB uses a different format for this, i.e. PR gdb/15121 (no brackets, and
the name of the affected component is included).
> * gdb/gdbarch.h (gdbarch_sign_extend_vma): New.
> (set_gdbarch_sign_extend_vma): New.
> * gdb/gdbarch.c (struct gdbarch): Add sign_extend_vma.
> (gdbarch_sign_extend_vma, set_gdbarch_sign_extend_vma): New.
> (gdbarch_find_by_info): Set sign_extend_vma from BFD.
These are generated files, please always change the `gdbarch.sh' master
instead and regenerate. Also no gdb/ prefix as this goes with ChangeLog
in that directory, not at the top level.
> diff --git a/gdb/testsuite/gdb.base/long_long.exp b/gdb/testsuite/gdb.base/long_long.exp
> index ab47374..8f48266 100644
> --- a/gdb/testsuite/gdb.base/long_long.exp
> +++ b/gdb/testsuite/gdb.base/long_long.exp
> @@ -249,7 +249,7 @@ gdb_test "x/2bd b" "1.*-89"
> gdb_test "x/2bu b" "1.*167"
> gdb_test "x/2bo b" "01.*0247"
> gdb_test "x/2bt b" "00000001.*10100111"
> -gdb_test_ptr "x/2ba b" "" "" "0x1.*0xffffffa7" "0x1.*0xffffffffffffffa7"
> +gdb_test_ptr "x/2ba b" "" "" "0x1.*0xa7" "0x1.*0xa7"
> gdb_test "x/2bc b" "1 '.001'.*-89 '.\[0-9\]*'"
> gdb_test "x/2bf b" "1.*-89"
>
> @@ -258,7 +258,7 @@ gdb_test "x/2hd h" "291.*-22738"
> gdb_test "x/2hu h" "291.*42798"
> gdb_test "x/2ho h" "0443.*0123456"
> gdb_test "x/2ht h" "0000000100100011.*1010011100101110"
> -gdb_test_ptr "x/2ha h" "" "" "0x123.*0xffffa72e" "0x123.*0xffffffffffffa72e"
> +gdb_test_ptr "x/2ha h" "" "" "0x123.*0xa72e" "0x123.*0xa72e"
> gdb_test "x/2hc h" "35 '.'.*46 '.'"
> gdb_test "x/2hf h" "291.*-22738"
>
> @@ -267,7 +267,7 @@ gdb_test "x/2wd w" "19088743.*-1490098887"
> gdb_test "x/2wu w" "19088743.*2804868409"
> gdb_test "x/2wo w" "0110642547.*024713562471"
> gdb_test "x/2wt w" "00000001001000110100010101100111.*10100111001011101110010100111001"
> -gdb_test_ptr "x/2wa w" "" "" "0x1234567.*0xa72ee539" "0x1234567.*0xffffffffa72ee539"
> +gdb_test_ptr "x/2wa w" "" "" "0x1234567.*0xa72ee539" "0x1234567.*0xa72ee539"
> gdb_test "x/2wc w" "103 'g'.*57 '9'"
> gdb_test "x/2wf w" "2.99881655e-0?38.*-2.42716126e-0?15"
>
> diff --git a/gdb/value.c b/gdb/value.c
> index b825aec..9c38cfe 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -3040,7 +3040,17 @@ unpack_pointer (struct type *type, const gdb_byte *valaddr)
> {
> /* Assume a CORE_ADDR can fit in a LONGEST (for now). Not sure
> whether we want this to be true eventually. */
> - return unpack_long (type, valaddr);
> + const LONGEST ret = unpack_long (type, valaddr);
> + const int len = TYPE_LENGTH (type);
> +
> + if (sizeof (CORE_ADDR) > len && ret < 0) {
> + struct gdbarch *arch = get_type_arch (type);
> +
> + if (!gdbarch_sign_extend_vma (arch))
> + /* Don't sign-extend 32-bit pointer. BZ 15121. */
> + return ret & ((1UL << (8 * len)) - 1);
s/pointer/pointers/ presumably, however given the test case update is
this actually accurate? AFAICT all data objects are/aren't sign-extended
based on this setting, including 8-bit and 16-bit ones.
This also indicates, as also shown with the test case you've adjusted
above, that this AFAICT is not the right place to make this change. What
these examine commands effectively do is:
(gdb) print (void *)*(int8_t *)...
(gdb) print (void *)*(int16_t *)...
(gdb) print (void *)*(int32_t *)...
and these by the semantics of the C language do need to be sign-extended
regardless of how the target treats addresses. So `unpack_long' does the
right thing here according to `type' and contrary to your comment in
<https://sourceware.org/ml/gdb-patches/2015-09/msg00074.html>
`gdb.base/long_long.exp' does not cover your actual problem.
Based on a cursory look at PR gdb/15121 I can see you have so far
demonstrated the issue with a reference to $ebp, a register defined by the
x86 target. So my first conclusion based solely on this observation would
be that the type of $ebp is incorrectly (according to, as I infer from
your problem statement, the x86 architecture specification) defined as
signed rather than unsigned. Consequently any pointer contained within is
sign- rather than zero-extended in expression processing if the type is
widened.
If this conclusion is right, then the bug ought to be reclassified as PR
tdep/15121 and the x86 register set reviewed so that correct types are
used according to what the architecture asks for. I suggest then making
specific testsuite cases using $ebp and any other registers required to
cover this problem, and including them under gdb.arch/ to be run on x86
targets only.
HTH,
Maciej