[patch] [4/5] Types reference counting [varobj-validation]

[patch] [4/5] Types reference counting [varobj-validation]

[Jan Kratochvil]

Hi,

currently GDB was sometimes keeping varobjs with stale references to objfiles.
With the changes GDB started crashing on this problem.

+/* Invalidate the varobjs that are tied to the specified OBJFILE.  Call this
+   function before you start removing OBJFILE.
+
+   Call varobj_revalidate after the OBJFILEs updates get finished.

As varobj_invalidate was unconditionally invalidating any local varobjs more
often calls to varobj_invalidate could become more intrusive to the user so
I also fixed varobj_invalidate to care only about varobjs bound to the
specific objfile.


Thanks,
Jan


gdb/
2009-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Split varobj_invalidate into a two-phased operation.
	* objfiles.c: Include varobj.h
	(free_objfile): Call varobj_invalidate.
	* symfile.c (new_symfile_objfile): Call varobj_revalidate.
	(reread_symbols): Call varobj_invalidate and varobj_revalidate.
	(clear_symtab_users): No longer call varobj_invalidate.
	* varobj.c (varobj_invalidate): New parameter `objfile', comment it.
	New variable `var'.  Invalidate any varobj related to `objfile'.
	Remove unconditional invalidation of local varobjs.  Move global
	varobjs revalidation to ...
	(varobj_revalidate): ... a new function.
	* varobj.h (varobj_invalidate): Update the prototype.
	(varobj_revalidate): New prototype.

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 795d53b..8e794e0 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -50,6 +50,7 @@
 #include "addrmap.h"
 #include "arch-utils.h"
 #include "exec.h"
+#include "varobj.h"
 
 /* Prototypes for local functions */
 
@@ -409,6 +410,7 @@ free_objfile (struct objfile *objfile)
   /* Remove any references to this objfile in the global value
      lists.  */
   preserve_values (objfile);
+  varobj_invalidate (objfile);
 
   /* First do any symbol file specific actions required when we are
      finished with a particular symbol file.  Note that if the objfile
diff --git a/gdb/symfile.c b/gdb/symfile.c
index af1ab74..54d2bad 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -927,6 +927,8 @@ new_symfile_objfile (struct objfile *objfile, int mainline, int verbo)
 
   /* We're done reading the symbol file; finish off complaints.  */
   clear_complaints (&symfile_complaints, 0, verbo);
+
+  varobj_revalidate ();
 }
 
 /* Process a symbol file, as either the main file or as a dynamically
@@ -2341,6 +2343,7 @@ reread_symbols (void)
 	      /* Remove any references to this objfile in the global
 		 value lists.  */
 	      preserve_values (objfile);
+	      varobj_invalidate (objfile);
 
 	      /* Nuke all the state that we will re-read.  Much of the following
 	         code which sets things to NULL really is necessary to tell
@@ -2437,6 +2440,7 @@ reread_symbols (void)
 	         frameless.  */
 
 	      reinit_frame_cache ();
+	      varobj_revalidate ();
 
 	      /* Discard cleanups as symbol reading was successful.  */
 	      discard_cleanups (old_cleanups);
@@ -2817,10 +2821,6 @@ clear_symtab_users (void)
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
   innermost_block = NULL;
-
-  /* Varobj may refer to old symbols, perform a cleanup.  */
-  varobj_invalidate ();
-
 }
 
 static void
diff --git a/gdb/varobj.c b/gdb/varobj.c
index a7957f6..b60b0a8 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -2748,12 +2748,74 @@ When non-zero, varobj debugging is enabled."),
 			    &setlist, &showlist);
 }
 
-/* Invalidate the varobjs that are tied to locals and re-create the ones that
-   are defined on globals.
+/* Invalidate the varobjs that are tied to the specified OBJFILE.  Call this
+   function before you start removing OBJFILE.
+
+   Call varobj_revalidate after the OBJFILEs updates get finished.
+
    Invalidated varobjs will be always printed in_scope="invalid".  */
 
 void 
