RFC: fix virtual base class bugs

[Tom Tromey <tromey at redhat dot com>]

This patch fixes all the virtual base class printing bugs I could find.

I plan to check this in soon unless there are comments or objections.

There were a few:

* search_struct_field and check_field did not call check_typedef, so if
  a base class was a stub, they failed.

* cp_print_value had the wrong sanity check for whether the virtual base
  offset ran off the end of the enclosing object.  This bug was further
  obscured by the fact that the wrong enclosing type was passed in to
  this function in some cases.

* cp_print_value was also looking for the baseclass offset using the
  wrong inferior address.

* The Python code failed to offset the address as well as the contents
  by the embedded offset.


While doing this I ran into some oddities.

common_val_print passes in the embedded_offset from the value it
unpacks, but it doesn't pass in the enclosing class.  I looked at fixing
this, either by adding the enclosing class to the whole hierarchy, or by
changing the whole hierarchy to pass around a value plus a type+offset
representing the current subobject to print.  Both of these were bigger
changes than I wanted to make; but I do think that eventually we ought
to do the latter.

I found some confusion in the code as to the meaning and use of the
various val_print arguments.  So, I documented them to the best of my
knowledge in language.h.  There are probably still calls which are
wrong, I did not audit them all.

I introduced a new cp_print_value_fields_rtti to get the correct
enclosing class.  This has a funny side effect -- it calls
value_rtti_type, which can modify the argument type, adding a
c++-specific structure to it.  This in turn means that a type that
formerly printed as "struct x" will now print as "class x".  This showed
up in the test suite as the userdef.exp change.

I looked at this code a little, too.  AFAICT none of the DECLARED_TYPE_*
constants are ever written to a C++ structure, and I think both
DECLARED_TYPE_UNION and DECLARED_TYPE_TEMPLATE are useless as well.  In
this case I chose to change userdef.exp and then later look into
changing the DWARF reader to respect DW_TAG_class_type.

Built and regtested on x86-64 (compile farm).  No regressions.

Tom


2010-01-29  Tom Tromey  <tromey@redhat.com>

	PR c++/11226, PR c++/9629, PR c++/9688, PR c++/8890:
	* valops.c (search_struct_field): Compute nbases after calling
	CHECK_TYPEDEF.
	(check_field): Call CHECK_TYPEDEF.
	* cp-valprint.c (cp_print_value): Pass correct address to
	baseclass_offset.  Fix check for virtual base past the end of the
	object.  Don't offset address passed to cp_print_value_fields or
	apply_val_pretty_printer.
	(cp_print_value_fields): Fix call to val_print.
	(cp_print_value_fields_rtti): New function.
	* c-valprint.c (c_val_print): Use cp_print_value_fields_rtti.
	* p-valprint.c (pascal_object_print_value_fields): Fix call to
	val_print.
	* python/py-prettyprint.c (apply_val_pretty_printer): Add embedded
	offset to address.
	* language.h (struct language_defn) <la_val_print>: Document.
	* c-lang.h (cp_print_value_fields_rtti): Declare.

2010-01-29  Tom Tromey  <tromey@redhat.com>

	PR c++/11226, PR c++/9629, PR c++/9688, PR c++/8890:
	* gdb.cp/virtbase.cc: New file.
	* gdb.cp/virtbase.exp: New file.
	* gdb.cp/userdef.exp: Allow 'struct' or 'class'.

Index: c-lang.h
===================================================================
RCS file: /cvs/src/src/gdb/c-lang.h,v
retrieving revision 1.26
diff -u -r1.26 c-lang.h
--- c-lang.h	14 Jan 2010 08:03:35 -0000	1.26
+++ c-lang.h	29 Jan 2010 16:16:46 -0000
@@ -102,6 +102,12 @@
 				   const struct value_print_options *,
 				   struct type **, int);
 
+extern void cp_print_value_fields_rtti (struct type *,
+					const gdb_byte *, int, CORE_ADDR,
+					struct ui_file *, int,
+					const struct value_print_options *,
+					struct type **, int);
+
 extern int cp_is_vtbl_ptr_type (struct type *);
 
 extern int cp_is_vtbl_member (struct type *);
