| Message ID | 1419815569-21854-1-git-send-email-yao@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
> 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.
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; }
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(+)