[RFA] Fix varobj/15166

[RFA] Fix varobj/15166

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

Hi,

$SUBJECT concerns an assertion failure that is triggered when using 
pretty-printing with varobj. The cause of the problem is actually pretty 
simple: cplus_describe_child knows nothing about pretty printing.

As a result, it strictly enforces that the immediate children of a root 
varobj whose type is a class/struct must be one of the "fake" children. 
When asking for an index greater than 2 (CPLUS_FAKE_CHILDren are indices 
0, 1, 2), the code asserts because of an unknown fake child.

The solution is to teach cplus_describe_child how to fetch the N'th 
child from the pretty-printed varobj.

No regressions on x86_64 native and native-gdbserver.

Keith

ChangeLog
2013-07-08  Keith Seitz  <keiths@redhat.com>

	PR varobj/15166
	* varobj.c (get_pretty_printed_element_index): New function.
	(cplus_describe_child): Add handling for pretty-printed varobjs.

testsuite/ChangeLog
2013-07-08  Keith Seitz  <keiths@redhat.com>

	PR varobj/15166
	* lib/mi-support.exp (mi_list_varobj_children_range): Ignore
	a display hint in the expected output.
	* gdb.python/py-pp-varobj-update.cc: New file.
	* gdb.python/py-pp.varobj-update.exp: New file.
	* gdb.python/py-pp-varobj-update.py: New file.


[-- Attachment #2: pp-varobj-update.patch --]
[-- Type: text/x-patch, Size: 13488 bytes --]

diff --git a/gdb/varobj.c b/gdb/varobj.c
index d4fa6ba..540b440 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -3697,6 +3697,60 @@ match_accessibility (struct type *type, int index, enum accessibility acc)
     return 0;
 }
 
