From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13737 invoked by alias); 27 Aug 2014 04:01:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 13708 invoked by uid 89); 27 Aug 2014 04:01:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-la0-f44.google.com Received: from mail-la0-f44.google.com (HELO mail-la0-f44.google.com) (209.85.215.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 27 Aug 2014 04:01:22 +0000 Received: by mail-la0-f44.google.com with SMTP id el20so16098526lab.17 for ; Tue, 26 Aug 2014 21:01:17 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.152.42.233 with SMTP id r9mr2639053lal.28.1409112077583; Tue, 26 Aug 2014 21:01:17 -0700 (PDT) Received: by 10.25.23.204 with HTTP; Tue, 26 Aug 2014 21:01:17 -0700 (PDT) In-Reply-To: <201404141700.s3EH0F7v020732@d06av02.portsmouth.uk.ibm.com> References: <201404141700.s3EH0F7v020732@d06av02.portsmouth.uk.ibm.com> Date: Wed, 27 Aug 2014 04:01:00 -0000 Message-ID: Subject: Re: [RFC] Use address_from_register in dwarf2-frame.c:read_addr_from_reg From: Andrew Pinski To: Ulrich Weigand Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00542.txt.bz2 On Mon, Apr 14, 2014 at 10:00 AM, Ulrich Weigand wrote: > Hello, > > with recent compilers I'm seeing lots of failures in the GDB testsuite on SPU, > because debug data now tends to use features more frequently that rely on the > debugger using DWARF frame unwinding. I had never enabled that on spu, and > just used prologue parsing instead, which shows up as a problem now ... > > However, I cannot simply enable DWARF unwinding, since dwarf2-frame.c common > code does not handle the situation well (that we have on SPU) where the stack > and/or frame pointer are maintained in *vector* registers. This is because > read_addr_from_reg is hard-coded to assume that such pointers can be read > from registers via a simple get_frame_register / unpack_pointer operation. > > Now, there *is* a routine address_from_register that calls into the > appropriate tdep routines to handle pointer values in "weird" registers > like on SPU, but it turns out I cannot simply change dwarf2-frame.c to > use address_from_register. This is because address_from_register uses > value_from_register to create a (temporary) value, and that routine > at some point calls get_frame_id in order to set up that value's > VALUE_FRAME_ID entry. > > However, the dwarf2-frame.c read_addr_from_reg routine will be called > during early unwinding (to unwind the frame's CFA), at which point the > frame's ID is not actually known yet! This would cause an assert. > > I not completely sure what the best fix is. > > One way might be to recognize that VALUE_FRAME_ID is only needed in the > value returned by value_from_register if that value is later used as an > lvalue. But this is obviously never done to the temporary value used in > address_from_register. So, if we could change address_from_register to > not call value_from_register but instead accept constructing a value > that doesn't have VALUE_FRAME_ID set, things should be fine. > > To do that, we can change the value_from_register callback to accept > a FRAME_ID instead of a FRAME; the only existing uses of the FRAME > argument were either to extract its frame ID, or its gdbarch. (To > keep a way of getting at the latter, we also change the callback's > type from "f" to "m".) Together with the required follow-on changes > in the existing value_from_register implementations (including the > default one), this seems to fix the problem. > > As another minor interface cleanup, I've removed the explicit TYPE > argument from address_from_register. This routine really always > uses a default pointer type, and in the new implementation it -to > some extent- relies on that fact, in that it will now no longer > handle types that require gdbarch_convert_register_p handling. > > Does this sound reasonable? Comments or alternative suggestions > would be appreciated! > > Tested on powerpc64-linux and spu-elf with no regressions. > > If there are no objections, I'm planning on committing this > later this week. I think this patch broke MIPS64 n32 big-endian support. We assert here: gdb_assert (!gdbarch_convert_register_p (gdbarch, regnum, type)); The convert_register_p code for MIPS does: return (register_size (gdbarch, regnum) == 8 && regnum % num_regs > 0 && regnum % num_regs < 32 && TYPE_LENGTH (type) < 8); Since the register size is 8 byte wide (MIPS64) and the type length is 4 (pointer), we return true. In MIPS64, the registers are stored 64bits but pointers are 32bits. Here is the code that is used by mips_register_to_value: int len = TYPE_LENGTH (type); CORE_ADDR offset; offset = gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG ? 8 - len : 0; if (!get_frame_register_bytes (frame, regnum, offset, len, to, optimizedp, unavailablep)) return 0; *optimizedp = *unavailablep = 0; return 1; Is there a way to fix this in a target neutral way? (I might need a way like this for AARCH64 ILP32 also). Thanks, Andrew Pinski > > Bye, > Ulrich > > > ChangeLog: > > * gdbarch.sh (value_from_register): Make class "m" instead of "f". > Replace FRAME argument with FRAME_ID. > * gdbarch.c, gdbarch.h: Regenerate. > * findvar.c (default_value_from_register): Add GDBARCH argument; > replace FRAME by FRAME_ID. No longer call get_frame_id. > (value_from_register): Update call to gdbarch_value_from_register. > * value.h (default_value_from_register): Update prototype. > * s390-linux-tdep.c (s390_value_from_register): Update interface > and call to default_value_from_register. > * spu-tdep.c (spu_value_from_register): Likewise. > > * findvar.c (address_from_register): Remove TYPE argument. > Do not call value_from_register; use gdbarch_value_from_register > with null_frame_id instead. > * value.h (address_from_register): Update prototype. > * dwarf2-frame.c (read_addr_from_reg): Use address_from_register. > * dwarf2loc.c (dwarf_expr_read_addr_from_reg): Update for > address_from_register interface change. > > > Index: binutils-gdb/gdb/dwarf2-frame.c > =================================================================== > --- binutils-gdb.orig/gdb/dwarf2-frame.c > +++ binutils-gdb/gdb/dwarf2-frame.c > @@ -291,15 +291,9 @@ read_addr_from_reg (void *baton, int reg > { > struct frame_info *this_frame = (struct frame_info *) baton; > struct gdbarch *gdbarch = get_frame_arch (this_frame); > - int regnum; > - gdb_byte *buf; > - > - regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, reg); > - > - buf = alloca (register_size (gdbarch, regnum)); > - get_frame_register (this_frame, regnum, buf); > + int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, reg); > > - return unpack_pointer (register_type (gdbarch, regnum), buf); > + return address_from_register (regnum, this_frame); > } > > /* Implement struct dwarf_expr_context_funcs' "get_reg_value" callback. */ > Index: binutils-gdb/gdb/dwarf2loc.c > =================================================================== > --- binutils-gdb.orig/gdb/dwarf2loc.c > +++ binutils-gdb/gdb/dwarf2loc.c > @@ -317,13 +317,9 @@ dwarf_expr_read_addr_from_reg (void *bat > { > struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton; > struct gdbarch *gdbarch = get_frame_arch (debaton->frame); > - CORE_ADDR result; > - int regnum; > + int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, dwarf_regnum); > > - regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, dwarf_regnum); > - result = address_from_register (builtin_type (gdbarch)->builtin_data_ptr, > - regnum, debaton->frame); > - return result; > + return address_from_register (regnum, debaton->frame); > } > > /* Implement struct dwarf_expr_context_funcs' "get_reg_value" callback. */ > Index: binutils-gdb/gdb/findvar.c > =================================================================== > --- binutils-gdb.orig/gdb/findvar.c > +++ binutils-gdb/gdb/findvar.c > @@ -625,15 +625,14 @@ read_var_value (struct symbol *var, stru > /* Install default attributes for register values. */ > > struct value * > -default_value_from_register (struct type *type, int regnum, > - struct frame_info *frame) > +default_value_from_register (struct gdbarch *gdbarch, struct type *type, > + int regnum, struct frame_id frame_id) > { > - struct gdbarch *gdbarch = get_frame_arch (frame); > int len = TYPE_LENGTH (type); > struct value *value = allocate_value (type); > > VALUE_LVAL (value) = lval_register; > - VALUE_FRAME_ID (value) = get_frame_id (frame); > + VALUE_FRAME_ID (value) = frame_id; > VALUE_REGNUM (value) = regnum; > > /* Any structure stored in more than one register will always be > @@ -741,7 +740,8 @@ value_from_register (struct type *type, > else > { > /* Construct the value. */ > - v = gdbarch_value_from_register (gdbarch, type, regnum, frame); > + v = gdbarch_value_from_register (gdbarch, type, > + regnum, get_frame_id (frame)); > > /* Get the data. */ > read_frame_register_value (v, frame); > @@ -750,18 +750,30 @@ value_from_register (struct type *type, > return v; > } > > -/* Return contents of register REGNUM in frame FRAME as address, > - interpreted as value of type TYPE. Will abort if register > - value is not available. */ > +/* Return contents of register REGNUM in frame FRAME as address. > + Will abort if register value is not available. */ > > CORE_ADDR > -address_from_register (struct type *type, int regnum, struct frame_info *frame) > +address_from_register (int regnum, struct frame_info *frame) > { > + struct gdbarch *gdbarch = get_frame_arch (frame); > + struct type *type = builtin_type (gdbarch)->builtin_data_ptr; > struct value *value; > CORE_ADDR result; > > - value = value_from_register (type, regnum, frame); > - gdb_assert (value); > + /* This routine may be called during early unwinding, at a time > + where the ID of FRAME is not yet known. Calling value_from_register > + would therefore abort in get_frame_id. However, since we only need > + a temporary value that is never used as lvalue, we actually do not > + really need to set its VALUE_FRAME_ID. Therefore, we re-implement > + the core of value_from_register, but use the null_frame_id. > + > + This works only if we do not require a special conversion routine, > + which is true for plain pointer types for all current targets. */ > + gdb_assert (!gdbarch_convert_register_p (gdbarch, regnum, type)); > + > + value = gdbarch_value_from_register (gdbarch, type, regnum, null_frame_id); > + read_frame_register_value (value, frame); > > if (value_optimized_out (value)) > { > Index: binutils-gdb/gdb/gdbarch.c > =================================================================== > --- binutils-gdb.orig/gdb/gdbarch.c > +++ binutils-gdb/gdb/gdbarch.c > @@ -2381,13 +2381,13 @@ set_gdbarch_value_to_register (struct gd > } > > struct value * > -gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_info *frame) > +gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id) > { > gdb_assert (gdbarch != NULL); > gdb_assert (gdbarch->value_from_register != NULL); > if (gdbarch_debug >= 2) > fprintf_unfiltered (gdb_stdlog, "gdbarch_value_from_register called\n"); > - return gdbarch->value_from_register (type, regnum, frame); > + return gdbarch->value_from_register (gdbarch, type, regnum, frame_id); > } > > void > Index: binutils-gdb/gdb/gdbarch.h > =================================================================== > --- binutils-gdb.orig/gdb/gdbarch.h > +++ binutils-gdb/gdb/gdbarch.h > @@ -422,12 +422,12 @@ extern void gdbarch_value_to_register (s > extern void set_gdbarch_value_to_register (struct gdbarch *gdbarch, gdbarch_value_to_register_ftype *value_to_register); > > /* Construct a value representing the contents of register REGNUM in > - frame FRAME, interpreted as type TYPE. The routine needs to > + frame FRAME_ID, interpreted as type TYPE. The routine needs to > allocate and return a struct value with all value attributes > (but not the value contents) filled in. */ > > -typedef struct value * (gdbarch_value_from_register_ftype) (struct type *type, int regnum, struct frame_info *frame); > -extern struct value * gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_info *frame); > +typedef struct value * (gdbarch_value_from_register_ftype) (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id); > +extern struct value * gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id); > extern void set_gdbarch_value_from_register (struct gdbarch *gdbarch, gdbarch_value_from_register_ftype *value_from_register); > > typedef CORE_ADDR (gdbarch_pointer_to_address_ftype) (struct gdbarch *gdbarch, struct type *type, const gdb_byte *buf); > Index: binutils-gdb/gdb/gdbarch.sh > =================================================================== > --- binutils-gdb.orig/gdb/gdbarch.sh > +++ binutils-gdb/gdb/gdbarch.sh > @@ -500,10 +500,10 @@ m:int:convert_register_p:int regnum, str > f:int:register_to_value:struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep:frame, regnum, type, buf, optimizedp, unavailablep:0 > f:void:value_to_register:struct frame_info *frame, int regnum, struct type *type, const gdb_byte *buf:frame, regnum, type, buf:0 > # Construct a value representing the contents of register REGNUM in > -# frame FRAME, interpreted as type TYPE. The routine needs to > +# frame FRAME_ID, interpreted as type TYPE. The routine needs to > # allocate and return a struct value with all value attributes > # (but not the value contents) filled in. > -f:struct value *:value_from_register:struct type *type, int regnum, struct frame_info *frame:type, regnum, frame::default_value_from_register::0 > +m:struct value *:value_from_register:struct type *type, int regnum, struct frame_id frame_id:type, regnum, frame_id::default_value_from_register::0 > # > m:CORE_ADDR:pointer_to_address:struct type *type, const gdb_byte *buf:type, buf::unsigned_pointer_to_address::0 > m:void:address_to_pointer:struct type *type, gdb_byte *buf, CORE_ADDR addr:type, buf, addr::unsigned_address_to_pointer::0 > Index: binutils-gdb/gdb/s390-linux-tdep.c > =================================================================== > --- binutils-gdb.orig/gdb/s390-linux-tdep.c > +++ binutils-gdb/gdb/s390-linux-tdep.c > @@ -380,11 +380,11 @@ s390_pseudo_register_write (struct gdbar > registers, even though we are otherwise a big-endian platform. */ > > static struct value * > -s390_value_from_register (struct type *type, int regnum, > - struct frame_info *frame) > +s390_value_from_register (struct gdbarch *gdbarch, struct type *type, > + int regnum, struct frame_id frame_id) > { > - struct value *value = default_value_from_register (type, regnum, frame); > - > + struct value *value = default_value_from_register (gdbarch, type, > + regnum, frame_id); > check_typedef (type); > > if (regnum >= S390_F0_REGNUM && regnum <= S390_F15_REGNUM > Index: binutils-gdb/gdb/spu-tdep.c > =================================================================== > --- binutils-gdb.orig/gdb/spu-tdep.c > +++ binutils-gdb/gdb/spu-tdep.c > @@ -314,10 +314,11 @@ spu_pseudo_register_write (struct gdbarc > /* Value conversion -- access scalar values at the preferred slot. */ > > static struct value * > -spu_value_from_register (struct type *type, int regnum, > - struct frame_info *frame) > +spu_value_from_register (struct gdbarch *gdbarch, struct type *type, > + int regnum, struct frame_id frame_id) > { > - struct value *value = default_value_from_register (type, regnum, frame); > + struct value *value = default_value_from_register (gdbarch, type, > + regnum, frame_id); > int len = TYPE_LENGTH (type); > > if (regnum < SPU_NUM_GPRS && len < 16) > Index: binutils-gdb/gdb/value.h > =================================================================== > --- binutils-gdb.orig/gdb/value.h > +++ binutils-gdb/gdb/value.h > @@ -579,9 +579,10 @@ extern struct value *value_from_contents > CORE_ADDR); > extern struct value *value_from_contents (struct type *, const gdb_byte *); > > -extern struct value *default_value_from_register (struct type *type, > +extern struct value *default_value_from_register (struct gdbarch *gdbarch, > + struct type *type, > int regnum, > - struct frame_info *frame); > + struct frame_id frame_id); > > extern void read_frame_register_value (struct value *value, > struct frame_info *frame); > @@ -589,7 +590,7 @@ extern void read_frame_register_value (s > extern struct value *value_from_register (struct type *type, int regnum, > struct frame_info *frame); > > -extern CORE_ADDR address_from_register (struct type *type, int regnum, > +extern CORE_ADDR address_from_register (int regnum, > struct frame_info *frame); > > extern struct value *value_of_variable (struct symbol *var, > -- > Dr. Ulrich Weigand > GNU/Linux compilers and toolchain > Ulrich.Weigand@de.ibm.com >