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

[Andrew Burgess <aburgess at broadcom dot com>]

Ping #2.

Thanks,
Andrew

On 05/06/2014 3:11 PM, Andrew Burgess wrote:
> Ping!
> 
> The regression Vladimir pointed out is fixed in V2 with no regressions
> in any of the new tests I've added.
> 
> Tested on x86-64 RHEL5 with no regressions.
> 
> Thanks,
> Andrew
> 
> On 29/05/2014 9:55 AM, Andrew Burgess wrote:
>> 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 */
>>>
>>
>>
>>
> 
> 
>