-varobj_invalidate (void)
+varobj_invalidate (struct objfile *objfile)
+{
+  struct varobj **all_rootvarobj;
+  struct varobj **varp;
+
+  if (varobj_list (&all_rootvarobj) > 0)
+    {
+      varp = all_rootvarobj;
+      while (*varp != NULL)
+	{
+	  struct varobj *var = *varp;
+
+	  /* Floating varobjs are reparsed on each stop, so we don't care if
+	     the presently parsed expression refers to something that's gone.
+	     */
+	  if (var->root->floating)
+	    continue;
+
+	  if (var->root->valid_block != NULL && var->root->is_valid)
+	    {
+	      struct block *block = var->root->valid_block;
+	      struct symbol *func = block_linkage_function (block);
+	      struct objfile *func_objfile = SYMBOL_SYMTAB (func)->objfile;
+
+	      if (func_objfile == objfile)
+		var->root->is_valid = 0;
+	    }
+	  
+	  if (var->type && TYPE_OBJFILE (var->type) == objfile)
+	    {
+	      if (!var->root->valid_block)
+		var->root->is_valid = 0;
+	      else
+		gdb_assert (!var->root->is_valid);
+
+	      var->type = NULL;
+	    }
+	  if (var->value
+	      && TYPE_OBJFILE (value_type (var->value)) == objfile)
+	    {
+	      if (!var->root->valid_block)
+		var->root->is_valid = 0;
+	      else
+		gdb_assert (!var->root->is_valid);
+
+	      value_free (var->value);
+	      var->value = NULL;
+	    }
+
+	  varp++;
+	}
+    }
+  xfree (all_rootvarobj);
+}
+
+/* Recreate any global varobjs possibly previously invalidated.  If the
+   expressions are no longer evaluatable set/keep the varobj invalid.  */
+
+void 
+varobj_revalidate (void)
 {
   struct varobj **all_rootvarobj;
   struct varobj **varp;
@@ -2763,36 +2825,34 @@ varobj_invalidate (void)
       varp = all_rootvarobj;
       while (*varp != NULL)
 	{
+	  struct varobj *var = *varp;
+
 	  /* Floating varobjs are reparsed on each stop, so we don't care if
 	     the presently parsed expression refers to something that's gone.
 	     */
-	  if ((*varp)->root->floating)
+	  if (var->root->floating)
 	    continue;
 
 	  /* global var must be re-evaluated.  */     
-	  if ((*varp)->root->valid_block == NULL)
+	  if (var->root->valid_block == NULL)
 	    {
 	      struct varobj *tmp_var;
 
 	      /* Try to create a varobj with same expression.  If we succeed
 		 replace the old varobj, otherwise invalidate it.  */
-	      tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0,
-				       USE_CURRENT_FRAME);
+	      tmp_var = varobj_create (NULL, var->name, 0, USE_CURRENT_FRAME);
 	      if (tmp_var != NULL) 
 		{ 
-		  tmp_var->obj_name = xstrdup ((*varp)->obj_name);
-		  varobj_delete (*varp, NULL, 0);
+		  tmp_var->obj_name = xstrdup (var->obj_name);
+		  varobj_delete (var, NULL, 0);
 		  install_variable (tmp_var);
 		}
 	      else
-		(*varp)->root->is_valid = 0;
+		var->root->is_valid = 0;
 	    }
-	  else /* locals must be invalidated.  */
-	    (*varp)->root->is_valid = 0;
 
 	  varp++;
 	}
     }
   xfree (all_rootvarobj);
-  return;
 }
diff --git a/gdb/varobj.h b/gdb/varobj.h
index f2cdcf8..85f2890 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -135,7 +135,9 @@ extern int varobj_list (struct varobj ***rootlist);
 extern VEC(varobj_update_result) *varobj_update (struct varobj **varp, 
 						 int explicit);
 
-extern void varobj_invalidate (void);
+extern void varobj_invalidate (struct objfile *objfile);
+
+extern void varobj_revalidate (void);
 
 extern int varobj_editable_p (struct varobj *var);
 

Re: [patch] [4/5] Types reference counting [varobj-validation]

[Eli Zaretskii]

