From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19984 invoked by alias); 21 Dec 2011 19:36:56 -0000 Received: (qmail 19976 invoked by uid 22791); 21 Dec 2011 19:36:54 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_XG X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 21 Dec 2011 19:36:38 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pBLJabIL009714 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 21 Dec 2011 14:36:37 -0500 Received: from host2.jankratochvil.net (ovpn-116-60.ams2.redhat.com [10.36.116.60]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id pBLJaV1c030043 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 21 Dec 2011 14:36:34 -0500 Date: Wed, 21 Dec 2011 19:37:00 -0000 From: Jan Kratochvil To: xgsa Cc: Tom Tromey , gdb-patches@sourceware.org Subject: Re: set print object on should affect MI varobjs (PR gdb/13393) Message-ID: <20111221193630.GA28985@host2.jankratochvil.net> References: <4ED92C05.9080803@yandex.ru> <4EF22A4D.80703@yandex.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4EF22A4D.80703@yandex.ru> User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2011-12/txt/msg00742.txt.bz2 On Wed, 21 Dec 2011 19:49:49 +0100, xgsa wrote: > It seems that none of neighboring functions (e.g. value_type() > & value_enclosing_type()) has such comment. I think it is better to be > consistent and not to add the comment, but if you insist I can add it. Yes, the GDB codebase is in a worse state than what is accepted for new patches. I do not see there bugs, the comments below are just formatting fixes. > 2011-12-02 Anton Gorenkov > > PR gdb/13393 This should be mi/13393. > * gdb/valops.c (value_rtti_target_type): add support for references. Sentence starts with capital 'A'. > Return also a reference or pointer type (because every caller do it after call that leads to code duplication) All the lines must be <= 80 characters (some people say 72 characters). The same applies to all the code lines, they must be <= 80 characters. Every sentece is terminated by '.'. > * gdb/c-valprint.c (c_value_print): updated for value_rtti_target_type() change. GNU does not use () at the function names. > * gdb/eval.c (evaluate_subexp_standard): updated for value_rtti_target_type() change. > * gdb/typeprint.c: updated for value_rtti_target_type() change. > * gdb/value.c(value_actual_type): new function. Space before '('. > (coerce_ref): support for enclosing type setting for references (as it is done for pointers in value_ind()) > * gdb/value.h(value_actual_type): add prototype. > * gdb/varobj.c(varobj_create): call value_actual_type() if necessary > (create_child_with_value): call value_actual_type(). > (value_of_root): support for type change if the value changed and RTTI is used to determine type. > (adjust_value_for_child_access): extended with a new parameter and cast given value to enclosing type is necessary. > (c_number_of_children): update for extended adjust_value_for_child_access() > (cplus_number_of_children): send a value as parameter if RTTI should be used to determine type > (cplus_describe_child): determine whether RTTI type should be used > > gdb/testsuite/ChangeLog: > > 2011-12-02 Anton Gorenkov > > PR gdb/13393 Likewise. > * gdb.mi/mi-var-rtti.cc:: New file. > * gdb.mi/mi-var-rtti.exp:: New file. Single ':'. > --- /dev/null > +++ b/gdb/testsuite/gdb.mi/mi-var-rtti.exp > @@ -0,0 +1,108 @@ > +# Copyright 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc. This is a new test (even if it is partially copied), 2011 is enough there. > --- a/gdb/valops.c > +++ b/gdb/valops.c > @@ -3528,8 +3528,7 @@ value_maybe_namespace_elt (const struct type *curtype, > return result; > } > > -/* Given a pointer value V, find the real (RTTI) type of the object it > - points to. > +/* Given a pointer or a reference value V, find its real (RTTI) type. > > Other parameters FULL, TOP, USING_ENC as with value_rtti_type() > and refer to the values computed for the object pointed to. */ > @@ -3539,12 +3538,37 @@ value_rtti_target_type (struct value *v, int *full, You are changing the behavior of this function wrt. indirection. I would find best to rename it, this way you ensure all the callers are reviewed, incl. possible callers in 3rd party patches which are common for GDB. > int *top, int *using_enc) > { > struct value *target; > + struct type *type, *real_type; > > - target = value_ind (v); > + type = value_type (v); > + type = check_typedef (type); > + if (TYPE_CODE (type) == TYPE_CODE_REF) > + target = coerce_ref (v); > + else if (TYPE_CODE (type) == TYPE_CODE_PTR) > + target = value_ind (v); > + else > + return 0; return NULL, it is a pointer. > > - return value_rtti_type (target, full, top, using_enc); > + real_type = value_rtti_type (target, full, top, using_enc); > + > + if (real_type) > + { > + struct type *target_type = value_type (target); > + > + /* Copy qualifiers to the referenced object. */ > + real_type = make_cv_type (TYPE_CONST (target_type), TYPE_VOLATILE (target_type), real_type, NULL); > + if (TYPE_CODE (type) == TYPE_CODE_REF) > + real_type = lookup_reference_type (real_type); > + else if (TYPE_CODE (type) == TYPE_CODE_PTR) > + real_type = lookup_pointer_type (real_type); Empty line before comment. > + /* Copy qualifiers to the pointer/reference. */ > + real_type = make_cv_type (TYPE_CONST (type), TYPE_VOLATILE (type), real_type, NULL); > + } > + > + return real_type; > } > > + > /* Given a value pointed to by ARGP, check its real run-time type, and > if that is different from the enclosing type, create a new value > using the real run-time type as the enclosing type (and of the same > diff --git a/gdb/value.c b/gdb/value.c > index d263d0c..7d9e5cc 100644 > --- a/gdb/value.c > +++ b/gdb/value.c > @@ -821,6 +821,33 @@ value_enclosing_type (struct value *value) > return value->enclosing_type; > } > > +struct type * > +value_actual_type (struct value *value, int resolve_simple_types) > +{ > + struct value_print_options opts; > + struct value *target; > + struct type *result; > + > + get_user_print_options (&opts); > + > + result = value_type (value); > + if (opts.objectprint) > + { > + if ((TYPE_CODE (result) == TYPE_CODE_PTR) || (TYPE_CODE (result) == TYPE_CODE_REF)) Those inner parens are excessive. if (TYPE_CODE (result) == TYPE_CODE_PTR || TYPE_CODE (result) == TYPE_CODE_REF) > + { > + struct type *real_type; > + > + real_type = value_rtti_target_type (value, 0, 0, 0); > + if (real_type) > + result = real_type; > + } > + else if (resolve_simple_types) > + result = value_enclosing_type (value); > + } > + > + return result; > +} > + > static void > require_not_optimized_out (const struct value *value) > { > @@ -3114,6 +3141,7 @@ coerce_ref (struct value *arg) > { > struct type *value_type_arg_tmp = check_typedef (value_type (arg)); > struct value *retval; > + struct type *enc_type; > > retval = coerce_ref_if_computed (arg); > if (retval) > @@ -3122,9 +3150,23 @@ coerce_ref (struct value *arg) > if (TYPE_CODE (value_type_arg_tmp) != TYPE_CODE_REF) > return arg; > > - return value_at_lazy (TYPE_TARGET_TYPE (value_type_arg_tmp), > + enc_type = check_typedef (value_enclosing_type (arg)); > + enc_type = TYPE_TARGET_TYPE (enc_type); > + > + retval = value_at_lazy (enc_type, > unpack_pointer (value_type (arg), > value_contents (arg))); > + /* Re-adjust type. */ > + deprecated_set_value_type (retval, TYPE_TARGET_TYPE (value_type_arg_tmp)); > + > + /* Add embedding info. */ > + set_value_enclosing_type (retval, enc_type); > + set_value_embedded_offset (retval, value_pointed_to_offset (arg)); > + > + /* We may be pointing to an object of some derived type. */ > + retval = value_full_object (retval, NULL, 0, 0, 0); > + > + return retval; > } Please put this code into some function called also by value_ind, instead of just copy-pasting it. > > struct value * > diff --git a/gdb/value.h b/gdb/value.h > index d2c58ec..c01da3e 100644 > --- a/gdb/value.h > +++ b/gdb/value.h > @@ -139,6 +139,15 @@ extern struct type *value_enclosing_type (struct value *); > extern void set_value_enclosing_type (struct value *val, > struct type *new_type); > > +/* Returns value_type or value_enclosing_type depending on > + value_print_options.objectprint. > + > + If RESOLVE_SIMPLE_TYPES is 0 the enclosing type will be resolved > + only for pointers and references, else it will be returned > + for all the types (e.g. structures). This option is useful > + to prevent retrieving enclosing type for the base classes fields. */ > +extern struct type *value_actual_type (struct value *value, int resolve_simple_types); > + > extern int value_pointed_to_offset (struct value *value); > extern void set_value_pointed_to_offset (struct value *value, int val); > extern int value_embedded_offset (struct value *value); > diff --git a/gdb/varobj.c b/gdb/varobj.c > index 7c68a93..47390cb 100644 > --- a/gdb/varobj.c > +++ b/gdb/varobj.c > @@ -660,7 +660,17 @@ varobj_create (char *objname, > var->type = value_type (type_only_value); > } > else > - var->type = value_type (value); > + { Here is some whitespace corruption, there should have been tab before var->type. > @@ -3259,7 +3301,14 @@ cplus_number_of_children (struct varobj *var) > int kids[3]; > > type = get_value_type (var->parent); > - adjust_value_for_child_access (NULL, &type, NULL); Empty line before a comment line. > + /* It is necessary to access a real type (via RTTI). */ > + if (opts.objectprint) > + { > + value = var->parent->value; > + lookup_actual_type = (TYPE_CODE (var->parent->type) == TYPE_CODE_REF > + || TYPE_CODE (var->parent->type) == TYPE_CODE_PTR); > + } > + adjust_value_for_child_access (&value, &type, NULL, lookup_actual_type); > > cplus_class_num_children (type, kids); > if (strcmp (var->name, "public") == 0) Thanks, Jan