Index: c-valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/c-valprint.c,v
retrieving revision 1.66
diff -u -r1.66 c-valprint.c
--- c-valprint.c	25 Jan 2010 19:31:23 -0000	1.66
+++ c-valprint.c	29 Jan 2010 16:16:46 -0000
@@ -389,8 +389,9 @@
 					  options->addressprint);
 	}
       else
-	cp_print_value_fields (type, type, valaddr, embedded_offset, address, stream,
-			       recurse, options, NULL, 0);
+	cp_print_value_fields_rtti (type, valaddr,
+				    embedded_offset, address, stream,
+				    recurse, options, NULL, 0);
       break;
 
     case TYPE_CODE_ENUM:
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.233
diff -u -r1.233 valops.c
--- valops.c	26 Jan 2010 16:47:34 -0000	1.233
+++ valops.c	29 Jan 2010 16:16:46 -0000
@@ -1787,9 +1787,10 @@
 		     struct type *type, int looking_for_baseclass)
 {
   int i;
-  int nbases = TYPE_N_BASECLASSES (type);
+  int nbases;
 
   CHECK_TYPEDEF (type);
+  nbases = TYPE_N_BASECLASSES (type);
 
   if (!looking_for_baseclass)
     for (i = TYPE_NFIELDS (type) - 1; i >= nbases; i--)
@@ -2763,6 +2764,9 @@
 {
   int i;
 
+  /* The type may be a stub.  */
+  CHECK_TYPEDEF (type);
+
   for (i = TYPE_NFIELDS (type) - 1; i >= TYPE_N_BASECLASSES (type); i--)
     {
       char *t_field_name = TYPE_FIELD_NAME (type, i);
Index: cp-valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-valprint.c,v
retrieving revision 1.62
diff -u -r1.62 cp-valprint.c
--- cp-valprint.c	1 Jan 2010 07:31:30 -0000	1.62
+++ cp-valprint.c	29 Jan 2010 16:16:46 -0000
@@ -295,7 +295,7 @@
 		  opts.deref_ref = 0;
 		  val_print (TYPE_FIELD_TYPE (type, i),
 			     valaddr, offset + TYPE_FIELD_BITPOS (type, i) / 8,
-			     address + TYPE_FIELD_BITPOS (type, i) / 8,
+			     address,
 			     stream, recurse + 1, &opts,
 			     current_language);
 		}
@@ -321,6 +321,39 @@
   fprintf_filtered (stream, "}");
 }
 
+/* Like cp_print_value_fields, but find the runtime type of the object
+   and pass it as the `real_type' argument to cp_print_value_fields.
+   This function is a hack to work around the fact that
+   common_val_print passes the embedded offset to val_print, but not
+   the enclosing type.  */
+
+void
+cp_print_value_fields_rtti (struct type *type,
+			    const gdb_byte *valaddr, int offset,
+			    CORE_ADDR address,
+			    struct ui_file *stream, int recurse,
+			    const struct value_print_options *options,
+			    struct type **dont_print_vb, int dont_print_statmem)
+{
+  struct value *value;
+  int full, top, using_enc;
+  struct type *real_type;
+
+  /* Ugh, we have to convert back to a value here.  */
+  value = value_from_contents_and_address (type, valaddr + offset,
+					   address + offset);
+  /* We don't actually care about most of the result here -- just the
+     type.  We already have the correct offset, due to how val_print
+     was initially called.  */
+  real_type = value_rtti_type (value, &full, &top, &using_enc);
+  if (!real_type)
+    real_type = type;
+
+  cp_print_value_fields (type, real_type, valaddr, offset,
+			 address, stream, recurse, options,
+			 dont_print_vb, dont_print_statmem);
+}
+
 /* Special val_print routine to avoid printing multiple copies of virtual
    baseclasses.  */
 
@@ -373,7 +406,7 @@
       thisoffset = offset;
       thistype = real_type;
 
-      boffset = baseclass_offset (type, i, valaddr + offset, address);
+      boffset = baseclass_offset (type, i, valaddr + offset, address + offset);
       skip = ((boffset == -1) || (boffset + offset) < 0) ? 1 : -1;
 
       if (BASETYPE_VIA_VIRTUAL (type, i))
@@ -384,7 +417,7 @@
 
 	  if (boffset != -1
 	      && ((boffset + offset) < 0
-		  || (boffset + offset) >= TYPE_LENGTH (type)))
+		  || (boffset + offset) >= TYPE_LENGTH (real_type)))
 	    {
 	      /* FIXME (alloca): unsafe if baseclass is really really large. */
 	      gdb_byte *buf = alloca (TYPE_LENGTH (baseclass));
@@ -427,14 +460,14 @@
 	  if (!options->raw)
 	    result = apply_val_pretty_printer (baseclass, base_valaddr,
 					       thisoffset + boffset,
-					       address + boffset,
+					       address,
 					       stream, recurse,
 					       options,
 					       current_language);
 	  	  
 	  if (!result)
 	    cp_print_value_fields (baseclass, thistype, base_valaddr,
-				   thisoffset + boffset, address + boffset,
+				   thisoffset + boffset, address,
 				   stream, recurse, options,
 				   ((struct type **)
 				    obstack_base (&dont_print_vb_obstack)),
@@ -501,7 +534,8 @@
 		    sizeof (CORE_ADDR));
 
       CHECK_TYPEDEF (type);
-      cp_print_value_fields (type, type, value_contents_all (val),
+      cp_print_value_fields (type, value_enclosing_type (val),
+			     value_contents_all (val),
 			     value_embedded_offset (val), addr,
 			     stream, recurse, options, NULL, 1);
       return;
Index: language.h
===================================================================
RCS file: /cvs/src/src/gdb/language.h,v
retrieving revision 1.62
diff -u -r1.62 language.h
--- language.h	14 Jan 2010 08:03:36 -0000	1.62
+++ language.h	29 Jan 2010 16:16:46 -0000
@@ -208,11 +208,32 @@
     void (*la_print_typedef) (struct type *type, struct symbol *new_symbol,
 			      struct ui_file *stream);
 
-    /* Print a value using syntax appropriate for this language. */
+    /* Print a value using syntax appropriate for this language.
+       
+       TYPE is the type of the sub-object to be printed.
 
-    int (*la_val_print) (struct type *, const gdb_byte *, int, CORE_ADDR,
-			 struct ui_file *, int,
-			 const struct value_print_options *);
+       CONTENTS holds the bits of the value.  This holds the entire
+       enclosing object.
+
+       EMBEDDED_OFFSET is the offset into the outermost object of the
+       sub-object represented by TYPE.  This is the object which this
+       call should print.  Note that the enclosing type is not
+       available.
+
+       ADDRESS is the address in the inferior of the enclosing object.
+
+       STREAM is the stream on which the value is to be printed.
+
+       RECURSE is the recursion depth.  It is zero-based.
+
+       OPTIONS are the formatting options to be used when
+       printing.  */
+
+    int (*la_val_print) (struct type *type,
+			 const gdb_byte *contents,
+			 int embedded_offset, CORE_ADDR address,
+			 struct ui_file *stream, int recurse,
+			 const struct value_print_options *options);
 
     /* Print a top-level value using syntax appropriate for this language. */
 
Index: python/py-prettyprint.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-prettyprint.c,v
retrieving revision 1.4
diff -u -r1.4 py-prettyprint.c
--- python/py-prettyprint.c	14 Jan 2010 08:03:37 -0000	1.4
+++ python/py-prettyprint.c	29 Jan 2010 16:16:46 -0000
@@ -538,7 +538,8 @@
   /* Instantiate the printer.  */
   if (valaddr)
     valaddr += embedded_offset;
-  value = value_from_contents_and_address (type, valaddr, address);
+  value = value_from_contents_and_address (type, valaddr,
+					   address + embedded_offset);
 
   val_obj = value_to_value_object (value);
   if (! val_obj)
Index: testsuite/gdb.cp/userdef.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/userdef.exp,v
retrieving revision 1.12
diff -u -r1.12 userdef.exp
--- testsuite/gdb.cp/userdef.exp	1 Jan 2010 07:32:02 -0000	1.12
+++ testsuite/gdb.cp/userdef.exp	29 Jan 2010 16:16:48 -0000
@@ -153,7 +153,7 @@
 gdb_test "print c" "\\\$\[0-9\]* = {m = {z = .*}}"
 gdb_test "print *c" "\\\$\[0-9\]* = \\(Member &\\) @$hex: {z = .*}"
 gdb_test "print &*c" "\\\$\[0-9\]* = \\(Member \\*\\) $hex"
-gdb_test "ptype &*c" "type = struct Member {\[\r\n \]+int z;\[\r\n\]+} &\\*"
+gdb_test "ptype &*c" "type = (struct|class) Member {(\[\r\n \]+public:)?\[\r\n \]+int z;\[\r\n\]+} &\\*"
 
 gdb_exit
 return 0
Index: testsuite/gdb.cp/virtbase.cc
===================================================================
RCS file: testsuite/gdb.cp/virtbase.cc
diff -N testsuite/gdb.cp/virtbase.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/virtbase.cc	29 Jan 2010 16:16:48 -0000
@@ -0,0 +1,57 @@
+// This first batch of classes are for PR 11226.
+namespace mc {
+  class Base {
+  protected:
+    int x;
+    
+  public:
+    Base(void) { x = 2; };
+  };
+}
+
+namespace ph {
+  class Middle: public virtual mc::Base {
+  protected:
+    int y;
+    
+  public:
+    Middle(void): mc::Base() { y = 3; };
+
+    int get_y(void)
+    {
+      return y;			// breakpoint 1
+    };
+  };
+
+  class Derived: public virtual Middle {
+  protected:
+    int z;
+    
+  public:
+    Derived(void): Middle() { z = 4; };
+
+    int get_z(void)
+    {
+      return z;			// breakpoint 2
+    };
+  };
+}
+
+// These classes are for PR 9629.
+struct A {};
+struct B : virtual A {};
+
+struct C {int v; C() {v=11;};};
+struct D:virtual C{};
+
+class E:B,D{};
+
+int main() {
+  ph::Derived tst;
+  tst.get_y();
+  tst.get_z();
+
+  E *e = new E;
+
+  return 0;			// breakpoint 3
+}
Index: testsuite/gdb.cp/virtbase.exp
===================================================================
RCS file: testsuite/gdb.cp/virtbase.exp
diff -N testsuite/gdb.cp/virtbase.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/virtbase.exp	29 Jan 2010 16:16:48 -0000
@@ -0,0 +1,57 @@
+# Copyright 2010 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.
+
+if { [skip_cplus_tests] } { continue }
+
+set testfile "virtbase"
+set srcfile ${testfile}.cc
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug c++}] != "" } {
+     untested virtbase.exp
+     return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if {![runto_main]} then {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+gdb_breakpoint [gdb_get_line_number "breakpoint 1"]
+gdb_continue_to_breakpoint "first breakpoint"
+
+# In PR 11226, we failed to print x correctly in the "print *this"
+# case.
+gdb_test "print *this" " = {<mc::Base> = {x = 2}, _vptr.Middle = $hex, y = 3}"
+gdb_test "print x" " = 2"
+
+gdb_breakpoint [gdb_get_line_number "breakpoint 2"]
+gdb_continue_to_breakpoint "second breakpoint"
+
+# In PR 11226, we could not find x here.
+gdb_test "print x" " = 2"
+
+gdb_breakpoint [gdb_get_line_number "breakpoint 3"]
+gdb_continue_to_breakpoint "third breakpoint"
+
+# In PR 9629, we failed to print v correctly here.
+gdb_test "print *(D *) e" " = {<C> = {v = 11}, _vptr.D = $hex}"