Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

["Doug Evans" <dje at google dot com>]

On Dec 13, 2007 4:29 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Thu, Dec 13, 2007 at 04:10:55PM -0800, Doug Evans wrote:
> > Ok to check in?  Or any suggestions for what's needed instead?
>
> Your patch seems strange to me.  Do we need the new fieldno /
> basetype, or not?  If we don't, we shouldn't be calculating it at all;
> if we do, there should be something detectable which breaks when you
> do this.  It's not just a cache, since the interface doesn't offer any
> other way to return the new fieldno / basetype besides in-place
> modification.
>
> I happen to know that for GNU v3 - which is in practice the only thing
> that any GDB users use nowadays - we don't need these fields any more.
> We still use them, but we could do without, since the ABI is quite
> clear on where to find the vtable pointer.
>
> For GNU v2, which is theoretically still supported, we do need this
> information.

Silly me.  How about this?

2007-12-20  Doug Evans  <dje@google.com>

	PR 2384
	* gdbtypes.c (get_vptr_fieldno): Renamed from fill_in_vptr_fieldno.
	Return basetype, fieldno if found.  All callers updated.
	Don't cache TYPE_VPTR_FIELDNO, TYPE_VPTR_BASETYPE if from different
	objfile.
	* gdbtypes.h (get_vptr_fieldno): Renamed from fill_in_vptr_fieldno.
	* symfile.h (fill_in_vptr_fieldno): Delete.

	* gdb.cp/gdb2384.exp: New file.
	* gdb.cp/gdb2384.cc: New file.
	* gdb.cp/gdb2384-base.h: New file.
	* gdb.cp/gdb2384-base.cc: New file.

Index: gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.141
diff -u -p -u -p -r1.141 gdbtypes.c
--- gdbtypes.c	20 Dec 2007 17:17:21 -0000	1.141
+++ gdbtypes.c	20 Dec 2007 19:25:59 -0000
@@ -1282,15 +1282,19 @@ lookup_struct_elt_type (struct type *typ
   return (struct type *) -1;	/* For lint */
 }
 
-/* If possible, make the vptr_fieldno and vptr_basetype fields of TYPE
-   valid.  Callers should be aware that in some cases (for example,
+/* Lookup the vptr basetype/fieldno values for TYPE.
+   If found store vptr_basetype in *BASETYPEP if non-NULL, and return
+   vptr_fieldno.  Also, if found and basetype is from the same objfile,
+   cache the results.
+   If not found, return -1 and ignore BASETYPEP.
+   Callers should be aware that in some cases (for example,
    the type or one of its baseclasses is a stub type and we are
    debugging a .o file), this function will not be able to find the
    virtual function table pointer, and vptr_fieldno will remain -1 and
-   vptr_basetype will remain NULL.  */
+   vptr_basetype will remain NULL or incomplete.  */
 
-void
-fill_in_vptr_fieldno (struct type *type)
+int
+get_vptr_fieldno (struct type *type, struct type **basetypep)
 {
   CHECK_TYPEDEF (type);
 
@@ -1302,16 +1306,34 @@ fill_in_vptr_fieldno (struct type *type)
          is virtual (and hence we cannot share the table pointer).  */
       for (i = 0; i < TYPE_N_BASECLASSES (type); i++)
 	{
-	  struct type *baseclass = check_typedef (TYPE_BASECLASS (type,
-								  i));
-	  fill_in_vptr_fieldno (baseclass);
-	  if (TYPE_VPTR_FIELDNO (baseclass) >= 0)
+	  struct type *baseclass = check_typedef (TYPE_BASECLASS (type, i));
+	  int fieldno;
+	  struct type *basetype;
+
+	  fieldno = get_vptr_fieldno (baseclass, &basetype);
+	  if (fieldno >= 0)
 	    {
-	      TYPE_VPTR_FIELDNO (type) = TYPE_VPTR_FIELDNO (baseclass);
-	      TYPE_VPTR_BASETYPE (type) = TYPE_VPTR_BASETYPE (baseclass);
-	      break;
+	      /* If the type comes from a different objfile we can't cache
+		 it, it may have a different lifetime. PR 2384 */
+	      if (TYPE_OBJFILE (type) == TYPE_OBJFILE (baseclass))
+		{
+		  TYPE_VPTR_FIELDNO (type) = fieldno;
+		  TYPE_VPTR_BASETYPE (type) = basetype;
+		}
+	      if (basetypep)
+		*basetypep = basetype;
+	      return fieldno;
 	    }
 	}
+
+      /* Not found.  */
+      return -1;
+    }
+  else
+    {
+      if (basetypep)
+	*basetypep = TYPE_VPTR_BASETYPE (type);
+      return TYPE_VPTR_FIELDNO (type);
     }
 }
 
