[PATCH] Clear upper bits during sign extension

[PATCH] Clear upper bits during sign extension

[Yao Qi]

I see the error message "access outside bounds of object referenced
via synthetic pointer" in the two fails below of mips gdb testing

print d[-2]^M
access outside bounds of object referenced via synthetic pointer^M
(gdb) FAIL: gdb.dwarf2/implptrconst.exp: print d[-2]
(gdb) print/d p[-1]^M
access outside bounds of object referenced via synthetic pointer^M
(gdb) FAIL: gdb.dwarf2/implptrpiece.exp: print/d p[-1]

in the first test, 'd[-2]' is processed by GDB as '* (&d[-2])'.  'd'
is a synthetic pointer, so its value is zero, the address of 'd[-2]'
is -2.  In dwarf2loc.c:indirect_pieced_value,

  /* This is an offset requested by GDB, such as value subscripts.
     However, due to how synthetic pointers are implemented, this is
     always presented to us as a pointer type.  This means we have to
     sign-extend it manually as appropriate.  */
  byte_offset = value_as_address (value);                  <---- [1]
  if (TYPE_LENGTH (value_type (value)) < sizeof (LONGEST))
    byte_offset = gdb_sign_extend (byte_offset,            <---- [2]
				   8 * TYPE_LENGTH (value_type (value)));
  byte_offset += piece->v.ptr.offset;

on MIPS target, after [1], byte_offset is -2 (0xfffffffffffffffe),
because 32-bit -2 (as an address) is sign extended to 64-bit.  After
[2], we manually sign extend byte_offset too, and then it becomes
0xfffffffefffffffe, which is wrong.  Function gdb_sign_extend
sign-extends VALUE on bit BIT, and assumes upper bits from bit BIT are
all zero.  That is why the code works well on targets on which address
is zero extended, such as x86.  On these targets, byte_offset is
0xfffffffe (zero extended from 32-bit address -2).

The patch is to clear upper bits of VALUE in gdb_sign_extend first.
Regression tested on mips-linux-gnu, and fixes two fails above.

gdb:

2014-12-29  Yao Qi  <yao@codesourcery.com>

	* utils.c (gdb_sign_extend): Clear bits from BIT in VALUE.
---
 gdb/utils.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/utils.c b/gdb/utils.c
index 47adb67..e029863 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3031,6 +3031,15 @@ gdb_sign_extend (LONGEST value, int bit)
   if (((value >> (bit - 1)) & 1) != 0)
     {
       LONGEST signbit = ((LONGEST) 1) << (bit - 1);
+      LONGEST mask = 1;
+      int i;
+
+      /* Generate a mask in which bits [0, BIT - 1] are one.  */
+      for (i = 0; i < bit; i++)
+	mask = mask << 1;
+      mask--;
+      /* Clear bits from bit BIT.  */
+      value &= mask;
 
       value = (value ^ signbit) - signbit;
     }
-- 
1.9.3

Re: [PATCH] Clear upper bits during sign extension

[Joel Brobecker]

