Re: [RFA] Fix varobj/15166

[Keith Seitz <keiths@redhat.com>]

On 07/23/2013 01:32 PM, Tom Tromey wrote:
> There were a few things I didn't understand here.
>
> First, which part of the test case actually triggered the crash?
> It isn't obvious from the PR or the patch submission or the test.

I apologize, it's buried in a bunch of other tests in there. [I found 
quite a few problems related to the actual PR, such as updating values 
outside varobj and noticing in CLI and vice-versa, so I fixed them all 
and added tests for them.]

It's this specifically:

(gdb)
-var-update var1.child_of_pretty_printed_varobj

where the index of child_of_pretty_printed_varobj > 2 (+ any 
baseclasses), e.g., if child_of_pretty_printed_varobj is the 3, 4, 5, 
... item returned by the pp's children method.

var1 is a C++ class/struct that has a pretty-printer installed, so its 
children are defined by the pretty-printer's children method.

However varobj.c assumes/requires that the first (up to) three children 
(beyond baseclasses) are "public", "private", and "protected". No other 
children are permitted. ["Everything beyond the baseclasses can only be 
'public', 'private', or 'protected'."]

cplus_describe_child enforces this requirement. It calculates the number 
of children, and attempts to compute a "name" for this requested child 
(requests for value/expression are ignored):

  	  switch (index)
	    {
	    case 0:
	      if (children[v_public] > 0)
	 	access = "public";
	      else if (children[v_private] > 0)
	 	access = "private";
	      else
	 	access = "protected";
	      break;
	    case 1:
	      if (children[v_public] > 0)
		{
		  if (children[v_private] > 0)
		    access = "private";
		  else
		    access = "protected";
		}
	      else if (children[v_private] > 0)
	 	access = "protected";
	      break;
	    case 2:
	      /* Must be protected.  */
	      access = "protected";
	      break;
	    default:
	      /* error!  */
	      break;
	    }

	  gdb_assert (access);
	  if (cname)
	    *cname = xstrdup (access);

For any requested child > 2, we end up in the default: case, which does 
nothing. ACCESS is NULL, and the assert triggers.

Varobj simply cannot enforce the CPLUS_FAKE_CHILD thing for 
pretty-printed varobjs. It is only an issue for C++, where varobj.c 
requires/enforces these CPLUS_FAKE_CHILDren.

> Keith> As a result, it strictly enforces that the immediate children of a
> Keith> root varobj whose type is a class/struct must be one of the "fake"
> Keith> children. When asking for an index greater than 2 (CPLUS_FAKE_CHILDren
> Keith> are indices 0, 1, 2), the code asserts because of an unknown fake
> Keith> child.
>
> It seems strange to me that the dynamic varobj case would ever end up in
> this code.

The path is pretty straightforward:

#0  internal_error (file=0xfdbd14 "../../gdb/gdb/varobj.c", line=3887, 
string=0xfdbbc0 "%s: Assertion `%s' failed.") at ../../gdb/gdb/utils.c:842
#1  0x000000000080f1df in cplus_describe_child (parent=0x2284bb0, 
index=5, cname=0x0, cvalue=0x7fffffffd428, ctype=0x0, 
cfull_expression=0x0) at ../../gdb/gdb/varobj.c:3887
#2  0x000000000080f324 in cplus_value_of_child (parent=0x2284bb0, 
index=5) at ../../gdb/gdb/varobj.c:3928
#3  0x000000000080cdfb in value_of_child (parent=0x2284bb0, index=5) at 
../../gdb/gdb/varobj.c:2839
#4  0x000000000080b837 in varobj_update (varp=0x7fffffffd598, 
explicit=1) at ../../gdb/gdb/varobj.c:2068
[...]

> Perhaps dynamic varobjs should just dispatch to their own
> "language_specific"-like object... or else it seems that all the
> languages in the 'languages' array will need updates.

It could dispatch, but the functionality for cplus_describe_child is 
capable of handling it (which is what this patch is attempting to add). 
Again, it is only an issue for C++ because CPLUS_FAKE_CHILD is only 
enforced for C++ varobjs.

> Keith> +static PyObject *
> Keith> +get_pretty_printed_element_index (struct varobj *var, int index)
> [...]
> Keith> +  iter = PyObject_GetIter (children);
>
> It seems like calling this could implicitly cause a varobj update.
> But probably I just don't understand.

This does not modify the varobj, only the actual values (outside of 
varobj/in gdb) of the children.

The user/UI can still call -varobj-update on the parent or any sibling 
and get notification of any changes.

This could be exceptionally slow, but there is no way in the current API 
to rewind the iterator (if we have one saved) or access any child randomly.

The alternatives are:
1) Update the root and report all the changes that occurred

While this would be much more efficient, I don't think this is what MI 
clients will be expecting, and it is a significant departure from the 
current behavior. [Although that is how it originally worked: only root 
varobjs could be updated.]

2) Change the pretty-printer API to allow either random access to 
children or rewind the iterator.

I also intentionally implemented it this way to avoid any such API changes.

> Keith> +	      if (cvalue != NULL && value != NULL)
> Keith> +		*cvalue = v;
>
> I didn't understand why the condition uses 'value' here but the
> assignment uses 'v'.

Sometimes it's more obvious than you think: it's a typo. :-)

Thank you for taking a look.

Keith