> Date: Sat, 11 Apr 2009 12:22:15 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> currently GDB was sometimes keeping varobjs with stale references to objfiles.
> With the changes GDB started crashing on this problem.
> 
> +/* Invalidate the varobjs that are tied to the specified OBJFILE.  Call this
> +   function before you start removing OBJFILE.
> +
> +   Call varobj_revalidate after the OBJFILEs updates get finished.

Thanks.

Should this be added to gdbint.texinfo?

Re: [patch] [4/5] Types reference counting [varobj-validation]

[Jan Kratochvil]

On Sat, 11 Apr 2009 13:27:59 +0200, Eli Zaretskii wrote:
> > +   Call varobj_revalidate after the OBJFILEs updates get finished.
> 
> Thanks.
> 
> Should this be added to gdbint.texinfo?

As there is currently no notice of anything varobj related in gdbint and
I have no FE developer experience about it --- I know only about varobj code
crashes and the GDB testsuite PASSing varobj stuff --- I do not feel confident
to write such gdbint section from scratch.


Thanks for catching it, though,
Jan

Re: [patch] [4/5] Types reference counting [varobj-validation]

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> As varobj_invalidate was unconditionally invalidating any local
Jan> varobjs more often calls to varobj_invalidate could become more
Jan> intrusive to the user so I also fixed varobj_invalidate to care
Jan> only about varobjs bound to the specific objfile.

I am not sure about this implementation.

It seems to me that a varobj expression can span multiple objfiles.
So, don't we have to use the same logic as Paul's recent "display"
change to determine whether a varobj expression is invalid?

Or am I missing something?  I ask because it looks like we have
existing bugs here.

Tom

Re: [patch] [4/5] Types reference counting [varobj-validation]

[Jan Kratochvil]

On Thu, 16 Apr 2009 23:54:31 +0200, Tom Tromey wrote:
> It seems to me that a varobj expression can span multiple objfiles.
> So, don't we have to use the same logic as Paul's recent "display"
> change to determine whether a varobj expression is invalid?

you are right, thanks for the notice, updated.

It is based on the posted patch
	Re: [patch] Make a function for block->objfile lookups
	http://sourceware.org/ml/gdb-patches/2009-04/msg00609.html


Thanks,
Jan


gdb/
2009-04-22  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Split varobj_invalidate into a two-phased operation.
	* objfiles.c: Include varobj.h
	(free_objfile): Call varobj_invalidate.
	* parser-defs.h (exp_uses_objfile): New prototype.
	* printcmd.c (display_uses_objfile): Move the EXP checking part to ...
	* parse.c (exp_uses_objfile): ... a new function here.
	* symfile.c (new_symfile_objfile): Call varobj_revalidate.
	(reread_symbols): Call varobj_invalidate and varobj_revalidate.
	(clear_symtab_users): No longer call varobj_invalidate.
	* varobj.c: New includes objfiles.h and parser-defs.h.
	(varobj_invalidate): New parameter `objfile', comment it.
	New variable `var'.  Invalidate any varobj related to `objfile'.
	Remove unconditional invalidation of local varobjs.  Move global
	varobjs revalidation to ...
	(varobj_revalidate): ... a new function.
	* varobj.h (varobj_invalidate): Update the prototype.
	(varobj_revalidate): New prototype.

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 2889372..0b42c6c 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -50,6 +50,7 @@
 #include "addrmap.h"
 #include "arch-utils.h"
 #include "exec.h"
+#include "varobj.h"
 
 /* Prototypes for local functions */
 
@@ -409,6 +410,7 @@ free_objfile (struct objfile *objfile)
   /* Remove any references to this objfile in the global value
      lists.  */
   preserve_values (objfile);
