This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix BZ15121 -- x/a broken for addresses in shared libraries (try #2)


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]