From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5639 invoked by alias); 11 Jun 2014 12:29:39 -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 5556 invoked by uid 89); 11 Jun 2014 12:29:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mga03.intel.com Received: from mga03.intel.com (HELO mga03.intel.com) (143.182.124.21) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 11 Jun 2014 12:29:37 +0000 Received: from azsmga001.ch.intel.com ([10.2.17.19]) by azsmga101.ch.intel.com with ESMTP; 11 Jun 2014 05:29:35 -0700 X-ExtLoop1: 1 Received: from kboell-mobl2.ger.corp.intel.com (HELO [172.28.205.55]) ([172.28.205.55]) by azsmga001.ch.intel.com with ESMTP; 11 Jun 2014 05:29:33 -0700 Message-ID: <53984BAC.5020206@linux.intel.com> Date: Wed, 11 Jun 2014 12:29:00 -0000 From: Keven Boell User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Joel Brobecker , 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 References: <1401861266-6240-1-git-send-email-keven.boell@intel.com> <1401861266-6240-3-git-send-email-keven.boell@intel.com> <20140610121014.GA6480@adacore.com> In-Reply-To: <20140610121014.GA6480@adacore.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-06/txt/msg00445.txt.bz2 Hi Joel, I've addressed your comments and the changes will be part of patch series v2. Keven > 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; >> + } Good catch, there is no reason to limit the processing here. > >> --- 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 I've updated the comment. > > >> + 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. I did this for improving the readability, however maybe this is just confusing. I changed it. > >> @@ -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. Done. > >> +/* 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. Done. > >> +#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 >