+  varobj_invalidate (objfile);
 
   /* First do any symbol file specific actions required when we are
      finished with a particular symbol file.  Note that if the objfile
diff --git a/gdb/parse.c b/gdb/parse.c
index 96dc1c5..a601aa7 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -1359,6 +1359,45 @@ parser_fprintf (FILE *x, const char *y, ...)
   va_end (args);
 }
 
+/* Return 1 if EXP uses OBJFILE (and will become dangling when OBJFILE
+   is unloaded), otherwise return 0.  */
+
+int
+exp_uses_objfile (struct expression *exp, struct objfile *objfile)
+{
+  int endpos;
+  const union exp_element *const elts = exp->elts;
+
+  for (endpos = exp->nelts; endpos > 0; )
+    {
+      int i, args, oplen = 0;
+
+      exp->language_defn->la_exp_desc->operator_length (exp, endpos,
+							&oplen, &args);
+      gdb_assert (oplen > 0);
+
+      i = endpos - oplen;
+      if (elts[i].opcode == OP_VAR_VALUE)
+	{
+	  const struct block *const block = elts[i + 1].block;
+	  const struct symbol *const symbol = elts[i + 2].symbol;
+	  const struct obj_section *const section =
+	    SYMBOL_OBJ_SECTION (symbol);
+
+	  /* Check objfile where is placed the code touching the variable.  */
+	  if (matching_objfiles (block_objfile (block), objfile))
+	    return 1;
+
+	  /* Check objfile where the variable itself is placed.  */
+	  if (section && section->objfile == objfile)
+	    return 1;
+	}
+      endpos -= oplen;
+    }
+
+  return 0;
+}
+
 void
 _initialize_parse (void)
 {
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index cbda9c3..3159a21 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -299,4 +299,6 @@ extern void print_subexp_standard (struct expression *, int *,
 
 extern void parser_fprintf (FILE *, const char *, ...) ATTR_FORMAT (printf, 2 ,3);
 
+extern int exp_uses_objfile (struct expression *exp, struct objfile *objfile);
+
 #endif /* PARSER_DEFS_H */
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index daca777..994f14a 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1765,39 +1765,11 @@ disable_display_command (char *args, int from_tty)
 static int
 display_uses_objfile (const struct display *d, struct objfile *objfile)
 {
-  int endpos;
-  struct expression *const exp = d->exp;
-  const union exp_element *const elts = exp->elts;
-
   if (matching_objfiles (block_objfile (d->block), objfile))
     return 1;
 
-  for (endpos = exp->nelts; endpos > 0; )
-    {
-      int i, args, oplen = 0;
-
-      exp->language_defn->la_exp_desc->operator_length (exp, endpos,
-							&oplen, &args);
-      gdb_assert (oplen > 0);
-
-      i = endpos - oplen;
-      if (elts[i].opcode == OP_VAR_VALUE)
-	{
-	  const struct block *const block = elts[i + 1].block;
-	  const struct symbol *const symbol = elts[i + 2].symbol;
-	  const struct obj_section *const section =
-	    SYMBOL_OBJ_SECTION (symbol);
-
-	  /* Check objfile where is placed the code touching the variable.  */
-	  if (matching_objfiles (block_objfile (block), objfile))
-	    return 1;
-
-	  /* Check objfile where the variable itself is placed.  */
-	  if (section && section->objfile == objfile)
-	    return 1;
-	}
-      endpos -= oplen;
-    }
+  if (exp_uses_objfile (d->exp, objfile))
+    return 1;
 
   return 0;
 }
diff --git a/gdb/symfile.c b/gdb/symfile.c
index af1ab74..54d2bad 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -927,6 +927,8 @@ new_symfile_objfile (struct objfile *objfile, int mainline, int verbo)
 
   /* We're done reading the symbol file; finish off complaints.  */
   clear_complaints (&symfile_complaints, 0, verbo);
+
+  varobj_revalidate ();
 }
 
 /* Process a symbol file, as either the main file or as a dynamically
@@ -2341,6 +2343,7 @@ reread_symbols (void)
 	      /* Remove any references to this objfile in the global
 		 value lists.  */
 	      preserve_values (objfile);
+	      varobj_invalidate (objfile);
 
 	      /* Nuke all the state that we will re-read.  Much of the following
 	         code which sets things to NULL really is necessary to tell
@@ -2437,6 +2440,7 @@ reread_symbols (void)
 	         frameless.  */
 
 	      reinit_frame_cache ();
+	      varobj_revalidate ();
 
 	      /* Discard cleanups as symbol reading was successful.  */
 	      discard_cleanups (old_cleanups);
@@ -2817,10 +2821,6 @@ clear_symtab_users (void)
      between expressions and which ought to be reset each time.  */
   expression_context_block = NULL;
   innermost_block = NULL;
-
-  /* Varobj may refer to old symbols, perform a cleanup.  */
-  varobj_invalidate ();
-
 }
 
 static void
diff --git a/gdb/varobj.c b/gdb/varobj.c
index a7957f6..f9a5db7 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -26,6 +26,8 @@
 #include "gdbcmd.h"
 #include "block.h"
 #include "valprint.h"
+#include "objfiles.h"
+#include "parser-defs.h"
 
 #include "gdb_assert.h"
 #include "gdb_string.h"
@@ -2748,12 +2750,15 @@ When non-zero, varobj debugging is enabled."),
 			    &setlist, &showlist);
 }
 
-/* Invalidate the varobjs that are tied to locals and re-create the ones that
-   are defined on globals.
+/* Invalidate the varobjs that are tied to the specified OBJFILE.  Call this
+   function before you start removing OBJFILE.
+
+   Call varobj_revalidate after the OBJFILEs updates get finished.
+
    Invalidated varobjs will be always printed in_scope="invalid".  */
 
 void 
