Re: [PATCH v2] Improve MI -var-info-path-expression for nested struct/union case.

[Andrew Burgess <aburgess at broadcom dot com>]

Ping!

Thanks,
Andrew

On 19/05/2014 10:33 PM, Andrew Burgess wrote:
> Volodya ,
> 
> Thanks for taking the time to look at this patch for me.
> 
> I've extended the test case to cover the extra case that you found.
> 
> In order to get this test passing I took onboard what you noticed, that the
> logic for is_path_expr_parent is really language specific, so I've made
> is_path_expr_parent a method on varobj_ops.  The current code is only ever
> called for C/C++ anyway, all the other languages do their own thing.  This
> allows me to move the existing is_path_expr_parent into c-varobj.c, and
> gives access to adjust_value_for_child_access, which fixes the issue.
> 
> The remaining additional changes are just fallout from adding the new
> varobj_ops method.
> 
> Is this ok to apply?
> 
> Thanks,
> 
> Andrew
> 
> ---
> The MI command -var-info-path-expression currently does not handle
> non-anonymous structs / unions nested within other structs / unions, it
> will skip parts of the expression.
> 
> Add a new method to the varobj_ops structure, is_path_expr_parent, to allow
> language specific control over finding the parent varobj, the current logic
> becomes the C/C++ version and is extended to handle the nested cases.
> 
> gdb/ChangeLog:
> 
> 	* ada-varobj.c (ada_varobj_ops): Fill in is_path_expr_parent
> 	field.
> 	* c-varobj.c (c_is_path_expr_parent): New function, moved core
> 	from varobj.c, with additional checks.
> 	(c_varobj_ops): Fill in is_path_expr_parent field.
> 	(cplus_varobj_ops): Fill in is_path_expr_parent field.
> 	* jv-varobj.c (java_varobj_ops): Fill in is_path_expr_parent
> 	field.
> 	* varobj.c (is_path_expr_parent): Call is_path_expr_parent varobj
> 	ops method.
> 	(varobj_default_is_path_expr_parent): New function.
> 	* varobj.h (lang_varobj_ops): Add is_path_expr_parent field.
> 	(varobj_default_is_path_expr_parent): Declare new function.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.mi/var-cmd.c (do_nested_struct_union_tests): New function
> 	setting up test structures.
> 	(main): Call new test function.
> 	* gdb.mi/mi2-var-child.exp: Create additional breakpoint in new
> 	test function, continue into test function and walk test
> 	structures.
> ---
>  gdb/ada-varobj.c                       |  3 +-
>  gdb/c-varobj.c                         | 56 ++++++++++++++++++++++++-
>  gdb/jv-varobj.c                        |  3 +-
>  gdb/testsuite/gdb.mi/mi2-var-child.exp | 76 ++++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.mi/var-cmd.c         | 68 ++++++++++++++++++++++++++++++
>  gdb/varobj.c                           | 20 ++++-----
>  gdb/varobj.h                           |  9 ++++
>  7 files changed, 221 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/ada-varobj.c b/gdb/ada-varobj.c
> index b9f83be..3d56526 100644
> --- a/gdb/ada-varobj.c
> +++ b/gdb/ada-varobj.c
> @@ -1026,5 +1026,6 @@ const struct lang_varobj_ops ada_varobj_ops =
>    ada_type_of_child,
>    ada_value_of_variable,
>    ada_value_is_changeable_p,
> -  ada_value_has_mutated
> +  ada_value_has_mutated,
> +  varobj_default_is_path_expr_parent
>  };
> diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
> index 9c2860d..f7bc52b 100644
> --- a/gdb/c-varobj.c
> +++ b/gdb/c-varobj.c
> @@ -126,6 +126,56 @@ adjust_value_for_child_access (struct value **value,
>      }
>  }
>  
> +/* Is VAR a path expression parent, i.e., can it be used to construct
> +   a valid path expression?  */
> +
> +static int
> +c_is_path_expr_parent (struct varobj *var)
> +{
> +  struct type *type;
> +
> +  /* "Fake" children are not path_expr parents.  */
> +  if (CPLUS_FAKE_CHILD (var))
> +    return 0;
> +
> +  type = varobj_get_gdb_type (var);
> +
> +  /* Anonymous unions and structs are also not path_expr parents.  */
> +  if ((TYPE_CODE (type) == TYPE_CODE_STRUCT
> +       || TYPE_CODE (type) == TYPE_CODE_UNION)
> +      && TYPE_NAME (type) == NULL
> +      && TYPE_TAG_NAME (type) == NULL)
> +    {
> +      struct varobj *parent = var->parent;
> +
> +      while (parent != NULL && CPLUS_FAKE_CHILD (parent))
> +	parent = parent->parent;
> +
> +      if (parent != NULL)
> +	{
> +	  struct type *parent_type;
> +	  int was_ptr;
> +
> +	  parent_type = varobj_get_value_type (parent);
> +	  adjust_value_for_child_access (NULL, &parent_type, &was_ptr, 0);
> +
> +	  if (TYPE_CODE (parent_type) == TYPE_CODE_STRUCT
> +	      || TYPE_CODE (parent_type) == TYPE_CODE_UNION)
> +	    {
> +	      const char *field_name;
> +
> +	      gdb_assert (var->index < TYPE_NFIELDS (parent_type));
> +	      field_name = TYPE_FIELD_NAME (parent_type, var->index);
> +	      return !(field_name == NULL || *field_name == '\0');
> +	    }
> +	}
> +
> +      return 0;
> +    }
> +
> +  return 1;
> +}
> +
>  /* C */
>  
>  static int
> @@ -493,7 +543,8 @@ const struct lang_varobj_ops c_varobj_ops =
>     c_type_of_child,
>     c_value_of_variable,
>     varobj_default_value_is_changeable_p,
> -   NULL /* value_has_mutated */
> +   NULL, /* value_has_mutated */
> +   c_is_path_expr_parent  /* is_path_expr_parent */
>  };
>  
>  /* A little convenience enum for dealing with C++/Java.  */
> @@ -904,7 +955,8 @@ const struct lang_varobj_ops cplus_varobj_ops =
>     cplus_type_of_child,
>     cplus_value_of_variable,
>     varobj_default_value_is_changeable_p,
> -   NULL /* value_has_mutated */
> +   NULL, /* value_has_mutated */
> +   c_is_path_expr_parent  /* is_path_expr_parent */
>  };
>  
>  
> diff --git a/gdb/jv-varobj.c b/gdb/jv-varobj.c
> index 0bb8e64..fb4d417 100644
> --- a/gdb/jv-varobj.c
> +++ b/gdb/jv-varobj.c
> @@ -101,5 +101,6 @@ const struct lang_varobj_ops java_varobj_ops =
>     java_type_of_child,
>     java_value_of_variable,
>     varobj_default_value_is_changeable_p,
> -   NULL /* value_has_mutated */
> +   NULL, /* value_has_mutated */
> +   varobj_default_is_path_expr_parent
>  };
> diff --git a/gdb/testsuite/gdb.mi/mi2-var-child.exp b/gdb/testsuite/gdb.mi/mi2-var-child.exp
> index f992a63..fc02066 100644
> --- a/gdb/testsuite/gdb.mi/mi2-var-child.exp
> +++ b/gdb/testsuite/gdb.mi/mi2-var-child.exp
> @@ -1163,6 +1163,13 @@ mi_create_breakpoint \
>      "$srcfile:$lineno" "break in do_anonymous_type_tests" \
>      -disp keep -func do_anonymous_type_tests \
>      -file ".*var-cmd.c" -line $lineno
> +
> +set lineno [gdb_get_line_number "nested struct union tests breakpoint"]
> +mi_create_breakpoint \
> +    "$srcfile:$lineno" "break in do_nested_struct_union_tests" \
> +    -disp keep -func do_nested_struct_union_tests \
> +    -file ".*var-cmd.c" -line $lineno
> +
>  mi_execute_to "exec-continue" "breakpoint-hit" "do_anonymous_type_tests" ""\
>      ".*" ".*" {"" "disp=\"keep\""} \
>      "continue to do_anonymous_type_tests breakpoint"
> @@ -1236,5 +1243,74 @@ set tree {
>  
>  mi_walk_varobj_tree c $tree verify_everything
>  
> +mi_send_resuming_command "exec-continue" \
> +    "continuing execution to enter do_nested_struct_union_tests"
> +mi_expect_stop "breakpoint-hit" "do_nested_struct_union_tests" ".*" ".*" ".*" \
> +    {.* disp="keep"} "Run till MI stops in do_nested_struct_union_tests"
> +
> +set struct_ss_tree {
> +    {struct s_a} a1 {
> +      int a {}
> +    }
> +    {struct s_b} b1 {
> +      int b {}
> +    }
> +    {union u_ab} u1 {
> +      {struct s_a} a {
> +        int a {}
> +      }
> +      {struct s_b} b {
> +        int b {}
> +      }
> +    }
> +    anonymous union {
> +      {struct s_a} a2 {
> +        int a {}
> +      }
> +      {struct s_b} b2 {
> +        int b {}
> +      }
> +    }
> +    {union {...}} u2 {
> +      {struct s_a} a3 {
> +        int a {}
> +      }
> +      {struct s_b} b3 {
> +        int b {}
> +      }
> +    }
> +  }
> +
> +set tree "
> +  {struct ss} var {
> +    $struct_ss_tree
> +  }
> +"
> +
> +mi_walk_varobj_tree c $tree verify_everything
> +
> +set tree {
> +  {struct {...}} var2 {
> +    {td_u_ab} ab {
> +      {td_s_a} a {
> +	int a {}
> +      }
> +      {td_s_b} b {
> +	int b {}
> +      }
> +    }
> +  }
> +}
> +
> +mi_walk_varobj_tree c $tree verify_everything
> +
> +set tree "
> +  {struct ss *} ss_ptr {
> +    $struct_ss_tree
> +  }
> +"
> +
> +mi_walk_varobj_tree c $tree verify_everything
> +
>  mi_gdb_exit
>  return 0
> diff --git a/gdb/testsuite/gdb.mi/var-cmd.c b/gdb/testsuite/gdb.mi/var-cmd.c
> index 698b294..4bb2746 100644
> --- a/gdb/testsuite/gdb.mi/var-cmd.c
> +++ b/gdb/testsuite/gdb.mi/var-cmd.c
> @@ -552,6 +552,73 @@ do_anonymous_type_tests (void)
>    return; /* anonymous type tests breakpoint */
>  }
>  
> +void
> +do_nested_struct_union_tests (void)
> +{
> +  struct s_a
> +  {
> +    int a;
> +  };
> +  struct s_b
> +  {
> +    int b;
> +  };
> +  union u_ab
> +  {
> +    struct s_a a;
> +    struct s_b b;
> +  };
> +  struct ss
> +  {
> +    struct s_a a1;
> +    struct s_b b1;
> +    union u_ab u1;
> +
> +    /* Anonymous union.  */
> +    union
> +    {
> +      struct s_a a2;
> +      struct s_b b2;
> +    };
> +
> +    union
> +    {
> +      struct s_a a3;
> +      struct s_b b3;
> +    } u2;
> +  };
> +
> +  typedef struct
> +  {
> +    int a;
> +  } td_s_a;
> +
> +  typedef struct
> +  {
> +    int b;
> +  } td_s_b;
> +
> +  typedef union
> +  {
> +    td_s_a a;
> +    td_s_b b;
> +  } td_u_ab;
> +
> +  struct ss var;
> +  struct
> +  {
> +    td_u_ab ab;
> +  } var2;
> +
> +  struct ss *ss_ptr;
> +
> +  memset (&var, 0, sizeof (var));
> +  memset (&var2, 0, sizeof (var2));
> +  ss_ptr = &var;
> +
> +  return; /* nested struct union tests breakpoint */
> +}
> +
>  int
>  main (int argc, char *argv [])
>  {
> @@ -563,6 +630,7 @@ main (int argc, char *argv [])
>    do_at_tests ();
>    do_bitfield_tests ();
>    do_anonymous_type_tests ();
> +  do_nested_struct_union_tests ();
>    exit (0);
>  }
>  
> diff --git a/gdb/varobj.c b/gdb/varobj.c
> index 8016368..7fa98f2 100644
> --- a/gdb/varobj.c
> +++ b/gdb/varobj.c
> @@ -1069,18 +1069,18 @@ varobj_get_gdb_type (struct varobj *var)
>  static int
>  is_path_expr_parent (struct varobj *var)
>  {
> -  struct type *type;
> -
> -  /* "Fake" children are not path_expr parents.  */
> -  if (CPLUS_FAKE_CHILD (var))
> -    return 0;
> +  gdb_assert (var->root->lang_ops->is_path_expr_parent != NULL);
> +  return var->root->lang_ops->is_path_expr_parent (var);
> +}
>  
> -  type = varobj_get_value_type (var);
> +/* Is VAR a path expression parent, i.e., can it be used to construct
> +   a valid path expression?  By default we assume any VAR can be a path
> +   parent.  */
>  
> -  /* Anonymous unions and structs are also not path_expr parents.  */
> -  return !((TYPE_CODE (type) == TYPE_CODE_STRUCT
> -	    || TYPE_CODE (type) == TYPE_CODE_UNION)
> -	   && TYPE_NAME (type) == NULL);
> +int
> +varobj_default_is_path_expr_parent (struct varobj *var)
> +{
> +  return 1;
>  }
>  
>  /* Return the path expression parent for VAR.  */
> diff --git a/gdb/varobj.h b/gdb/varobj.h
> index 1199e0b..12a842b 100644
> --- a/gdb/varobj.h
> +++ b/gdb/varobj.h
> @@ -213,6 +213,12 @@ struct lang_varobj_ops
>       Languages where types do not mutate can set this to NULL.  */
>    int (*value_has_mutated) (struct varobj *var, struct value *new_value,
>  			    struct type *new_type);
> +
> +  /* Return nonzero if VAR is a suitable path expression parent.
> +
> +     For C like languages with anonymous structures and unions an anonymous
> +     structure or union is not a suitable parent.  */
> +  int (*is_path_expr_parent) (struct varobj *var);
>  };
>  
>  extern const struct lang_varobj_ops c_varobj_ops;
> @@ -326,4 +332,7 @@ extern void varobj_formatted_print_options (struct value_print_options *opts,
>  
>  extern void varobj_restrict_range (VEC (varobj_p) *children, int *from,
>  				   int *to);
> +
> +extern int varobj_default_is_path_expr_parent (struct varobj *var);
> +
>  #endif /* VAROBJ_H */
>