This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC 2/5] Frame unwinding using struct value


On Mon, Apr 07, 2008 at 10:58:26AM -0300, Thiago Jung Bauermann wrote:
> My two cents.

Much appreciated.

> > +      /* Offsets are not supported here; lazy register values must
> > +	 refer to the entire register.  */
> > +      gdb_assert (value_offset (val) == 0);
> 
> Is this a limitation of the current implementation or is there an
> underlying reason for it?

You could call it either one.  I didn't add support for lazy registers
containing offsets because I don't think it's useful; lazy registers
are currently aimed to support unwinding, not to generally suppress
target fetches the way lazy memory values do.  And it keeps things
simpler if we know we've got the whole register.  So I've disallowed
it, and that way it will be clear where to start from if we want this
later.

> The conditions in the while loop above fit in one line.

So it does.

> > +	  frame = frame_find_by_id (VALUE_FRAME_ID (new_val));
> 
> I thought it was strange that VALUE_FRAME_ID expands to a deprecated
> function, since it plays such an important role in the new world
> order... Do you know why it is deprecated?

It's deprecated because it's writable.  If it returned a read-only
copy of the frame ID, then there'd be no need to deprecate it; this
was added when struct value was moved from value.h to value.c.
Things like:

findvar.c:  VALUE_FRAME_ID (reg_val) = get_frame_id (frame);

Perhaps we need some new constructors.

> > +struct value *
> > +frame_unwind_got_address (struct frame_info *frame, int regnum,
> > +			  CORE_ADDR addr)
> > +{
> > +  struct gdbarch *gdbarch = get_frame_arch (frame);
> > +  struct value *reg_val;
> > +
> > +  reg_val = value_zero (register_type (gdbarch, regnum), not_lval);
> > +  pack_long (value_contents_writeable (reg_val),
> > +	     register_type (gdbarch, regnum), addr);
> 
> This code assumes the register type is TYPE_CODE_PTR, right?
> Does it make sense to put an assertion here?

No, I don't think so.  It will work OK for integers, too, and the
assertion could be triggered by bad debug information.  I don't like
to add asserts where a corrupt file could trigger them, unless the
alternative is crashing.

> s/the this/this/

Fixed.

> > +The first time a sniffer returns non-zero, the corresponding unwinder
> > +is assigned to the frame.
> 
> What about changing the last sentence to "The first sniffer returning
> non-zero will have its corresponding unwinder assigned to the frame."?

I don't think either is particularly clearer.

> This looks like a good place to explain the motivation behind frame IDs.
> What about the following?

Thanks.  I changed the wording a little, but basically used this.

> Is this a good place to list the other reasons to stop frame unwinding?

I'm not sure, so I've left it out for now.

-- 
Daniel Jacobowitz
CodeSourcery


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]