-varobj_invalidate (void)
+varobj_invalidate (struct objfile *objfile)
 {
   struct varobj **all_rootvarobj;
   struct varobj **varp;
@@ -2763,36 +2768,99 @@ varobj_invalidate (void)
       varp = all_rootvarobj;
       while (*varp != NULL)
 	{
+	  struct varobj *var = *varp;
+
+	  /* Floating varobjs are reparsed on each stop, so we don't care if
+	     the presently parsed expression refers to something that's gone.
+	     */
+	  if (var->root->floating)
+	    continue;
+
+	  if (var->root->is_valid
+	      && matching_objfiles (block_objfile (var->root->valid_block),
+	                            objfile))
+	    var->root->is_valid = 0;
+	  
+	  if (var->root->is_valid
+	      && exp_uses_objfile (var->root->exp, objfile))
+	    {
+	      var->root->is_valid = 0;
+
+	      /* No one touches EXP for !IS_VALID varobj.  */
+	      xfree (var->root->exp);
+	      var->root->exp = NULL;
+	    }
+	  
+	  if (var->type && TYPE_OBJFILE (var->type) == objfile)
+	    {
+	      if (!var->root->valid_block)
+		var->root->is_valid = 0;
+	      else
+		gdb_assert (!var->root->is_valid);
+
+	      var->type = NULL;
+	    }
+
+	  if (var->value
+	      && TYPE_OBJFILE (value_type (var->value)) == objfile)
+	    {
+	      if (!var->root->valid_block)
+		var->root->is_valid = 0;
+	      else
+		gdb_assert (!var->root->is_valid);
+
+	      value_free (var->value);
+	      var->value = NULL;
+	    }
+
+	  varp++;
+	}
+    }
+  xfree (all_rootvarobj);
+}
+
+/* Recreate any global varobjs possibly previously invalidated.  If the
+   expressions are no longer evaluatable set/keep the varobj invalid.  */
+
+void 
+varobj_revalidate (void)
+{
+  struct varobj **all_rootvarobj;
+  struct varobj **varp;
+
+  if (varobj_list (&all_rootvarobj) > 0)
+    {
+      varp = all_rootvarobj;
+      while (*varp != NULL)
+	{
+	  struct varobj *var = *varp;
+
 	  /* Floating varobjs are reparsed on each stop, so we don't care if
 	     the presently parsed expression refers to something that's gone.
 	     */
-	  if ((*varp)->root->floating)
+	  if (var->root->floating)
 	    continue;
 
 	  /* global var must be re-evaluated.  */     
-	  if ((*varp)->root->valid_block == NULL)
+	  if (var->root->valid_block == NULL)
 	    {
 	      struct varobj *tmp_var;
 
 	      /* Try to create a varobj with same expression.  If we succeed
 		 replace the old varobj, otherwise invalidate it.  */
-	      tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0,
-				       USE_CURRENT_FRAME);
+	      tmp_var = varobj_create (NULL, var->name, 0, USE_CURRENT_FRAME);
 	      if (tmp_var != NULL) 
 		{ 
-		  tmp_var->obj_name = xstrdup ((*varp)->obj_name);
-		  varobj_delete (*varp, NULL, 0);
+		  tmp_var->obj_name = xstrdup (var->obj_name);
+		  varobj_delete (var, NULL, 0);
 		  install_variable (tmp_var);
 		}
 	      else
-		(*varp)->root->is_valid = 0;
+		var->root->is_valid = 0;
 	    }
-	  else /* locals must be invalidated.  */
-	    (*varp)->root->is_valid = 0;
 
 	  varp++;
 	}
     }
   xfree (all_rootvarobj);
-  return;
 }
diff --git a/gdb/varobj.h b/gdb/varobj.h
index f2cdcf8..85f2890 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -135,7 +135,9 @@ extern int varobj_list (struct varobj ***rootlist);
 extern VEC(varobj_update_result) *varobj_update (struct varobj **varp, 
 						 int explicit);
 
-extern void varobj_invalidate (void);
+extern void varobj_invalidate (struct objfile *objfile);
+
+extern void varobj_revalidate (void);
 
 extern int varobj_editable_p (struct varobj *var);
 

obsolete: Re: [patch] [4/5] Types reference counting [varobj-validation]

[Jan Kratochvil]

This patch (not checked in) got now obsoleted by:
	[patch 8/8] Types GC [varobj]
	http://sourceware.org/ml/gdb-patches/2009-05/msg00551.html