[RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Doug Evans]

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

I think(!) this patch is reasonable.  It seems to be in line with what
Jim has in mind
(based on my interpretation of your comments Jim on the gdb list -
please correct
me if I'm wrong of course).

When gdb resolves type information for class "derived" from objfile
gdb2384 (from the testcase), it fills 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.

Ok to check in?  Or any suggestions for what's needed instead?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-071213-2384-1.patch --]
[-- Type: text/x-patch; name=gdb-071213-2384-1.patch, Size: 5659 bytes --]

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

	PR 2384
	* gdbtypes.c (fill_in_vptr_fieldno): Don't set TYPE_VPTR_FIELDNO,
	TYPE_VPTR_BASETYPE if from different objfile.

	* 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.140
diff -u -p -u -p -r1.140 gdbtypes.c
--- gdbtypes.c	4 Dec 2007 23:33:00 -0000	1.140
+++ gdbtypes.c	13 Dec 2007 23:59:35 -0000
@@ -1307,8 +1307,13 @@ fill_in_vptr_fieldno (struct type *type)
 	  fill_in_vptr_fieldno (baseclass);
 	  if (TYPE_VPTR_FIELDNO (baseclass) >= 0)
 	    {
-	      TYPE_VPTR_FIELDNO (type) = TYPE_VPTR_FIELDNO (baseclass);
-	      TYPE_VPTR_BASETYPE (type) = TYPE_VPTR_BASETYPE (baseclass);
+	      /* If the type comes from a different objfile we can't use it
+		 here, it may have a different lifetime. PR 2384 */
+	      if (TYPE_OBJFILE (type) == TYPE_OBJFILE (baseclass))
+		{
+		  TYPE_VPTR_FIELDNO (type) = TYPE_VPTR_FIELDNO (baseclass);
+		  TYPE_VPTR_BASETYPE (type) = TYPE_VPTR_BASETYPE (baseclass);
+		}
 	      break;
 	    }
 	}
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	13 Dec 2007 23:59:35 -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	13 Dec 2007 23:59:35 -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	13 Dec 2007 23:59:35 -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	13 Dec 2007 23:59:35 -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"

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Daniel Jacobowitz]

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.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Doug Evans]

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.

For the test case in the patch, type info for the derived class is left
as opaque/stub, and when the vptr is needed check_typedef is called
which notices the fact that is has an incomplete type and does a lookup
by name to get the complete typo info.  In this case check_typedef
notices it has a cross-objfile reference and returns a pointer to the
complete type info without updating the original type. This way cross-objfile
pointers don't get created and the required data is still obtained when needed.

There are other use cases.  If base and derived are both in the
same compilation unit then dwarf2read.c will fill in
TYPE_VPTR_{BASETYPE,FIELDNO} (and presumably stabsread.c
but I don't know offhand), and fill_in_vptr_fieldno is a nop.

Another use case is if base and derived are in different compilation
units but in the same objfile.  Here dwarf2read.c will leave the base
class as a stub for derived's type info.  Later when vptr is needed
fill_in_vptr_fieldno is called and it will notice fieldno < 0 and call
check_typedef on the base type.  check_typedef will see that
orig_type and newtype are in the same objfile and will call
make_cv_type to update the original type with the complete
type info.

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

Clearly fill_in_vptr_fieldno can't create cross-objfile pointers.
All callers of fill_in_vptr_fieldno (should) expect that it may fail.

I'm not quite sure what you're suggesting, as it relates to the patch.
The v3 abi still needs logic to handle cross-objfile references,
though it could be done without the use of fill_in_vptr_fieldno.

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Doug Evans]

One improvement might be to add more comments at the change site
to clarify the situation.   e.g. when fill_in_vptr_fieldno does useful work
and when it doesn't.  [assuming fill_in_vptr_fieldno remains ... it's not
clear to me whether the ultimate fix will remove it]

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Doug Evans]

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.

Hold the fort, I think I see what you're saying now.
I'll send another patch for review.

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Doug Evans]

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

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?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-071220-2384-3.patch --]
[-- Type: text/x-patch; name=gdb-071220-2384-3.patch, Size: 15656 bytes --]

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"

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Doug Evans]

Ping ...

On Dec 20, 2007 11:40 AM, Doug Evans <dje@google.com> wrote:
> 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?
>

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Daniel Jacobowitz]

On Thu, Dec 20, 2007 at 11:40:06AM -0800, Doug Evans wrote:
> 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.

Thanks, this looks OK.  Could you please add copyright notices to the
new test files before committing?

> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}
> +#gdb_load_shlibs ${sofile}

Why's that commented out?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] patch for 2384, dangling TYPE_VPTR_BASETYPE

[Doug Evans]

On Jan 29, 2008 5:06 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Thu, Dec 20, 2007 at 11:40:06AM -0800, Doug Evans wrote:
> > 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.
>
> Thanks, this looks OK.  Could you please add copyright notices to the
> new test files before committing?
>
> > +gdb_start
> > +gdb_reinitialize_dir $srcdir/$subdir
> > +gdb_load ${binfile}
> > +#gdb_load_shlibs ${sofile}
>
> Why's that commented out?

Thanks.

Commenting out gdb_load_shlibs was probably a leftover from some
experimentation - I put it back.