+#if HAVE_PYTHON
+/* Return the INDEX'th child element from the pretty-printed varobj VAR.
+   The return value must be decref'd by the caller (if non-NULL).  */
+
+static PyObject *
+get_pretty_printed_element_index (struct varobj *var, int index)
+{
+  struct cleanup *back_to;
+  PyObject *children, *iter, *item;
+
+  /* This function should not be called with a non-pretty-printed varobj.  */
+  gdb_assert (varobj_pretty_printed_p (var));
+
+  if (!gdb_python_initialized)
+    return NULL;
+
+  if (!PyObject_HasAttr (var->pretty_printer, gdbpy_children_cst))
+    return NULL;
+
+  children = PyObject_CallMethodObjArgs (var->pretty_printer,
+					 gdbpy_children_cst, NULL);
+  if (children == NULL)
+    {
+      gdbpy_print_stack ();
+      error (_("Null value for children."));
+    }
+
+  back_to = make_cleanup_py_decref (children);
+
+  iter = PyObject_GetIter (children);
+  if (iter == NULL)
+    {
+      gdbpy_print_stack ();
+      error (_("Could not get children iterator"));
+    }
+
+  make_cleanup_py_decref (iter);
+
+  item = NULL;
+  while (index-- >= 0)
+    {
+      Py_XDECREF (item);
+      item = PyIter_Next (iter);
+
+      /* We should not be able to ask for an index for which we do not
+	 have a child varobj already!  */
+      gdb_assert (item != NULL);
+    }
+
+  do_cleanups (back_to);
+  return item;
+}
+#endif /* HAVE_PYTHON */
+
 static void
 cplus_describe_child (struct varobj *parent, int index,
 		      char **cname, struct value **cvalue, struct type **ctype,
@@ -3737,7 +3791,52 @@ cplus_describe_child (struct varobj *parent, int index,
     {
       char *join = was_ptr ? "->" : ".";
 
-      if (CPLUS_FAKE_CHILD (parent))
+      if (varobj_pretty_printed_p (var))
+	{
+#if HAVE_PYTHON
+	  struct cleanup *back_to;
+	  PyObject *py_v, *item;
+	  const char *name;
+	  struct value *v = NULL;
+
+	  if (cfull_expression != NULL)
+	    error (_("Cannot get path expression for dynamic varobj."));
+
+	  back_to = varobj_ensure_python_env (var);
+	  item = get_pretty_printed_element_index (var, index);
+	  if (item != NULL)
+	    {
+	      if (!PyArg_ParseTuple (item, "sO", &name, &py_v))
+		{
+		  gdbpy_print_stack ();
+		  error (_("Invalid item from the child list"));
+		}
+
+	      if (cname != NULL)
+		*cname = xstrdup (name);
+
+	      if (cvalue != NULL || ctype != NULL)
+		{
+		  v = convert_value_from_python (py_v);
+		  if (v == NULL)
+		    gdbpy_print_stack ();
+		}
+
+	      if (cvalue != NULL && value != NULL)
+		*cvalue = v;
+
+	      if (ctype != NULL && v != NULL)
+		*ctype = value_type (v);
+	    }
+
+	  Py_XDECREF (item);
+	  do_cleanups (back_to);
+#else /* !HAVE_PYTHON */
+	  gdb_assert_not_reached
+	    ("should never be called if Python is not enabled");
+#endif /* !HAVE_PYTHON */
+	}
+      else if (CPLUS_FAKE_CHILD (parent))
 	{
 	  /* The fields of the class type are ordered as they
 	     appear in the class.  We are given an index for a
diff --git a/gdb/testsuite/gdb.python/py-pp-varobj-update.cc b/gdb/testsuite/gdb.python/py-pp-varobj-update.cc
new file mode 100644
index 0000000..1b2552f
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-pp-varobj-update.cc
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+// This class is used alongside its pretty-printer to test varobj/15166.
+
+class hole
+{
+public:
+  hole () : a (1), b (2), c (3), d (4), e (5), f (6), g (7),
+	    x (10), y (20), z (30) {}
+  int sum (void) const;
+
+  int a;
+  int b;
+  int c;
+  int d;
+  int e;
+  int f;
+  int g;
+
+private:
+  int x;
+  int y;
+  int z;
+};
+
+int
+hole::sum (void) const
+{
+  return a + b + c + d + e + f + g + y - x - z - 8;
+}
+
+int
+main (void)
+{
+  hole h;
+
+  return h.sum (); // break here
+}
diff --git a/gdb/testsuite/gdb.python/py-pp-varobj-update.exp b/gdb/testsuite/gdb.python/py-pp-varobj-update.exp
new file mode 100644
index 0000000..e130657
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-pp-varobj-update.exp
@@ -0,0 +1,172 @@
+# Copyright (C) 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite.
+#
+# Test varobj/15166
+#
+# This test creates a pretty-printer which inserts a "hole" in the
+# layout for the class.  The hole is intentionally put after the
+# indices for the CPLUS_FAKE_CHILD (0, 1, 2), which caused an assert
+# in cplus_describe_child.
+#
+# For reference, these "holes" are:
+#
+# index   0 1 2 3 4 5 6 7 8 9
+# gdb     a b c d e f g x y z
+# PP      a c e g
+#
+# So, for example, the fourth value child (#7 when keys are taken into
+# account) of the pretty-printed varobj is actually index six in the real
+# type information for the class.
+
+load_lib mi-support.exp
+load_lib cp-support.exp
+set MIFLAGS "-i=mi2"
+
+standard_testfile .cc
+
+# Start gdb so that we can query supported MI features.
+gdb_exit
+if {[mi_gdb_start]} {
+    continue
+}
+
+# Skip these tests if either Python scripting is disabled or C++ is
+# verboten.
+if {[lsearch -exact [mi_get_features] python] < 0
+    || [skip_cplus_tests]} { continue }
+
+if {[gdb_compile $srcdir/$subdir/$srcfile $binfile executable \
+	 {debug c++}] != ""} {
+    untested "Could not compile $srcfile"
+    return -1
+}
+
+mi_delete_breakpoints
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load $binfile
+mi_runto main
+
+# The test is actually pretty simple: for a given child (index
+# must be greater than CPLUS_FAKE_CHILD, ie, 2):
+# We will attempt to modify and evaluate the child's value both
+# with MI and without, making sure the values are in sync.
+
+mi_continue_to_line [gdb_get_line_number "break here" $srcfile] \
+    "step to `break here'"
+
+# First, run the test without pretty-printing.
+
+# Create the raw (no pretty-printing) varobj.
+mi_create_dynamic_varobj raw h \
+    "create raw varobj, no pretty-printing"
+
+# Get the list of public children
+mi_list_varobj_children raw {
+    {raw.public public 7}
+    {raw.private private 3}
+} "children of `raw'"
+
+mi_list_varobj_children raw.public {
+    {raw.public.a a 0 int}
+    {raw.public.b b 0 int}
+    {raw.public.c c 0 int}
+    {raw.public.d d 0 int}
+    {raw.public.e e 0 int}
+    {raw.public.f f 0 int}
+    {raw.public.g g 0 int}
+} "children of `raw.public'"
+
+# Check the value of `h.g'
+mi_check_varobj_value raw.public.g 7 "value of `raw.public.g'"
+
+# Set the value and double-check the varobj's value.  The value will
+# not change until the varobj is updated.
+mi_gdb_test "set var h.g = 77" ""
+mi_check_varobj_value raw.public.g 7 \
+    "value of `raw.public.g' after external set"
+mi_varobj_update raw.public.g raw.public.g "update `raw.public.g'"
+
+# Check the value of this varobj
+mi_check_varobj_value raw.public.g 77 \
+    "value of `raw.public.f' after update"
+
+# Now change the value of this varobj using MI
+mi_gdb_test "-var-assign raw.public.g 123" \
+    "\\^done,value=\"123\"" "assign new value to `raw.public.g'"
+
+# Finally, check the CLI's value of this varobj
+mi_gdb_test "print h.g" ".* = 123.*\\^done" "new CLI value of h.g"
+
+# Delete the root varobj and try again using pretty-printing
+mi_delete_varobj raw "delete `raw'"
+
+# Load the pretty-printer
+set remote_python_file [remote_download host \
+			    "$srcdir/$subdir/${testfile}.py"]
+mi_gdb_test "python exec (open ('$remote_python_file').read ())" ""
+
+# Enable pretty-printing
+mi_gdb_test "-enable-pretty-printing" ""
+
+# Create the new pretty-printed varobj root
+mi_create_dynamic_varobj pp h \
+    "create pp varobj, pretty-printing enabled"
+
+# Get the list of children, which will be alternating keys and values
+mi_list_varobj_children pp {
+    {pp.\\[0\\] \\[0\\] 4 "char \\[4\\]"}
+    {pp.\\[1\\] \\[1\\] 0 int}
+    {pp.\\[2\\] \\[2\\] 4 "char \\[4\\]"}
+    {pp.\\[3\\] \\[3\\] 0 int}
+    {pp.\\[4\\] \\[4\\] 4 "char \\[4\\]"}
+    {pp.\\[5\\] \\[5\\] 0 int}
+    {pp.\\[6\\] \\[6\\] 4 "char \\[4\\]"}
+    {pp.\\[7\\] \\[7\\] 0 int}
+} "children of `pp'"
+
+# Check the value of `h.g'
+mi_check_varobj_value pp.\[7\] 123 "value of `pp.\[7\]'"
+
+# Set the value and double-check the varobj's value.  The value will
+# not change until the varobj is updated.
+mi_gdb_test "set var h.g = 77" ""
+mi_check_varobj_value pp.\[7\] 123 \
+    "value of `pp.\[7\]' after external set"
+
+# Update the child varobj and check the new value
+mi_varobj_update pp.\[7\] {
+    pp.\\[7\\]
+} "update `pp.\[7\]'"
+mi_check_varobj_value pp.\[7\] 77 \
+    "value of `pp.\[7\]' after update"
+
+# Now change the value of this varobj using MI
+mi_gdb_test "-var-assign pp.\[7\] 321" \
+    "\\^done,value=\"321\"" "assign new value to `pp.\[7\]'"
+
+# Check the CLI's value of this varobj
+mi_gdb_test "print h.g" ".* = 321.*\\^done" "new CLI value of h.g"
+
+# One last test: attempt to modify a key's value
+mi_gdb_test "-var-assign pp.\[0\] 1" \
+    "\\^error,msg=\"-var-assign: Variable object is not editable\"" \
+    "attempt to modify a key"
+
+# Delete the root varobj and try again using pretty-printing
+mi_delete_varobj pp "delete `pp'"
+
+remote_file host delete $remote_python_file
diff --git a/gdb/testsuite/gdb.python/py-pp-varobj-update.py b/gdb/testsuite/gdb.python/py-pp-varobj-update.py
new file mode 100644
index 0000000..c1624b2
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-pp-varobj-update.py
@@ -0,0 +1,88 @@
+# Copyright (C) 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB test suite.
+
+# This file defines a pretty printer that is used as a test case for
+# varobj/15166.
+
+import gdb
+
+# This iterator method is used to create "holes" in the `hole' class
+# layout so that the indices of the type used by gdb (internally) are
+# intentionally skewed from the pretty-printer, e.g.,
+#
+# index   0 1 2 3 4 5 6 7 8 9
+# gdb     a b c d e f g x y z
+# PP      a c e g
+
+def myiterator (var):
+    p = 0
+    while p != 2 * 4:
+        if ((p % 2) == 0):
+            # key
+            value = "key" + str (p / 2)
+        else:
+            # value
+            i = ((p - 1) / 2) % 4
+            if (i == 0):
+                name = 'a'
+            elif (i == 1):
+                name = 'c'
+            elif (i == 2):
+                name = 'e'
+            else:
+                name = 'g'
+            value = var.val[name]
+
+        yield ("[" + str (p) + "]", value)
+        p += 1
+
+class HolePrinter (object):
+    def __init__ (self, val):
+        self.val = val
+
+    def to_string (self):
+        return "<" + str (self.val['a']) + "," + str (self.val['c']) + "," \
+            + str (self.val['e']) + "," + str (self.val['g']) + ">"
+
+    def display_hint (self):
+        return 'map'
+
+    def children (self):
+        return myiterator (self)
+
+def hole_lookup_function (val):
+    "Look-up and return a pretty-printer that can print val."
+
+    # Get the type.
+    type = val.type
+
+    # If it points to a reference, get the reference.
+    if type.code == gdb.TYPE_CODE_REF:
+        type = type.target ()
+
+    # Get the unqualified type, stripped of typedefs.
+    type = type.unqualified ().strip_typedefs ()
+
+    # Get the type name.    
+    typename = type.tag
+
+    if typename == "hole":
+        return HolePrinter (val)
+
+    return None
+
+gdb.pretty_printers.append (hole_lookup_function)

Re: [RFA] Fix varobj/15166

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> $SUBJECT concerns an assertion failure that is triggered when using
Keith> pretty-printing with varobj. The cause of the problem is actually
Keith> pretty simple: cplus_describe_child knows nothing about pretty
Keith> printing.

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.

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.  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.

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.

Keith> +	      if (cvalue != NULL && value != NULL)
Keith> +		*cvalue = v;

I didn't understand why the condition uses 'value' here but the
assignment uses 'v'.

Tom

Re: [RFA] Fix varobj/15166

[Keith Seitz]

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

Re: [RFA] Fix varobj/15166

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> $SUBJECT concerns an assertion failure that is triggered when using
Keith> pretty-printing with varobj. The cause of the problem is actually
Keith> pretty simple: cplus_describe_child knows nothing about pretty
Keith> printing.

I think your reply to me answered all my big questions about the patch.

Looking at it I have one other minor issue (in addition to the typo thing).

Keith> +  item = NULL;
Keith> +  while (index-- >= 0)
Keith> +    {
Keith> +      Py_XDECREF (item);
Keith> +      item = PyIter_Next (iter);
Keith> +
Keith> +      /* We should not be able to ask for an index for which we do not
Keith> +	 have a child varobj already!  */
Keith> +      gdb_assert (item != NULL);

I don't think this is ok to do.  Iterators run Python code, which can do
anything.

This is why I was curious about value preservation and updating.  It
seems like re-running the Python may yield different results from what
is "expected" (in the sense that the varobj's current state can be seen
as a snapshot of a particular inferior state -- even though we don't
actually store the bits making up this snapshot).

Anyway I think the checking here has to be more lenient, not an assert.

Tom