diff mbox

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

Message ID 1400535181-5620-1-git-send-email-aburgess@broadcom.com
State Committed
Headers show

Commit Message

Andrew Burgess May 19, 2014, 9:33 p.m. UTC
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(-)

Comments

Andrew Burgess May 29, 2014, 8:55 a.m. UTC | #1
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 */
>
Andrew Burgess June 5, 2014, 2:11 p.m. UTC | #2
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 */
>>
> 
> 
>
Andrew Burgess June 12, 2014, 9:02 a.m. UTC | #3
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 */
>>>
>>
>>
>>
> 
> 
>
Vladimir Prus June 17, 2014, 9:20 a.m. UTC | #4
On 05/20/2014 01:33 AM, 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?

Andrew,

thanks for the updated patch - it is OK.

Thanks,
Volodya
diff mbox

Patch

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