Index: gdbtypes.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.h,v
retrieving revision 1.82
diff -u -p -u -p -r1.82 gdbtypes.h
--- gdbtypes.h	4 Dec 2007 23:33:00 -0000	1.82
+++ gdbtypes.h	20 Dec 2007 19:25:59 -0000
@@ -375,7 +375,9 @@ struct main_type
   /* Field number of the virtual function table pointer in
      VPTR_BASETYPE.  If -1, we were unable to find the virtual
      function table pointer in initial symbol reading, and
-     fill_in_vptr_fieldno should be called to find it if possible.
+     get_vptr_fieldno should be called to find it if possible.
+     get_vptr_fieldno will update this field if possible.
+     Otherwise the value is left at -1.
 
      Unused if this type does not have virtual functions.  */
 
@@ -1274,7 +1276,7 @@ extern struct type *lookup_typename (cha
 extern struct type *lookup_template_type (char *, struct type *,
 					  struct block *);
 
-extern void fill_in_vptr_fieldno (struct type *);
+extern int get_vptr_fieldno (struct type *, struct type **);
 
 extern int get_destructor_fn_field (struct type *, int *, int *);
 
Index: symfile.h
===================================================================
RCS file: /cvs/src/src/gdb/symfile.h,v
retrieving revision 1.43
diff -u -p -u -p -r1.43 symfile.h
--- symfile.h	22 Oct 2007 01:16:34 -0000	1.43
+++ symfile.h	20 Dec 2007 19:25:59 -0000
@@ -207,8 +207,6 @@ extern struct symtab *allocate_symtab (c
 
 extern int free_named_symtabs (char *);
 
-extern void fill_in_vptr_fieldno (struct type *);
-
 extern void add_symtab_fns (struct sym_fns *);
 
 extern void syms_from_objfile (struct objfile *,
Index: gnu-v2-abi.c
===================================================================
RCS file: /cvs/src/src/gdb/gnu-v2-abi.c,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 gnu-v2-abi.c
--- gnu-v2-abi.c	5 Sep 2007 00:07:07 -0000	1.26
+++ gnu-v2-abi.c	20 Dec 2007 19:25:59 -0000
@@ -88,8 +88,6 @@ gnuv2_virtual_fn_field (struct value **a
 {
   struct value *arg1 = *arg1p;
   struct type *type1 = check_typedef (value_type (arg1));
-
-
   struct type *entry_type;
   /* First, get the virtual function table pointer.  That comes
      with a strange type, so cast it to type `pointer to long' (which
@@ -102,6 +100,9 @@ gnuv2_virtual_fn_field (struct value **a
 				     (LONGEST) TYPE_FN_FIELD_VOFFSET (f, j));
   struct type *fcontext = TYPE_FN_FIELD_FCONTEXT (f, j);
   struct type *context;
+  struct type *context_vptr_basetype;
+  int context_vptr_fieldno;
+
   if (fcontext == NULL)
     /* We don't have an fcontext (e.g. the program was compiled with
        g++ version 1).  Try to get the vtbl from the TYPE_VPTR_BASETYPE.
@@ -123,13 +124,13 @@ gnuv2_virtual_fn_field (struct value **a
   /* This type may have been defined before its virtual function table
      was.  If so, fill in the virtual function table entry for the
      type now.  */
-  if (TYPE_VPTR_FIELDNO (context) < 0)
-    fill_in_vptr_fieldno (context);
+  context_vptr_fieldno = get_vptr_fieldno (context, &context_vptr_basetype);
+  /* FIXME: What to do if vptr_fieldno is still -1?  */
 
   /* The virtual function table is now an array of structures
      which have the form { int16 offset, delta; void *pfn; }.  */
-  vtbl = value_primitive_field (arg1, 0, TYPE_VPTR_FIELDNO (context),
-				TYPE_VPTR_BASETYPE (context));
+  vtbl = value_primitive_field (arg1, 0, context_vptr_fieldno,
+				context_vptr_basetype);
 
   /* With older versions of g++, the vtbl field pointed to an array
      of structures.  Nowadays it points directly to the structure. */
@@ -194,6 +195,8 @@ gnuv2_value_rtti_type (struct value *v, 
   struct symbol *sym;
   char *demangled_name, *p;
   struct type *btype;
+  struct type *known_type_vptr_basetype;
+  int known_type_vptr_fieldno;
 
   if (full)
     *full = 0;
@@ -214,18 +217,18 @@ gnuv2_value_rtti_type (struct value *v, 
      the type info functions, which are always right.  Deal with it
      until then.  */
 
-  /* If the type has no vptr fieldno, try to get it filled in */
-  if (TYPE_VPTR_FIELDNO(known_type) < 0)
-    fill_in_vptr_fieldno(known_type);
+  /* Try to get the vptr basetype, fieldno.  */
+  known_type_vptr_fieldno = get_vptr_fieldno (known_type,
+					      &known_type_vptr_basetype);
 
-  /* If we still can't find one, give up */
-  if (TYPE_VPTR_FIELDNO(known_type) < 0)
+  /* If we can't find it, give up.  */
+  if (known_type_vptr_fieldno < 0)
     return NULL;
 
   /* Make sure our basetype and known type match, otherwise, cast
      so we can get at the vtable properly.
   */
-  btype = TYPE_VPTR_BASETYPE (known_type);
+  btype = known_type_vptr_basetype;
   CHECK_TYPEDEF (btype);
   if (btype != known_type )
     {
@@ -238,10 +241,10 @@ gnuv2_value_rtti_type (struct value *v, 
     we'd waste a bunch of time figuring out we already know the type.
     Besides, we don't care about the type, just the actual pointer
   */
-  if (VALUE_ADDRESS (value_field (v, TYPE_VPTR_FIELDNO (known_type))) == 0)
+  if (VALUE_ADDRESS (value_field (v, known_type_vptr_fieldno)) == 0)
     return NULL;
 
-  vtbl=value_as_address(value_field(v,TYPE_VPTR_FIELDNO(known_type)));
+  vtbl = value_as_address (value_field (v, known_type_vptr_fieldno));
 
   /* Try to find a symbol that is the vtable */
   minsym=lookup_minimal_symbol_by_pc(vtbl);
Index: gnu-v3-abi.c
===================================================================
RCS file: /cvs/src/src/gdb/gnu-v3-abi.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 gnu-v3-abi.c
--- gnu-v3-abi.c	7 Nov 2007 06:53:41 -0000	1.38
+++ gnu-v3-abi.c	20 Dec 2007 19:26:00 -0000
@@ -201,6 +201,8 @@ gnuv3_rtti_type (struct value *value,
   struct type *run_time_type;
   struct type *base_type;
   LONGEST offset_to_top;
+  struct type *values_type_vptr_basetype;
+  int values_type_vptr_fieldno;
 
   /* We only have RTTI for class objects.  */
   if (TYPE_CODE (values_type) != TYPE_CODE_CLASS)
@@ -208,8 +210,9 @@ gnuv3_rtti_type (struct value *value,
 
   /* If we can't find the virtual table pointer for values_type, we
      can't find the RTTI.  */
-  fill_in_vptr_fieldno (values_type);
-  if (TYPE_VPTR_FIELDNO (values_type) == -1)
+  values_type_vptr_fieldno = get_vptr_fieldno (values_type,
+					       &values_type_vptr_basetype);
+  if (values_type_vptr_fieldno == -1)
     return NULL;
 
   if (using_enc_p)
@@ -217,7 +220,7 @@ gnuv3_rtti_type (struct value *value,
 
   /* Fetch VALUE's virtual table pointer, and tweak it to point at
      an instance of our imaginary gdb_gnu_v3_abi_vtable structure.  */
-  base_type = check_typedef (TYPE_VPTR_BASETYPE (values_type));
+  base_type = check_typedef (values_type_vptr_basetype);
   if (values_type != base_type)
     {
       value = value_cast (base_type, value);
@@ -225,7 +228,7 @@ gnuv3_rtti_type (struct value *value,
 	*using_enc_p = 1;
     }
   vtable_address
-    = value_as_address (value_field (value, TYPE_VPTR_FIELDNO (values_type)));
+    = value_as_address (value_field (value, values_type_vptr_fieldno));
   vtable = value_at_lazy (vtable_type,
                           vtable_address - vtable_address_point_offset ());
   
@@ -381,6 +384,7 @@ gnuv3_baseclass_offset (struct type *typ
   struct value *offset_val, *vbase_array;
   CORE_ADDR vtable_address;
   long int cur_base_offset, base_offset;
+  int vbasetype_vptr_fieldno;
 
   /* If it isn't a virtual base, this is easy.  The offset is in the
      type definition.  */
@@ -414,11 +418,10 @@ gnuv3_baseclass_offset (struct type *typ
      we have debugging information for that baseclass.  */
 
   vbasetype = TYPE_VPTR_BASETYPE (type);
-  if (TYPE_VPTR_FIELDNO (vbasetype) < 0)
-    fill_in_vptr_fieldno (vbasetype);
+  vbasetype_vptr_fieldno = get_vptr_fieldno (vbasetype, NULL);
 
-  if (TYPE_VPTR_FIELDNO (vbasetype) >= 0
-      && TYPE_FIELD_BITPOS (vbasetype, TYPE_VPTR_FIELDNO (vbasetype)) != 0)
+  if (vbasetype_vptr_fieldno >= 0
+      && TYPE_FIELD_BITPOS (vbasetype, vbasetype_vptr_fieldno) != 0)
     error (_("Illegal vptr offset in class %s"),
 	   TYPE_NAME (vbasetype) ? TYPE_NAME (vbasetype) : "<unknown>");
 
Index: testsuite/gdb.cp/gdb2384-base.cc
===================================================================
RCS file: testsuite/gdb.cp/gdb2384-base.cc
diff -N testsuite/gdb.cp/gdb2384-base.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/gdb2384-base.cc	20 Dec 2007 19:26:00 -0000
@@ -0,0 +1,12 @@
+#include "gdb2384-base.h"
+
+base::base (int _x)
+  : x (_x)
+{
+}
+
+int
+base::meth ()
+{
+  return x;
+}
Index: testsuite/gdb.cp/gdb2384-base.h
===================================================================
RCS file: testsuite/gdb.cp/gdb2384-base.h
diff -N testsuite/gdb.cp/gdb2384-base.h
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/gdb2384-base.h	20 Dec 2007 19:26:00 -0000
@@ -0,0 +1,7 @@
+class base
+{
+ public:
+  base (int _x);
+  int x;
+  virtual int meth ();
+};
Index: testsuite/gdb.cp/gdb2384.cc
===================================================================
RCS file: testsuite/gdb.cp/gdb2384.cc
diff -N testsuite/gdb.cp/gdb2384.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/gdb2384.cc	20 Dec 2007 19:26:00 -0000
@@ -0,0 +1,22 @@
+#include "gdb2384-base.h"
+
+class derived : public base
+{
+ public:
+  derived (int);
+};
+
+derived::derived (int _x)
+  : base (_x)
+{
+}
+
+int g;
+
+int
+main ()
+{
+  derived d (42);
+  g = d.meth (); // set breakpoint here
+  return 0;
+}
Index: testsuite/gdb.cp/gdb2384.exp
===================================================================
RCS file: testsuite/gdb.cp/gdb2384.exp
diff -N testsuite/gdb.cp/gdb2384.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/gdb2384.exp	20 Dec 2007 19:26:00 -0000
@@ -0,0 +1,99 @@
+# Copyright 2007 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/>.
+
+# When gdb resolves type information for class "derived" from objfile
+# gdb2384, it use to fill in the TYPE_VPTR_BASETYPE field with class "base"
+# from objfile gdb2384-base.so.  When the program is rerun the type
+# information for base-in-so-base.so is discarded leaving
+# TYPE_VPTR_BASETYPE dangling.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+if { [skip_cplus_tests] } { continue }
+
+set prms_id 2384
+set bug_id 0
+
+set testfile "gdb2384"
+set srcfile ${testfile}.cc
+set binfile $objdir/$subdir/$testfile
+
+set libfile "gdb2384-base"
+set libsrcfile ${libfile}.cc
+set sofile $objdir/$subdir/${libfile}.so
+
+# Create and source the file that provides information about the compiler
+# used to compile the test case.
+if [get_compiler_info ${binfile} "c++"] {
+    return -1
+}
+
+if { [gdb_compile_shlib $srcdir/$subdir/$libsrcfile $sofile {debug c++}] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable [list debug "c++" shlib=${sofile}]] != ""} {
+    untested gdb2384.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+#gdb_load_shlibs ${sofile}
+
+set bp_location [gdb_get_line_number "set breakpoint here"]
+
+# Set a breakpoint with multiple locations.
+
+gdb_test "break $srcfile:$bp_location" \
+    "Breakpoint.*at.* file .*$srcfile, line.*" \
+    "set breakpoint"
+
+gdb_run_cmd
+gdb_expect {
+    -re "Breakpoint \[0-9\]+,.*main \\(.*\\).*$gdb_prompt $" {
+	pass "run to breakpoint"
+    }
+    -re "$gdb_prompt $" {
+	fail "run to breakpoint"
+    }
+    timeout {
+	fail "run to breakpoint (timeout)"
+    }
+}
+
+gdb_test "print d.meth ()" \
+    ".*42.*" \
+    "print d.meth ()"
+
+# Now try again.  gdb's without the fix will hopefully segv here
+
+gdb_run_cmd
+gdb_expect {
+    -re "Breakpoint \[0-9\]+,.*main \\(.*\\).*$gdb_prompt $" {
+	pass "run to breakpoint #2"
+    }
+    -re "$gdb_prompt $" {
+	fail "run to breakpoint #2"
+    }
+    timeout {
+	fail "run to breakpoint #2 (timeout)"
+    }
+}
+
+gdb_test "print d.meth ()" \
+    ".*42.*" \
+    "gdb2384"