Clear upper bits during sign extension

  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(+)
  

> 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...
  

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
  

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.