From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30030 invoked by alias); 17 Apr 2008 15:51:41 -0000 Received: (qmail 30019 invoked by uid 22791); 17 Apr 2008 15:51:40 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 17 Apr 2008 15:51:20 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 69373983D6; Thu, 17 Apr 2008 15:51:19 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id 504D9982C4; Thu, 17 Apr 2008 15:51:19 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1JmWOM-0005sF-2S; Thu, 17 Apr 2008 11:51:18 -0400 Date: Thu, 17 Apr 2008 16:02:00 -0000 From: Daniel Jacobowitz To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org Subject: Re: [RFC 2/5] Frame unwinding using struct value Message-ID: <20080417155118.GC17488@caradoc.them.org> Mail-Followup-To: Thiago Jung Bauermann , gdb-patches@sourceware.org References: <20080331221024.GA22334@caradoc.them.org> <1207576706.29533.244.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1207576706.29533.244.camel@localhost.localdomain> User-Agent: Mutt/1.5.17 (2007-12-11) X-IsSubscribed: yes 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 X-SW-Source: 2008-04/txt/msg00340.txt.bz2 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