Re: set print object on should affect MI varobjs (PR gdb/13393)

Jan Kratochvil
2011-12-21T19:36:56Z

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 <xgsa@yandex.ru>
>
> 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 <xgsa@yandex.ru>
>
> 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