From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1428 invoked by alias); 19 Nov 2013 15:14:26 -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 1398 invoked by uid 89); 19 Nov 2013 15:14:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mga11.intel.com Received: from Unknown (HELO mga11.intel.com) (192.55.52.93) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Nov 2013 15:14:22 +0000 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 19 Nov 2013 07:14:14 -0800 X-ExtLoop1: 1 Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by fmsmga002.fm.intel.com with ESMTP; 19 Nov 2013 07:12:23 -0800 Received: from irsmsx151.ger.corp.intel.com (163.33.192.59) by IRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP Server (TLS) id 14.3.123.3; Tue, 19 Nov 2013 15:11:33 +0000 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.215]) by IRSMSX151.ger.corp.intel.com ([169.254.4.126]) with mapi id 14.03.0123.003; Tue, 19 Nov 2013 15:11:33 +0000 From: "Agovic, Sanimir" To: "Tom Tromey (tromey@redhat.com)" CC: "gdb-patches@sourceware.org" Subject: RE: [PATCH 02/10] type: add c99 variable length array support Date: Tue, 19 Nov 2013 15:31:00 -0000 Message-ID: <0377C58828D86C4588AEEC42FC3B85A7176B7861@IRSMSX105.ger.corp.intel.com> References: <1382366424-21010-1-git-send-email-sanimir.agovic@intel.com> <1382366424-21010-3-git-send-email-sanimir.agovic@intel.com> <87eh6sujea.fsf@fleche.redhat.com> In-Reply-To: <87eh6sujea.fsf@fleche.redhat.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg00542.txt.bz2 Thanks for your review. > Sanimir> + switch (ctx->location) > Sanimir> + { > Sanimir> + case DWARF_VALUE_REGISTER: > Sanimir> + *valp =3D dwarf_expr_read_reg (&baton, dwarf_expr_fetch_a= ddress (ctx, 0)); > Sanimir> + break; > Sanimir> + case DWARF_VALUE_MEMORY: > Sanimir> + *valp =3D dwarf_expr_fetch_address (ctx, 0); > Sanimir> + break; > Sanimir> + } >=20 > It seems that something should be done for other DWARF_VALUE_* results > here. > I will try, please have a closer look at this hunk in the upcoming v2. > Sanimir> +struct type * > Sanimir> +resolve_dynamic_type (struct type *type, CORE_ADDR address) > Sanimir> +{ > [...] > Sanimir> + if (!TYPE_OBJFILE_OWNED (ty)) > Sanimir> + return type; >=20 > This seems like a bit of a wart, though I am not sure whether the > situation can actually arise. >=20 >=20 > One thing I didn't see in here is error-checking of whether resolution > makes sense. >=20 > E.g., suppose I print the value of a pointer-to-VLA. Then I move to > some other frame and "print *$". >=20 1| void foo (size_t n) { 2| int vla[n]; 3| } (gdb) print &vla $1 =3D (int (*)[42]) 0xffffffffffff (gdb) up (gdb) print *$ $2 =3D {0 } (gdb) down (gdb) print *$ $3 =3D {0 } > In this situation the bounds have not been resolved -- but applying the > DWARF expression in the currently-selected frame will silently do the > wrong thing. > The bounds will be resolved in this case and therefore it is save to change the frame+re-evaluate. If you have a different example in mind please tell me and I see how to deal with it. > I wonder if this series also needs to handle DW_AT_count. > Maybe no compiler generates it. >=20 I will add a separate patch which implements DW_AT_count, I could not test it as no compiler emits this attribute so far. Below you will find the trivial reviews. -Sanimir > -----Original Message----- > From: Tom Tromey [mailto:tromey@redhat.com] > Sent: Thursday, November 07, 2013 08:00 PM > To: Agovic, Sanimir > Cc: gdb-patches@sourceware.org > Subject: Re: [PATCH 02/10] type: add c99 variable length array support >=20 > >>>>> "Sanimir" =3D=3D Sanimir Agovic writes: >=20 > Sanimir> The dwarf standard allow certain attributes to be expressed as > Sanimir> dwarf expressions rather than constants. For instance > Sanimir> upper-/lowerbound attributes. In case of a c99 variable length > Sanimir> array the upperbound is a dynamic attribute. >=20 >=20 > Sanimir> +int > Sanimir> +dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *d= lbaton, > Sanimir> + CORE_ADDR addr, CORE_ADDR *valp) >=20 > Need an introductory comment. It can just say "See dwarf2loc.h.", since > you put the real comment there. >=20 Done. > Sanimir> +static struct dwarf2_locexpr_baton* attr_to_locexprbaton > Sanimir> +(const struct attribute *, struct dwarf2_cu *); > Sanimir> + > Sanimir> +static struct dwarf2_locexpr_baton* attr_to_locexprbaton_1 > Sanimir> +(const struct attribute *, struct dwarf2_cu *, const gdb_byte *= additional_data, > Sanimir> + int extra_size); > Sanimir> + > Sanimir> +static int attr_to_dwarf2_prop > Sanimir> +(struct die_info *, const struct attribute *, struct dwarf2_cu = *, > Sanimir> + struct dwarf2_prop *); >=20 > In cases like this we usually indent the subsequent lines a bit, like: >=20 > static int attr_to_dwarf2_prop > (struct die_info *, const struct attribute *, struct dwarf2_cu *, > struct dwarf2_prop *); >=20 > However in this case I think it may be preferable to rearrange the > functions so that forward declarations are not needed. What do you > think? >=20 Done. Moved functions and dropped forward declaration. > Sanimir> +static struct dwarf2_locexpr_baton* > Sanimir> +attr_to_locexprbaton (const struct attribute *attribute, struct= dwarf2_cu *cu) > Sanimir> +{ > Sanimir> + return attr_to_locexprbaton_1 (attribute, cu, NULL, 0); > Sanimir> +} >=20 > If there is just a single caller (there is in this patch, but I haven't > read all the patches yet), I would remove this function and just update > the caller. >=20 Done. Folded function attr_to_locexprbaton/_1 into attr_to_locexprbaton. > Sanimir> +static struct dwarf2_locexpr_baton* > Sanimir> +attr_to_locexprbaton_1 (const struct attribute *attribute, stru= ct dwarf2_cu *cu, > Sanimir> + const gdb_byte *additional_data, int extra_size) >=20 > Needs an introductory comment. >=20 Done. > Sanimir> + /* Copy the data pointer as the blocks lifetime is >=20 > Missing apostrophe: "block's". >=20 Done. > Sanimir> + gdb_assert(baton->data !=3D NULL); >=20 > Space before open paren. >=20 Done. > Sanimir> +/* Parse dwarf attribute if it's a block, reference or constant= and put the > Sanimir> + resulting value of the attribute into struct dwarf2_prop. */ > Sanimir> + > Sanimir> +static int > Sanimir> +attr_to_dwarf2_prop (struct die_info *die, const struct attribu= te *attr, > Sanimir> + struct dwarf2_cu *cu, > Sanimir> + struct dwarf2_prop *prop) >=20 > I think it would be good if the introductory comment describe the return > value. >=20 Done. > Sanimir> + else if (attr_form_is_ref (attr)) > Sanimir> + { > Sanimir> + struct dwarf2_cu *target_cu =3D cu; > Sanimir> + struct die_info *target_die; > Sanimir> + struct attribute *target_attr; > Sanimir> + const gdb_byte append_ops[] =3D { DW_OP_deref }; > Sanimir> + > Sanimir> + target_die =3D follow_die_ref (die, attr, &target_cu); > Sanimir> + target_attr =3D dwarf2_attr (target_die, DW_AT_location, = target_cu); > Sanimir> + > Sanimir> + prop->data.locexpr =3D > Sanimir> + attr_to_locexprbaton_1 (target_attr, cu, append_ops, > Sanimir> + sizeof (append_ops) / sizeof (append_ops[0])); > Sanimir> + prop->kind =3D DWARF_LOCEXPR; > Sanimir> + gdb_assert (prop->data.locexpr !=3D NULL); >=20 > I don't understand this hunk. Could you say why it is needed? >=20 DW_AT_upper_bound may reference a die e.g. artificial variable, in this cas= e we need to read DW_AT_location of the target die. Some compilers emit this for= m to implement vla.=20 > Sanimir> + dwarf2_invalid_attrib_class_complaint(dwarf_form_name (at= tr->form), > Sanimir> + dwarf2_name (die, cu)); >=20 > Missing space before a paren. >=20 Done. > Sanimir> +static int > Sanimir> +has_static_range (const struct range_bounds *bounds) > Sanimir> +{ > Sanimir> + return bounds->low.kind =3D=3D DWARF_CONST > Sanimir> + && bounds->high.kind =3D=3D DWARF_CONST; > Sanimir> +} >=20 > THis needs parens around the argument to "return" and then an > indentation fix on the second line. >=20 Done. > Sanimir> +/* Calculates the size of a type given the upper and lower boun= d of a dynamic > Sanimir> + type. */ > Sanimir> + > Sanimir> +static ULONGEST > Sanimir> +get_type_length (const struct type *type) > Sanimir> +{ > Sanimir> + const struct type *range_type, *target_type; > Sanimir> + ULONGEST len =3D TYPE_LENGTH (type); > Sanimir> + LONGEST low_bound, high_bound; > Sanimir> + > Sanimir> + if (TYPE_CODE (type) !=3D TYPE_CODE_ARRAY > Sanimir> + && TYPE_CODE (type) !=3D TYPE_CODE_STRING) > Sanimir> + return len; > Sanimir> + > Sanimir> + range_type =3D TYPE_INDEX_TYPE (type); > Sanimir> + > Sanimir> + if (!has_static_range (TYPE_RANGE_DATA (range_type))) > Sanimir> + return len; >=20 > This seems like it doesn't follow what the introductory comment says it > does. >=20 Fixed document. > Sanimir> + > Sanimir> +static void > Sanimir> +resolve_dynamic_bounds (struct type *type, CORE_ADDR address) >=20 > Introductory comment. >=20 Done. > Sanimir> + do { > Sanimir> + struct type *range_type =3D TYPE_INDEX_TYPE (ary_dim); >=20 > It's hard to know but perhaps a check_typedef is required here. >=20 > Sanimir> + ary_dim =3D TYPE_TARGET_TYPE (ary_dim); >=20 > Here too. >=20 Done. Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052