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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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