> 2014-12-29  Yao Qi  <yao@codesourcery.com>
> 
> 	* utils.c (gdb_sign_extend): Clear bits from BIT in VALUE.
> ---
>  gdb/utils.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 47adb67..e029863 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3031,6 +3031,15 @@ gdb_sign_extend (LONGEST value, int bit)
>    if (((value >> (bit - 1)) & 1) != 0)
>      {
>        LONGEST signbit = ((LONGEST) 1) << (bit - 1);
> +      LONGEST mask = 1;
> +      int i;
> +
> +      /* Generate a mask in which bits [0, BIT - 1] are one.  */
> +      for (i = 0; i < bit; i++)
> +	mask = mask << 1;
> +      mask--;
> +      /* Clear bits from bit BIT.  */
> +      value &= mask;

I think you can simplify the above with just:

        value &= ((LONGEST) 1 << bit) - 1;

? I don't know if the cast to LONGEST is really necessary, but we use
it for signbit, so I did the same for the mask...

-- 
Joel

Re: [PATCH] Clear upper bits during sign extension

[Yao Qi]

Joel Brobecker <brobecker@adacore.com> writes:

> I think you can simplify the above with just:
>
>         value &= ((LONGEST) 1 << bit) - 1;
>
> ? I don't know if the cast to LONGEST is really necessary, but we use
> it for signbit, so I did the same for the mask...

Yeah, that is better, or even we can left shift signbit by one.

-- 
Yao (齐尧)

gdb:

2014-12-29  Yao Qi  <yao@codesourcery.com>

        * utils.c (gdb_sign_extend): Clear bits from BIT in VALUE.

diff --git a/gdb/utils.c b/gdb/utils.c
index 47adb67..61873f7 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3032,6 +3032,9 @@ gdb_sign_extend (LONGEST value, int bit)
     {
       LONGEST signbit = ((LONGEST) 1) << (bit - 1);
 
+      /* Clear upper bits from bit BIT.  */
+      value &= (signbit << 1) - 1;
+
       value = (value ^ signbit) - signbit;
     }

Re: [PATCH] Clear upper bits during sign extension

[Joel Brobecker]

> Yeah, that is better, or even we can left shift signbit by one.

Indeed!

> 2014-12-29  Yao Qi  <yao@codesourcery.com>
> 
>         * utils.c (gdb_sign_extend): Clear bits from BIT in VALUE.

Looks good to me.

> diff --git a/gdb/utils.c b/gdb/utils.c
> index 47adb67..61873f7 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3032,6 +3032,9 @@ gdb_sign_extend (LONGEST value, int bit)
>      {
>        LONGEST signbit = ((LONGEST) 1) << (bit - 1);
>  
> +      /* Clear upper bits from bit BIT.  */
> +      value &= (signbit << 1) - 1;
> +
>        value = (value ^ signbit) - signbit;
>      }

-- 
Joel

Re: [PATCH] Clear upper bits during sign extension

[Doug Evans]

On Sun, Dec 28, 2014 at 7:37 PM, Yao Qi <yao@codesourcery.com> wrote:
> Joel Brobecker <brobecker@adacore.com> writes:
>
>> I think you can simplify the above with just:
>>
>>         value &= ((LONGEST) 1 << bit) - 1;
>>
>> ? I don't know if the cast to LONGEST is really necessary, but we use
>> it for signbit, so I did the same for the mask...
>
> Yeah, that is better, or even we can left shift signbit by one.
>
> --
> Yao (齐尧)
>
> gdb:
>
> 2014-12-29  Yao Qi  <yao@codesourcery.com>
>
>         * utils.c (gdb_sign_extend): Clear bits from BIT in VALUE.
>
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 47adb67..61873f7 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -3032,6 +3032,9 @@ gdb_sign_extend (LONGEST value, int bit)
>      {
>        LONGEST signbit = ((LONGEST) 1) << (bit - 1);
>
> +      /* Clear upper bits from bit BIT.  */
> +      value &= (signbit << 1) - 1;
> +
>        value = (value ^ signbit) - signbit;
>      }

It's not immediately clear to this reader that undefined behaviour is
avoided here.
E.g., what if sizeof (LONGEST) == 8 && bit == 64.

There's an assert at the beginning of the function that bit could be 64.

Re: [PATCH] Clear upper bits during sign extension

[Yao Qi]

Doug Evans <dje@google.com> writes:

> It's not immediately clear to this reader that undefined behaviour is
> avoided here.
> E.g., what if sizeof (LONGEST) == 8 && bit == 64.

How about returning VALUE simply in this case? like the patch below,

-- 
Yao (齐尧)

gdb:

2014-12-29  Yao Qi  <yao@codesourcery.com>

        * utils.c (gdb_sign_extend): Do sign extension if BIT is less
        than 8 * sizeof (LONGEST).  Clear bits from BIT in VALUE.

diff --git a/gdb/utils.c b/gdb/utils.c
index 47adb67..83a6df6 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3028,10 +3028,13 @@ gdb_sign_extend (LONGEST value, int bit)
 {
   gdb_assert (bit >= 1 && bit <= 8 * sizeof (LONGEST));
 
-  if (((value >> (bit - 1)) & 1) != 0)
+  if (bit < 8 * sizeof (LONGEST) && ((value >> (bit - 1)) & 1) != 0)
     {
       LONGEST signbit = ((LONGEST) 1) << (bit - 1);
 
+      /* Clear upper bits from bit BIT.  */
+      value &= (signbit << 1) - 1;
+
       value = (value ^ signbit) - signbit;
     }
 

Re: [PATCH] Clear upper bits during sign extension

[Pedro Alves]

On 12/29/2014 01:12 AM, Yao Qi wrote:
> I see the error message "access outside bounds of object referenced
> via synthetic pointer" in the two fails below of mips gdb testing
> 
> print d[-2]^M
> access outside bounds of object referenced via synthetic pointer^M
> (gdb) FAIL: gdb.dwarf2/implptrconst.exp: print d[-2]
> (gdb) print/d p[-1]^M
> access outside bounds of object referenced via synthetic pointer^M
> (gdb) FAIL: gdb.dwarf2/implptrpiece.exp: print/d p[-1]
> 
> in the first test, 'd[-2]' is processed by GDB as '* (&d[-2])'.  'd'
> is a synthetic pointer, so its value is zero, the address of 'd[-2]'
> is -2.  In dwarf2loc.c:indirect_pieced_value,
> 
>   /* This is an offset requested by GDB, such as value subscripts.
>      However, due to how synthetic pointers are implemented, this is
>      always presented to us as a pointer type.  This means we have to
>      sign-extend it manually as appropriate.  */
>   byte_offset = value_as_address (value);                  <---- [1]
>   if (TYPE_LENGTH (value_type (value)) < sizeof (LONGEST))
>     byte_offset = gdb_sign_extend (byte_offset,            <---- [2]
> 				   8 * TYPE_LENGTH (value_type (value)));
>   byte_offset += piece->v.ptr.offset;
> 
> on MIPS target, after [1], byte_offset is -2 (0xfffffffffffffffe),
> because 32-bit -2 (as an address) is sign extended to 64-bit.  After
> [2], we manually sign extend byte_offset too, and then it becomes
> 0xfffffffefffffffe, which is wrong.  Function gdb_sign_extend
> sign-extends VALUE on bit BIT, and assumes upper bits from bit BIT are
> all zero.  That is why the code works well on targets on which address
> is zero extended, such as x86.  On these targets, byte_offset is
> 0xfffffffe (zero extended from 32-bit address -2).
> 
> The patch is to clear upper bits of VALUE in gdb_sign_extend first.
> Regression tested on mips-linux-gnu, and fixes two fails above.

This seems to me to paper over an issue elsewhere, and is likely
to paper over issues as gdb_sign_extend is used more throughout.

I'm not immediately familiar with all the conditions indirect_pieced_value
is called, but going by the comment quoted, I think the root issue
might be that we shouldn't use value_as_address in the first place,
but something like unpack_long directly.

E.g., I don't see how it makes sense to interpret -2 as an address
on spu, which ends up calling:

 static CORE_ADDR
 spu_integer_to_address (struct gdbarch *gdbarch,
 			struct type *type, const gdb_byte *buf)
 {
   int id = spu_gdbarch_id (gdbarch);
   ULONGEST addr = unpack_long (type, buf);

   return SPUADDR (id, addr);
 }

or on avr, which ends up calling:

static CORE_ADDR
avr_integer_to_address (struct gdbarch *gdbarch,
			struct type *type, const gdb_byte *buf)
{
  ULONGEST addr = unpack_long (type, buf);

  return avr_make_saddr (addr);
}

...

/* SRAM address checks and convertions.  */

static CORE_ADDR
avr_make_saddr (CORE_ADDR x)
{
  /* Return 0 for NULL.  */
  if (x == 0)
    return 0;

  return ((x) | AVR_SMEM_START);
}

...
  AVR_SMEM_START = 0x00800000,	/* SRAM memory */
...

Thanks,
Pedro Alves

Re: [PATCH] Clear upper bits during sign extension

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> This seems to me to paper over an issue elsewhere, and is likely
> to paper over issues as gdb_sign_extend is used more throughout.
>
> I'm not immediately familiar with all the conditions indirect_pieced_value
> is called, but going by the comment quoted, I think the root issue
> might be that we shouldn't use value_as_address in the first place,
> but something like unpack_long directly.

indirect_pieced_value is called by value_ind, in which its argument ARG1
should be regarded as an address, IMO.

#0  indirect_pieced_value (value=0x8af0fd8) at ../../../git/gdb/dwarf2loc.c:2006
#1  0x081d99fa in value_ind (arg1=0x8af0fd8) at ../../../git/gdb/valops.c:1548
#2  0x081de7f2 in value_subscript (array=0x8b41678, index=-2) at ../../../git/gdb/valarith.c:181

See value_ind's comment:

/* Given a value of a pointer type, apply the C unary * operator to
   it.  */

struct value *
value_ind (struct value *arg1)

>
> E.g., I don't see how it makes sense to interpret -2 as an address
> on spu, which ends up calling:

-2 is *not* the address in this case.  The address is 0xfffffffe, and
 sign extended to 64-bit (0xfffffffffffffffe) on MIPS target.

>
>  static CORE_ADDR
>  spu_integer_to_address (struct gdbarch *gdbarch,
>  			struct type *type, const gdb_byte *buf)
>  {
>    int id = spu_gdbarch_id (gdbarch);
>    ULONGEST addr = unpack_long (type, buf);
>
>    return SPUADDR (id, addr);
>  }

Sorry, I don't understand how is gdbarch_integer_to_address hook related
to this problem.  The address (0xfffffffe) is the address of synthetic
pointer, instead of the actual address.

-- 
Yao (齐尧)

Re: [PATCH] Clear upper bits during sign extension

[Pedro Alves]

On 12/30/2014 09:19 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> This seems to me to paper over an issue elsewhere, and is likely
>> to paper over issues as gdb_sign_extend is used more throughout.
>>
>> I'm not immediately familiar with all the conditions indirect_pieced_value
>> is called, but going by the comment quoted, I think the root issue
>> might be that we shouldn't use value_as_address in the first place,
>> but something like unpack_long directly.
> 
> indirect_pieced_value is called by value_ind, in which its argument ARG1
> should be regarded as an address, IMO.
> 
> #0  indirect_pieced_value (value=0x8af0fd8) at ../../../git/gdb/dwarf2loc.c:2006
> #1  0x081d99fa in value_ind (arg1=0x8af0fd8) at ../../../git/gdb/valops.c:1548
> #2  0x081de7f2 in value_subscript (array=0x8b41678, index=-2) at ../../../git/gdb/valarith.c:181
> 
> See value_ind's comment:
> 
> /* Given a value of a pointer type, apply the C unary * operator to
>    it.  */
> 
> struct value *
> value_ind (struct value *arg1)
> 
>>
>> E.g., I don't see how it makes sense to interpret -2 as an address
>> on spu, which ends up calling:
> 
> -2 is *not* the address in this case.  The address is 0xfffffffe, and
>  sign extended to 64-bit (0xfffffffffffffffe) on MIPS target.

Well, that -2 is being interpreted as an address, given value_as_address is
called on it.   From your original post:

>> in the first test, 'd[-2]' is processed by GDB as '* (&d[-2])'.  'd'
>> is a synthetic pointer, so its value is zero, the address of 'd[-2]'
>> is -2.  In dwarf2loc.c:indirect_pieced_value,
>>
>>   /* This is an offset requested by GDB, such as value subscripts.
>>      However, due to how synthetic pointers are implemented, this is
>>      always presented to us as a pointer type.  This means we have to
>>      sign-extend it manually as appropriate.  */
>>   byte_offset = value_as_address (value);                  <---- [1]

...

>> on MIPS target, after [1], byte_offset is -2 (0xfffffffffffffffe),
>> because 32-bit -2 (as an address) is sign extended to 64-bit.  After

> Sorry, I don't understand how is gdbarch_integer_to_address hook related
> to this problem.  The address (0xfffffffe) is the address of synthetic
> pointer, instead of the actual address.

I thought value_as_address was reaching the call to gdbarch_integer_to_address.
But given indirect_pieced_value has this at the top:

  if (TYPE_CODE (type) != TYPE_CODE_PTR)
    return NULL;

we know we're handling a TYPE_CODE_PTR.

That means that instead, value_as_address is calling unpack_long
at the bottom, which then calls extract_typed_address, which calls
gdbarch_pointer_to_address.  The same point applies.  The default of
that hook is unsigned_pointer_to_address.  But on MIPS, the hook
calls signed_pointer_to_address, which does the sign extension.

That would suggest the fix to be to do something like:

  /* This is an offset requested by GDB, such as value subscripts.
     However, due to how synthetic pointers are implemented, this is
     always presented to us as a pointer type.  This means we have to
-     sign-extend it manually as appropriate.  */
+     sign-extend it if needed (on some architectures, like MIPS,
+     addresses are signed).

   byte_offset = value_as_address (value);
-  if (TYPE_LENGTH (value_type (value)) < sizeof (LONGEST))
+  if (TYPE_UNSIGNED (value_type (value)
+      && TYPE_LENGTH (value_type (value)) < sizeof (LONGEST))
    byte_offset = gdb_sign_extend (byte_offset,
				   8 * TYPE_LENGTH (value_type (value)));

however, that would not look correct to me on AVR, SPU, or other ports
that install a custom gdbarch_pointer_to_address hook, where value_as_address
ends up returning a CORE_ADDR that had some magic bit manipulations
thrown in.  Your change to gdb_sign_extend would wipe those (high) bits
out, for sure, but that clearly is not the intended role of gdb_sign_extend,
so looks brittle and not as direct to rely on that.

So what we need here is to get back the raw value of the pointer
as a signed integer, without any GDB magic address bits.
That is, we don't want the manipulations from gdbarch_pointer_to_address.

So I think we should either explicitly always clear bits above TYPE_LENGTH
after value_as_address, with a comment mentioning that we don't want
any magic bits that gdbarch_pointer_to_address would give us,
or, given we know the value is really an offset, simply extract the value
that way.  Like in the patch below:

From 57e268c3f0da5eb90f6c39c307b60c321c76faa2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 30 Dec 2014 11:07:35 +0000
Subject: [PATCH] always read synthetic pointers as signed integers

---
 gdb/dwarf2loc.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index fd5856c..5dd0867 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2012,6 +2012,7 @@ indirect_pieced_value (struct value *value)
   int i, bit_offset, bit_length;
   struct dwarf_expr_piece *piece = NULL;
   LONGEST byte_offset;
+  enum bfd_endian byte_order;

   type = check_typedef (value_type (value));
   if (TYPE_CODE (type) != TYPE_CODE_PTR)
@@ -2056,11 +2057,16 @@ indirect_pieced_value (struct value *value)
   /* This is an offset requested by GDB, such as value subscripts.
      However, due to how synthetic pointers are implemented, this is
      always presented to us as a pointer type.  This means we have to
-     sign-extend it manually as appropriate.  */
-  byte_offset = value_as_address (value);
-  if (TYPE_LENGTH (value_type (value)) < sizeof (LONGEST))
-    byte_offset = gdb_sign_extend (byte_offset,
-				   8 * TYPE_LENGTH (value_type (value)));
+     sign-extend it manually as appropriate.  Use raw
+     extract_signed_integer directly rather than value_as_address and
+     sign extend afterwards on architectures that would need it
+     (mostly everywhere except MIPS, which has signed addresses) as
+     the later would go through gdbarch_pointer_to_address and thus
+     return a CORE_ADDR with high bits set on architectures that
+     encode address spaces and other things in CORE_ADDR.  */
+  byte_order = gdbarch_byte_order (get_type_arch (type));
+  byte_offset = extract_signed_integer (value_contents (value),
+					TYPE_LENGTH (type), byte_order);
   byte_offset += piece->v.ptr.offset;

   gdb_assert (piece);
-- 
1.9.3

Re: [PATCH] Clear upper bits during sign extension

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> So I think we should either explicitly always clear bits above TYPE_LENGTH
> after value_as_address, with a comment mentioning that we don't want
> any magic bits that gdbarch_pointer_to_address would give us,
> or, given we know the value is really an offset, simply extract the value
> that way.  Like in the patch below:

The latter is fine to me.

> +  byte_order = gdbarch_byte_order (get_type_arch (type));

How about getting gdbarch via get_frame_arch (frame)?  How about
removing gdb_sign_extend as it is no longer used?

I'll post a full version on top of yours.

-- 
Yao (齐尧)

Re: [PATCH] Clear upper bits during sign extension

[Yao Qi]

Yao Qi <yao@codesourcery.com> writes:

>> +  byte_order = gdbarch_byte_order (get_type_arch (type));
>
> How about getting gdbarch via get_frame_arch (frame)?  How about
> removing gdb_sign_extend as it is no longer used?
>
> I'll post a full version on top of yours.

Here is the patch, what do you think?

-- 
Yao (齐尧)

Subject: [PATCH] always read synthetic pointers as signed integers

I see the error message "access outside bounds of object referenced
via synthetic pointer" in the two fails below of mips gdb testing

print d[-2]^M
access outside bounds of object referenced via synthetic pointer^M
(gdb) FAIL: gdb.dwarf2/implptrconst.exp: print d[-2]
(gdb) print/d p[-1]^M
access outside bounds of object referenced via synthetic pointer^M
(gdb) FAIL: gdb.dwarf2/implptrpiece.exp: print/d p[-1]

in the first test, 'd[-2]' is processed by GDB as '* (&d[-2])'.  'd'
is a synthetic pointer, so its value is zero, the address of 'd[-2]'
is -2.  In dwarf2loc.c:indirect_pieced_value,

  /* This is an offset requested by GDB, such as value subscripts.
     However, due to how synthetic pointers are implemented, this is
     always presented to us as a pointer type.  This means we have to
     sign-extend it manually as appropriate.  */
  byte_offset = value_as_address (value);
  if (TYPE_LENGTH (value_type (value)) < sizeof (LONGEST))
    byte_offset = gdb_sign_extend (byte_offset,
				   8 * TYPE_LENGTH (value_type (value)));
  byte_offset += piece->v.ptr.offset;

We know that the value is really an offset instead of address, so the
fix is to extract the value as an (signed) offset.

gdb:

2015-01-08  Pedro Alves  <palves@redhat.com>
	    Yao Qi  <yao@codesourcery.com>

	* dwarf2loc.c (indirect_pieced_value): Don't call
	gdb_sign_extend.  Call extract_signed_integer instead.
	* utils.c (gdb_sign_extend): Remove.
	* utils.h (gdb_sign_extend): Remove declaration.
---
 gdb/dwarf2loc.c | 16 +++++++++++-----
 gdb/utils.c     | 17 -----------------
 gdb/utils.h     |  5 -----
 3 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 2bd12d6..bdb2160 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -2012,6 +2012,7 @@ indirect_pieced_value (struct value *value)
   int i, bit_offset, bit_length;
   struct dwarf_expr_piece *piece = NULL;
   LONGEST byte_offset;
+  enum bfd_endian byte_order;
 
   type = check_typedef (value_type (value));
   if (TYPE_CODE (type) != TYPE_CODE_PTR)
@@ -2056,11 +2057,16 @@ indirect_pieced_value (struct value *value)
   /* This is an offset requested by GDB, such as value subscripts.
      However, due to how synthetic pointers are implemented, this is
      always presented to us as a pointer type.  This means we have to
-     sign-extend it manually as appropriate.  */
-  byte_offset = value_as_address (value);
-  if (TYPE_LENGTH (value_type (value)) < sizeof (LONGEST))
-    byte_offset = gdb_sign_extend (byte_offset,
-				   8 * TYPE_LENGTH (value_type (value)));
+     sign-extend it manually as appropriate.  Use raw
+     extract_signed_integer directly rather than value_as_address and
+     sign extend afterwards on architectures that would need it
+     (mostly everywhere except MIPS, which has signed addresses) as
+     the later would go through gdbarch_pointer_to_address and thus
+     return a CORE_ADDR with high bits set on architectures that
+     encode address spaces and other things in CORE_ADDR.  */
+  byte_order = gdbarch_byte_order (get_frame_arch (frame));
+  byte_offset = extract_signed_integer (value_contents (value),
+					TYPE_LENGTH (type), byte_order);
   byte_offset += piece->v.ptr.offset;
 
   gdb_assert (piece);
diff --git a/gdb/utils.c b/gdb/utils.c
index 084db87..72b1e2a 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -3021,23 +3021,6 @@ align_down (ULONGEST v, int n)
   return (v & -n);
 }
 
-/* See utils.h.  */
-
-LONGEST
-gdb_sign_extend (LONGEST value, int bit)
-{
-  gdb_assert (bit >= 1 && bit <= 8 * sizeof (LONGEST));
-
-  if (((value >> (bit - 1)) & 1) != 0)
-    {
-      LONGEST signbit = ((LONGEST) 1) << (bit - 1);
-
-      value = (value ^ signbit) - signbit;
-    }
-
-  return value;
-}
-
 /* Allocation function for the libiberty hash table which uses an
    obstack.  The obstack is passed as DATA.  */
 
diff --git a/gdb/utils.h b/gdb/utils.h
index 0a73864..3debde7 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -340,11 +340,6 @@ extern int myread (int, char *, int);
 extern ULONGEST align_up (ULONGEST v, int n);
 extern ULONGEST align_down (ULONGEST v, int n);
 
-/* Sign extend VALUE.  BIT is the (1-based) index of the bit in VALUE
-   to sign-extend.  */
-
-extern LONGEST gdb_sign_extend (LONGEST value, int bit);
-
 /* Resource limits used by getrlimit and setrlimit.  */
 
 enum resource_limit_kind
-- 
1.9.3

Re: [PATCH] Clear upper bits during sign extension

[Pedro Alves]

On 01/08/2015 05:40 AM, Yao Qi wrote:
> Yao Qi <yao@codesourcery.com> writes:
> 
>>> +  byte_order = gdbarch_byte_order (get_type_arch (type));
>>
>> How about getting gdbarch via get_frame_arch (frame)?  How about
>> removing gdb_sign_extend as it is no longer used?
>>
>> I'll post a full version on top of yours.
> 
> Here is the patch, what do you think?

Looks good to me.  Thanks.

-- 
Pedro Alves

Re: [PATCH] Clear upper bits during sign extension

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Looks good to me.  Thanks.

Patch is pushed in.

-- 
Yao (齐尧)