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

[Andrew Burgess <aburgess at broadcom dot com>]

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 */
>>
> 
> 
>