diff mbox

Fix BZ15121 -- x/a broken for addresses in shared libraries

Message ID CALoOobP+FH-rRO-HYOFgu0QrfHwgteeu_ufawVDL9zeyef7h7g@mail.gmail.com
State New, archived
Headers

Commit Message

Paul Pluzhnikov Sept. 6, 2015, 8:03 p.m. UTC 
  Greetings,

Attached patch fixes BZ #15121 -- x/a broken for addresses in shared libraries.
Not adding a new test since this is already covered by gdb.base/long_long.exp

Tested on Linux/x86_64, no new failures.

Thanks,

ChangeLog:

2015-09-06  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #15121]
        * gdb/value.c (unpack_pointer): Don't sign-extend.

testsuite/ChangeLog:

2015-09-06  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * gdb.base/long_long.exp: Adjust.

Comments

Pedro Alves Sept. 10, 2015, 2:17 p.m. UTC | #1 
On 09/06/2015 09:03 PM, Paul Pluzhnikov wrote:

> index 91bf49e..0624b90 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -2927,7 +2927,13 @@ 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);
> +  LONGEST ret = unpack_long (type, valaddr);
> +  int len = TYPE_LENGTH (type);
> +  if (sizeof (CORE_ADDR) > len && ret < 0) {
> +    /* Don't sign-extend 32-bit pointer.  BZ 15121.  */
> +    return ret & ((1UL << (8 * len)) - 1);
> +  }
> +  return ret;
>  }

Do we need to keep sign-extending on MIPS?  Adding Maciej.

Thanks,
Pedro Alves
Mark Kettenis Sept. 10, 2015, 2:33 p.m. UTC | #2 
> Date: Thu, 10 Sep 2015 15:17:36 +0100
> From: Pedro Alves <palves@redhat.com>
> 
> On 09/06/2015 09:03 PM, Paul Pluzhnikov wrote:
> 
> > index 91bf49e..0624b90 100644
> > --- a/gdb/value.c
> > +++ b/gdb/value.c
> > @@ -2927,7 +2927,13 @@ 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);
> > +  LONGEST ret = unpack_long (type, valaddr);
> > +  int len = TYPE_LENGTH (type);
> > +  if (sizeof (CORE_ADDR) > len && ret < 0) {
> > +    /* Don't sign-extend 32-bit pointer.  BZ 15121.  */
> > +    return ret & ((1UL << (8 * len)) - 1);
> > +  }
> > +  return ret;
> >  }
> 
> Do we need to keep sign-extending on MIPS?  Adding Maciej.

I'm pretty sure you do.
Paul Pluzhnikov Sept. 10, 2015, 3 p.m. UTC | #3 
On Thu, Sep 10, 2015 at 7:33 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> > From: Pedro Alves <palves@redhat.com>

> > Do we need to keep sign-extending on MIPS?  Adding Maciej.
>
> I'm pretty sure you do.

What makes MIPS special?

I can't imagine any reason why (upper levels of) 64-bit GDB would need
to be lied to that the 32-bit inferior's 0x80000004 pointer has
CORE_ADDR value of 0xFFFFFFFF80000004.

Thanks,
Maciej W. Rozycki Sept. 10, 2015, 3:02 p.m. UTC | #4 
On Thu, 10 Sep 2015, Mark Kettenis wrote:

> > > index 91bf49e..0624b90 100644
> > > --- a/gdb/value.c
> > > +++ b/gdb/value.c
> > > @@ -2927,7 +2927,13 @@ 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);
> > > +  LONGEST ret = unpack_long (type, valaddr);
> > > +  int len = TYPE_LENGTH (type);
> > > +  if (sizeof (CORE_ADDR) > len && ret < 0) {
> > > +    /* Don't sign-extend 32-bit pointer.  BZ 15121.  */
> > > +    return ret & ((1UL << (8 * len)) - 1);
> > > +  }
> > > +  return ret;
> > >  }
> > 
> > Do we need to keep sign-extending on MIPS?  Adding Maciej.

 Thanks for the heads-up!

> I'm pretty sure you do.

 Indeed.  Use `bfd_get_sign_extend_vma' to determine.

  Maciej
Pedro Alves Sept. 10, 2015, 3:07 p.m. UTC | #5 
On 09/10/2015 04:00 PM, Paul Pluzhnikov wrote:
> On Thu, Sep 10, 2015 at 7:33 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
>>> From: Pedro Alves <palves@redhat.com>
> 
>>> Do we need to keep sign-extending on MIPS?  Adding Maciej.
>>
>> I'm pretty sure you do.
> 
> What makes MIPS special?
> 
> I can't imagine any reason why (upper levels of) 64-bit GDB would need
> to be lied to that the 32-bit inferior's 0x80000004 pointer has
> CORE_ADDR value of 0xFFFFFFFF80000004.

MIPS has signed addresses, at the hardware level.  Thus a 32-bit ABI on
a 64-bit machine ends up seeing with sign-extended addresses.

See e.g.:
 https://www.sourceware.org/ml/gdb/2002-09/msg00084.html

Thanks,
Pedro Alves
Maciej W. Rozycki Sept. 10, 2015, 3:19 p.m. UTC | #6 
On Thu, 10 Sep 2015, Pedro Alves wrote:

> > I can't imagine any reason why (upper levels of) 64-bit GDB would need
> > to be lied to that the 32-bit inferior's 0x80000004 pointer has
> > CORE_ADDR value of 0xFFFFFFFF80000004.
> 
> MIPS has signed addresses, at the hardware level.  Thus a 32-bit ABI on
> a 64-bit machine ends up seeing with sign-extended addresses.

 SH seems to be the same.

  Maciej
Paul Pluzhnikov Sept. 12, 2015, 9:41 p.m. UTC | #7 
On Thu, Sep 10, 2015 at 8:02 AM, Maciej W. Rozycki <macro@linux-mips.org> wrote:

>  Indeed.  Use `bfd_get_sign_extend_vma' to determine.

For the life of me, I can't figure out how I can get the bfd in the
context of do_examine().

Clues?

Thanks,
Maciej W. Rozycki Sept. 12, 2015, 9:56 p.m. UTC | #8 
On Sat, 12 Sep 2015, Paul Pluzhnikov wrote:

> >  Indeed.  Use `bfd_get_sign_extend_vma' to determine.
> 
> For the life of me, I can't figure out how I can get the bfd in the
> context of do_examine().
> 
> Clues?

 If it's not reachable in this context, then you may have to figure it out 
earlier on and store in `gdbarch'.

 HTH,

  Maciej
diff mbox

Patch

diff --git a/gdb/testsuite/gdb.base/long_long.exp b/gdb/testsuite/gdb.base/long_long.exp
index 5eea391..f4f3961 100644
--- a/gdb/testsuite/gdb.base/long_long.exp
+++ b/gdb/testsuite/gdb.base/long_long.exp
@@ -255,7 +255,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"
 
@@ -264,7 +264,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"
 
@@ -273,7 +273,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 91bf49e..0624b90 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -2927,7 +2927,13 @@  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);
+  LONGEST ret = unpack_long (type, valaddr);
+  int len = TYPE_LENGTH (type);
+  if (sizeof (CORE_ADDR) > len && ret < 0) {
+    /* Don't sign-extend 32-bit pointer.  BZ 15121.  */
+    return ret & ((1UL << (8 * len)) - 1);
+  }
+  return ret;
 }