This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 04/17/2014 04:05 PM, Andrew Burgess wrote:
The MI command -var-info-path-expression currently does not handle some
cases of nested structures / unions correctly, consider the following
source file:

int
main ( void )
{
   struct a_t
   {
     int var;
   };

   struct s_t
   {
     struct a_t a;
   };

   struct s_t s;

   return 0; /* Break Here (line 16) */
}

The changes to varobj.c feel like it might be possible to simplify them,
but I can't figure out how, any change I make seems to regress something,
so for now I'm proposing this patch.  The change is very contained anyway,
so if someone can improve it later it's not going to cause any disruption.

OK to apply?

Andrew,

thanks for the patch - this is quite a complicated area. It seems like I found a bug
though. I've modified your test function to end with:

  struct ss *ptr;

  memset (&var, 0, sizeof (var));
  memset (&var2, 0, sizeof (var2));
  ptr = &var;

and I see this interaction:

	-var-create var * ptr
	^done,name="var",numchild="5",value="0x7fffffffd860",type="struct ss *",thread-id="1",has_more="0"
	(gdb)
	-var-list-children var
	^done,numchild="5",children=[child={name="var.a1",exp="a1",numchild="1",type="struct s_a",thread-id="1"},child=	
	{name="var.b1",exp="b1",numchild="1",type="struct s_b",thread-id="1"},child={name="var.u1",exp="u1",numchild="2",type="union
	u_ab",thread-	id="1"},child={name="var.3_anonymous",exp="<anonymous union>",numchild="2",type="union {...}",thread-id="1"},child=
	{name="var.u2",exp="u2",numchild="2",type="union {...}",thread-id="1"}],has_more="0"
	(gdb)
	-var-list-children var.u2
	^done,numchild="2",children=[child={name="var.u2.a3",exp="a3",numchild="1",type="struct s_a",thread-id="1"},child=		
	{name="var.u2.b3",exp="b3",numchild="1",type="struct s_b",thread-id="1"}],has_more="0"
	(gdb)
	-var-info-path-expression var.u2.a3
	^done,path_expr="(ptr).a3"
	(gdb)

ss.u2 is a named field, so I would expect the above to produce (ptr)->u2.a3. Am I wrong, or this is a bug?

For reference, here are my notes.

Basically anonymous union or struct is this:

	struct S {
		union { int a; int b; };
	};

So the inner declaration both has no tag name for union nor declarator. Existing GDB code just
checks for tagless union, which fails for this case:

	struct S {
		union { int a; int b; } u;
	};

With your patch, we first check for tagless union, and then separately check when it has a field name,
catching the second case. I though that maybe we could directly check for empty field name, using the
code below like this.

	diff --git a/gdb/varobj.c b/gdb/varobj.c
	index 8016368..ea0bb5b 100644
	--- a/gdb/varobj.c
	+++ b/gdb/varobj.c
	@@ -1069,18 +1069,46 @@ varobj_get_gdb_type (struct varobj *var)
	 static int
	 is_path_expr_parent (struct varobj *var)
	 {
	-  struct type *type;
	+  struct type *type, *parent_type;
	+  int r1, r2;
	
	   /* "Fake" children are not path_expr parents.  */
	   if (CPLUS_FAKE_CHILD (var))
	     return 0;
	
	-  type = varobj_get_value_type (var);
	-
	-  /* 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);
	+  // See whether this is anonymous structure or union, like so:
	+  // struct S {
	+  //   union { int a; int b; };
	+  // };
	+  //
	+  // In which case each member is union in injected in eclosing scope (here, S)
	+  // we need to use varobj corresponding to S to obtain path expression.
	+  //
	+  // This situation is only possible where a varobj has structure type and its
	+  // parent has structure type, and varobj does not have a field name within
	+  // its parent.
	+  if (var->parent != NULL)
	+    {
	+      int var_is_structural, parent_is_structural;
	+      type = varobj_get_value_type (var);
	+      var_is_structural = (TYPE_CODE (type) == TYPE_CODE_STRUCT
	+                          || TYPE_CODE (type) == TYPE_CODE_UNION);
	+
	+      parent_type = varobj_get_value_type (var->parent);
	+      parent_type = get_target_type (parent_type);
	+      parent_is_structural = (TYPE_CODE (parent_type) == TYPE_CODE_STRUCT
	+                             || TYPE_CODE (parent_type) == TYPE_CODE_UNION);
	+
	+      if (var_is_structural && parent_is_structural) {
	+       const char *field_name;
	+
	+       gdb_assert (var->index < TYPE_NFIELDS (parent_type));
	+       field_name = TYPE_FIELD_NAME (parent_type, var->index);
	+       if (field_name == NULL || *field_name == '\0')
	+         return 0;
	+      }
	+    }
	+  return 1;
	 }


This code failed testing pretty quickly. When we have a pointer to structure containing anonymous unions,
the parent varobj has PTR as type code. It's only inside c-varobj that we know that for listing children
of such varobj, we need to look at the structure. Therefore, my code below fails in that case. And looks
like yours code fails in a similar way.

It actually seems that the code above deals with C and C++ specific logic, but it's in varobj.c, not in
c-varobj.c, which is rather inconvenient. Inside c-varobj.c, we could have used
adjust_value_for_child_access.


Thanks,
Volodya










Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]