From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7127 invoked by alias); 8 Feb 2007 15:59:34 -0000 Received: (qmail 7113 invoked by uid 22791); 8 Feb 2007 15:59:33 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Thu, 08 Feb 2007 15:59:26 +0000 Received: from drow by nevyn.them.org with local (Exim 4.63) (envelope-from ) id 1HFBCi-0002cH-5u; Thu, 08 Feb 2007 10:28:56 -0500 Date: Thu, 08 Feb 2007 15:59:00 -0000 From: Daniel Jacobowitz To: Vladimir Prus Cc: gdb-patches@sources.redhat.com Subject: Re: Lazy bitfields Message-ID: <20070208152856.GG6861@nevyn.them.org> Mail-Followup-To: Vladimir Prus , gdb-patches@sources.redhat.com References: <200701241123.43649.vladimir@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200701241123.43649.vladimir@codesourcery.com> User-Agent: Mutt/1.5.13 (2006-08-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: 2007-02/txt/msg00080.txt.bz2 On Wed, Jan 24, 2007 at 11:23:43AM +0300, Vladimir Prus wrote: > One nit is that when writing a bitfield, we first fetch the entire > parent value, modify bits and write entire parent value. In some > *limited* set of cases it is possible to read only some of the bytes, > or not read anything at all, but introducing bit-mask telling which > bits are read and which are not is more trouble that its worth. The other nit is that when we read a bitfield, we read the whole struct, which might be much larger. I don't think either of these nits are problematic. Your approach seems fine. > +extern struct value *value_parent (struct value *value) > +{ > + return value->parent; > +} > + > +extern int value_fieldno (struct value *value) > +{ > + return value->fieldno; > +} > +extern void value_free (struct value *value) I haven't gotten to complain about your formatting in a while :-) Function at start of line please. > + /* If there's a parent, drop parents's reference count "parent's" > @@ -562,6 +601,11 @@ value_copy (struct value *arg) > val->embedded_offset = value_embedded_offset (arg); > val->pointed_to_offset = arg->pointed_to_offset; > val->modifiable = arg->modifiable; > + val->fieldno = arg->fieldno; > + val->parent = arg->parent; > + if (val->parent) > + ++val->parent->reference_count; > + val->reference_count = 1; Don't think you need the last line - allocate_value should have set it to one. > @@ -1300,15 +1344,23 @@ value_primitive_field (struct value *arg > > if (TYPE_FIELD_BITSIZE (arg_type, fieldno)) > { > - v = value_from_longest (type, > - unpack_field_as_long (arg_type, > - value_contents (arg1) > - + offset, > - fieldno)); > + LONGEST num; > + int len; > + > + > + v = allocate_value (type); > v->bitpos = TYPE_FIELD_BITPOS (arg_type, fieldno) % 8; > v->bitsize = TYPE_FIELD_BITSIZE (arg_type, fieldno); > v->offset = value_offset (arg1) + offset > + TYPE_FIELD_BITPOS (arg_type, fieldno) / 8; > + v->parent = arg1; > + v->lval = lval_bitfield; > + v->fieldno = fieldno; > + > + if (!value_lazy (arg1)) > + value_fetch_lazy (v); > + else > + set_value_lazy (v, 1); > } > else if (fieldno < TYPE_N_BASECLASSES (arg_type)) > { Two problems here. First of all, the whole point of adding reference counts is to increment the parent's reference count here, right? Better do that then :-) Second, I'm worried about the "offset" argument to value_primitive_field. This was primarily for CHILL, due to a strange choice when generating or reading debug info, and it's possible that it's never non-zero any more, which would be nice. But if it ever is non-zero, this will break it. It gets lumped in with value_offset so when you call unpack_field_as_long in value_fetch_lazy you can't pick it out again to apply it to the parent. That's my only real concern with this patch; everything else looks OK. I'm going to try to figure out if that argument is ever non-zero or if we can eliminate it now. -- Daniel Jacobowitz CodeSourcery