[RFA] Fix varobj/15166

[Keith Seitz <keiths at redhat dot com>]

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.

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)