From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29311 invoked by alias); 10 Jun 2014 12:10:23 -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 29299 invoked by uid 89); 10 Jun 2014 12:10:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 10 Jun 2014 12:10:20 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5047F116165; Tue, 10 Jun 2014 08:10:17 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 2wyaGOYikR-g; Tue, 10 Jun 2014 08:10:17 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id E140F116163; Tue, 10 Jun 2014 08:10:16 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 9CFF640E7F; Tue, 10 Jun 2014 14:10:14 +0200 (CEST) Date: Tue, 10 Jun 2014 12:10:00 -0000 From: Joel Brobecker To: Keven Boell Cc: gdb-patches@sourceware.org, sanimir.agovic@intel.com, Tom Tromey Subject: Re: [PATCH 02/23] dwarf: add DW_AT_data_location support Message-ID: <20140610121014.GA6480@adacore.com> References: <1401861266-6240-1-git-send-email-keven.boell@intel.com> <1401861266-6240-3-git-send-email-keven.boell@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1401861266-6240-3-git-send-email-keven.boell@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-06/txt/msg00416.txt.bz2 Hello, > An object might have a descriptor proceeding the actual value. > To point the debugger to the actually value of an object > DW_AT_data_location is used for. For example the compile may > emit for this entity: > > 1| int foo[N]; > > the following descriptor: > > struct array { > size_t size; > void* data; // DW_AT_data_location describes this location > } > > This allows GDB to print the actual data of an type. > > 2014-05-28 Sanimir Agovic > Keven Boell > > * dwarf2read.c (set_die_type): Parse and save DW_AT_data_location > attribute. > * gdbtypes.c (is_dynamic_type): Consider a type being dynamic if > the data location has not yet been resolved. > (resolve_dynamic_type): Evaluate data location baton > if present and save its value. > * gdbtypes.h : Add data_location. > (TYPE_DATA_LOCATION): New macro. > (TYPE_DATA_LOCATION_ADDR): New macro. > (TYPE_DATA_LOCATION_IS_ADDRESS): New macro. > * value.c: Include dwarf2loc.h. > (value_fetch_lazy): Use data location addres to read value from > memory. > (coerce_ref): Construct new value from data location. Here are some comments and questions, but it would be nice, IMO, if the patch was also reviewed by Tom, particularly since it introduces a new field in struct main_type, which is a size-sensitive, I think. Also, it would be nice if you could include a copy of the actual DWARF output in the revision log of your patch, for easy reference of what you're trying to support. > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 6ebfffc..7a0f7f4 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -21499,6 +21499,7 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu) > { > struct dwarf2_per_cu_offset_and_type **slot, ofs; > struct objfile *objfile = cu->objfile; > + struct attribute *attr; > > /* For Ada types, make sure that the gnat-specific data is always > initialized (if not already set). There are a few types where > @@ -21513,6 +21514,20 @@ set_die_type (struct die_info *die, struct type *type, struct dwarf2_cu *cu) > && !HAVE_GNAT_AUX_INFO (type)) > INIT_GNAT_SPECIFIC (type); > > + /* Read DW_AT_data_location and set in type. */ > + attr = dwarf2_attr (die, DW_AT_data_location, cu); > + if (attr_form_is_block (attr)) Here (in set_die_type), why do you limit the processing the block-form data_location attributes? Why not just call attr_to_dynamic_prop regardless of the form, and let that function deal with whatever the form is? In other words, trop the form check and just keep the following bit: > + struct dynamic_prop prop; > + > + if (attr_to_dynamic_prop (attr, die, cu, &prop)) > + { > + TYPE_DATA_LOCATION (type) > + = obstack_alloc (&objfile->objfile_obstack, sizeof (prop)); > + *TYPE_DATA_LOCATION (type) = prop; > + } > --- a/gdb/gdbtypes.c > +++ b/gdb/gdbtypes.c > @@ -1635,8 +1635,12 @@ is_dynamic_type (struct type *type) > or the elements it contains have a dynamic contents. */ > if (is_dynamic_type (TYPE_INDEX_TYPE (type))) > return 1; > - else > - return is_dynamic_type (TYPE_TARGET_TYPE (type)); > + else if (TYPE_DATA_LOCATION (type) != NULL > + && (TYPE_DATA_LOCATION_KIND (type) == PROP_LOCEXPR > + || TYPE_DATA_LOCATION_KIND (type) == PROP_LOCLIST)) > + return 1; > + else > + return is_dynamic_type (TYPE_TARGET_TYPE (type)); The comment needs to be updated. Your change is also splitting the "if bounds or contents is dynamic" logic. Perhaps you could simply add the data-location check at the start of the function with a command that says: A type which has a data_location which [bla bla] is a dynamic type > + type = resolved_type; > + > + /* Resolve data_location attribute. */ > + prop = TYPE_DATA_LOCATION (type); > + if (dwarf2_evaluate_property (prop, addr, &value)) > + { > + TYPE_DATA_LOCATION_ADDR (type) = value; > + TYPE_DATA_LOCATION_KIND (type) = PROP_CONST; > + } > + else > + TYPE_DATA_LOCATION (type) = NULL; Here, I do not understand why you override TYPE, instead of just using RESOLVED_TYPE directly. > @@ -722,6 +722,10 @@ struct main_type > > struct func_type *func_stuff; > } type_specific; > + > + /* * Indirection to actual data. */ > + > + struct dynamic_prop *data_location; I'd like to have a comment which is a little more descriptive of what this field contains. Someone who reads this comment should be able to understand it without having to grep for this field to get an idea of how this field is used. > +/* Attribute accessors for VLA support. */ I think this comment is too specific. Although this field is instroduced to support VLAs, descriptors can probably be used in other contexts. I don't think you really need a comment, in this case, though. > +#define TYPE_DATA_LOCATION(thistype) \ > + TYPE_MAIN_TYPE(thistype)->data_location > +#define TYPE_DATA_LOCATION_BATON(thistype) \ > + TYPE_DATA_LOCATION (thistype)->data.baton > +#define TYPE_DATA_LOCATION_ADDR(thistype) \ > + TYPE_DATA_LOCATION (thistype)->data.const_val > +#define TYPE_DATA_LOCATION_KIND(thistype) \ > + TYPE_DATA_LOCATION (thistype)->kind > + > /* Moto-specific stuff for FORTRAN arrays. */ > > #define TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED(arraytype) \ > diff --git a/gdb/value.c b/gdb/value.c > index d125a09..1c88dfd 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -3635,8 +3635,14 @@ value_fetch_lazy (struct value *val) > } > else if (VALUE_LVAL (val) == lval_memory) > { > - CORE_ADDR addr = value_address (val); > struct type *type = check_typedef (value_enclosing_type (val)); > + CORE_ADDR addr; > + > + if (TYPE_DATA_LOCATION (type) != NULL > + && TYPE_DATA_LOCATION_KIND (type) == PROP_CONST) > + addr = TYPE_DATA_LOCATION_ADDR (type); > + else > + addr = value_address (val); I am wondering if this part shouldn't be in value_address itself. WDYT? Tom? > > if (TYPE_LENGTH (type)) > read_value_memory (val, 0, value_stack (val), > -- > 1.7.9.5 -- Joel