Re: [RFC] varobj deletion after the binary has changed

[Denis PILAT <denis.pilat@st.com>]

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

Nick Roberts wrote:
>  
> Looks good.  Maybe varobj_update could use an enum:
>
> enum varobj_update_values
>   {
>     SCOPE_FALSE = -1,
>     TYPE_CHANGED,
>     SCOPE_INVALID
>   }
>   
Yes, it's better and more readable.
As you'll see in my patch, the varobj_update function could return more 
than these case of error, it's the caller that decides how to deal with 
these errors.
>
> (It looks like we could remove the return value of varobj_update_one as it
> doesn't seem to be used.)
>   
You're right, I removed it as well.

Attached is the new implementation plus the new exp file.

Denis

[-- Attachment #2: varobj.patch --]
[-- Type: text/plain, Size: 10307 bytes --]

2007-01-31  Denis Pilat  <denis.pilat@st.com>

	* varobj.c (struct varobj_root): Add is_valid member.
	(varobj_get_type): Check for invalid varobj.
	(varobj_get_attributes): Likewise.
	(variable_editable):Likewise.
	(varobj_update): Likewise plus use an enum for returned error values.
	(new_root_variable): Set root varobj as valid by default.
	(varobj_invalidate): New function.
	* varobj.h (enum varobj_update_error): New enum.
	(varobj_invalidate): New function.
	* symfile.c (clear_symtab_users): Use varobj_invalidate.
	* mi/mi-cmd-var.c (varobj_update_one): Change returned type to void and
	use of new enum varobj_update_error to deal with errors.


Index: varobj.c
===================================================================
--- varobj.c	(revision 553)
+++ varobj.c	(working copy)
@@ -71,6 +71,10 @@ struct varobj_root
      using the currently selected frame. */
   int use_selected_frame;
 
+  /* Flag that indicates validity: set to 0 when this varobj_root refers 
+     to symbols that do not exist anymore.  */
+  int is_valid;
+
   /* Language info for this variable and its children */
   struct language_specific *lang;
 
@@ -742,8 +746,9 @@ varobj_get_type (struct varobj *var)
   long length;
 
   /* For the "fake" variables, do not return a type. (It's type is
-     NULL, too.) */
-  if (CPLUS_FAKE_CHILD (var))
+     NULL, too.)
+     Do not return a type for invalid variables as well.  */
+  if (CPLUS_FAKE_CHILD (var) || !var->root->is_valid)
     return NULL;
 
   stb = mem_fileopen ();
@@ -778,7 +783,7 @@ varobj_get_attributes (struct varobj *va
 {
   int attributes = 0;
 
-  if (variable_editable (var))
+  if (var->root->is_valid && variable_editable (var))
     /* FIXME: define masks for attributes */
     attributes |= 0x00000001;	/* Editable */
 
@@ -1018,16 +1023,15 @@ install_new_value (struct varobj *var, s
    expression to see if it's changed.  Then go all the way
    through its children, reconstructing them and noting if they've
    changed.
-   Return value:
-    -1 if there was an error updating the varobj
-    -2 if the type changed
-    Otherwise it is the number of children + parent changed
+   Return value: 
+    < 0 for error values, see varobj.h.
+    Otherwise it is the number of children + parent changed.
 
    Only root variables can be updated... 
 
    NOTE: This function may delete the caller's varobj. If it
-   returns -2, then it has done this and VARP will be modified
-   to point to the new varobj. */
+   returns TYPE_CHANGED, then it has done this and VARP will be modified
+   to point to the new varobj.  */
 
 int
 varobj_update (struct varobj **varp, struct varobj ***changelist)
@@ -1048,12 +1052,15 @@ varobj_update (struct varobj **varp, str
 
   /* sanity check: have we been passed a pointer? */
   if (changelist == NULL)
-    return -1;
+    return WRONG_PARAM;
 
   /*  Only root variables can be updated... */
   if (!is_root_p (*varp))
     /* Not a root var */
-    return -1;
+    return WRONG_PARAM;
+
+  if (!(*varp)->root->is_valid)
+    return INVALID;
 
   /* Save the selected stack frame, since we will need to change it
      in order to evaluate expressions. */
@@ -1090,7 +1097,7 @@ varobj_update (struct varobj **varp, str
       /* This means the varobj itself is out of scope.
 	 Report it.  */
       VEC_free (varobj_p, result);
-      return -1;
+      return NOT_IN_SCOPE;
     }
 
   VEC_safe_push (varobj_p, stack, *varp);
@@ -1140,7 +1147,7 @@ varobj_update (struct varobj **varp, str
   *cv = 0;
 
   if (type_changed)
-    return -2;
+    return TYPE_CHANGED;
   else
     return changed;
 }
@@ -1409,6 +1416,7 @@ new_root_variable (void)
   var->root->frame = null_frame_id;
   var->root->use_selected_frame = 0;
   var->root->rootvar = NULL;
+  var->root->is_valid = 1;
 
   return var;
 }
@@ -1692,7 +1700,10 @@ variable_editable (struct varobj *var)
 static char *
 my_value_of_variable (struct varobj *var)
 {
-  return (*var->root->lang->value_of_variable) (var);
+  if (var->root->is_valid)
+    return (*var->root->lang->value_of_variable) (var);
+  else
+    return NULL;
 }
 
 static char *
@@ -2504,3 +2515,44 @@ When non-zero, varobj debugging is enabl
 			    show_varobjdebug,
 			    &setlist, &showlist);
 }
+
+/* Invalidate the varobjs that are tied to locals and re-create the ones that
+   are defined on globals.
+   Invalidated varobjs will be always printed in_scope="false".  */
+void 
+varobj_invalidate (void)
+{
+  struct varobj **all_rootvarobj;
+  struct varobj **varp;
+
+  if (varobj_list (&all_rootvarobj) > 0)
+  {
+    varp = all_rootvarobj;
+    while (*varp != NULL)
+      {
+        /* global var must be re-evaluated.  */     
+        if ((*varp)->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);
+          if (tmp_var != NULL) 
+            { 
+	      tmp_var->obj_name = xstrdup ((*varp)->obj_name);
+              varobj_delete (*varp, NULL, 0);
+              install_variable (tmp_var);
+            }
+          else
+              (*varp)->root->is_valid = 0;
+        }
+        else /* locals must be invalidated.  */
+          (*varp)->root->is_valid = 0;
+
+        varp++;
+      }
+    xfree (all_rootvarobj);
+  }
+  return;
+}
Index: varobj.h
===================================================================
--- varobj.h	(revision 553)
+++ varobj.h	(working copy)
@@ -38,7 +38,16 @@ enum varobj_type
     USE_CURRENT_FRAME,          /* Use the current frame */
     USE_SELECTED_FRAME          /* Always reevaluate in selected frame */
   };
-    
+
+/* Error return values for varobj_update function.  */
+enum varobj_update_error
+  {
+    NOT_IN_SCOPE = -1,          /* varobj not in scope, can not be updated.  */
+    TYPE_CHANGED = -2,          /* varobj type has changed.  */
+    INVALID = -3,               /* varobj is not valid anymore.  */
+    WRONG_PARAM = -4            /* function is called with wrong arguments.  */
+  };
+
 /* String representations of gdb's format codes (defined in varobj.c) */
 extern char *varobj_format_string[];
 
@@ -99,4 +108,6 @@ extern int varobj_list (struct varobj **
 
 extern int varobj_update (struct varobj **varp, struct varobj ***changelist);
 
+extern void varobj_invalidate (void);
+
 #endif /* VAROBJ_H */
Index: symfile.c
===================================================================
--- symfile.c	(revision 553)
+++ symfile.c	(working copy)
@@ -52,6 +52,7 @@
 #include "observer.h"
 #include "exec.h"
 #include "parser-defs.h"
+#include "varobj.h"
 
 #include <sys/types.h>
 #include <fcntl.h>
@@ -2602,6 +2603,10 @@ 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
Index: mi/mi-cmd-var.c
===================================================================
--- mi/mi-cmd-var.c	(revision 553)
+++ mi/mi-cmd-var.c	(working copy)
@@ -34,9 +34,9 @@ const char mi_no_values[] = "--no-values
 const char mi_simple_values[] = "--simple-values";
 const char mi_all_values[] = "--all-values";
 
-extern int varobjdebug;		/* defined in varobj.c */
+extern int varobjdebug;		/* defined in varobj.c.  */
 
-static int varobj_update_one (struct varobj *var,
+static void varobj_update_one (struct varobj *var,
 			      enum print_values print_values);
 
 static int mi_print_value_p (struct type *type, enum print_values print_values);
@@ -535,11 +535,9 @@ mi_cmd_var_update (char *command, char *
     return MI_CMD_DONE;
 }
 
-/* Helper for mi_cmd_var_update() Returns 0 if the update for
-   the variable fails (usually because the variable is out of
-   scope), and 1 if it succeeds. */
+/* Helper for mi_cmd_var_update().  */
 
-static int
+static void
 varobj_update_one (struct varobj *var, enum print_values print_values)
 {
   struct varobj **changelist;
@@ -549,37 +547,39 @@ varobj_update_one (struct varobj *var, e
 
   nc = varobj_update (&var, &changelist);
 
-  /* nc == 0 means that nothing has changed.
-     nc == -1 means that an error occured in updating the variable.
-     nc == -2 means the variable has changed type. */
+  /* nc >= 0  represents the number of changes reported into changelist.
+     nc < 0   means that an error occured or the the variable has 
+              changed type (TYPE_CHANGED).  */
   
   if (nc == 0)
-    return 1;
-  else if (nc == -1)
+    return;
+  else if (nc < 0)
     {
       if (mi_version (uiout) > 1)
         cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
       ui_out_field_string (uiout, "name", varobj_get_objname(var));
-      ui_out_field_string (uiout, "in_scope", "false");
-      if (mi_version (uiout) > 1)
-        do_cleanups (cleanup);
-      return -1;
-    }
-  else if (nc == -2)
-    {
-      if (mi_version (uiout) > 1)
-        cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
-      ui_out_field_string (uiout, "name", varobj_get_objname (var));
-      ui_out_field_string (uiout, "in_scope", "true");
-      ui_out_field_string (uiout, "new_type", varobj_get_type(var));
-      ui_out_field_int (uiout, "new_num_children", 
-			   varobj_get_num_children(var));
+
+      switch (nc)
+      {
+        case NOT_IN_SCOPE:
+        case WRONG_PARAM:
+          ui_out_field_string (uiout, "in_scope", "false");
+	  break;
+        case INVALID:
+          ui_out_field_string (uiout, "in_scope", "invalid");
+ 	  break;
+        case TYPE_CHANGED:
+	  ui_out_field_string (uiout, "in_scope", "true");
+          ui_out_field_string (uiout, "new_type", varobj_get_type(var));
+          ui_out_field_int (uiout, "new_num_children", 
+			    varobj_get_num_children(var));
+	  break;
+      }
       if (mi_version (uiout) > 1)
         do_cleanups (cleanup);
     }
   else
     {
-      
       cc = changelist;
       while (*cc != NULL)
 	{
@@ -595,7 +595,5 @@ varobj_update_one (struct varobj *var, e
 	  cc++;
 	}
       xfree (changelist);
-      return 1;
     }
-  return 1;
 }

[-- Attachment #3: mi-var-invalidate.exp --]
[-- Type: text/plain, Size: 3821 bytes --]

# Copyright 1999, 2000, 2001, 2002, 2004, 2005 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 2 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, write to the Free Software
# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.

# Test essential Machine interface (MI) operations
#
# Verify that once binary file has changed, GDB correctly handles
# previously defined MI variables.
#


load_lib mi-support.exp
set MIFLAGS "-i=mi"

gdb_exit
if [mi_gdb_start] {
    continue
}

set testfile "var-cmd"
set srcfile ${testfile}.c
set binfile ${objdir}/${subdir}/${testfile}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}
# Just change the output binary.
set binfile_bis ${objdir}/${subdir}/${testfile}_bis
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile_bis}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}

set testfile2 "basics"
set srcfile2 ${testfile2}.c
set binfile2 ${objdir}/${subdir}/${testfile2}
if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug additional_flags=-DFAKEARGV}] != "" } {
    untested mi-var-invalidate.exp
    return -1
}

mi_delete_breakpoints
mi_gdb_reinitialize_dir $srcdir/$subdir
mi_gdb_load ${binfile}

# Desc:  Create global variable
mi_gdb_test "-var-create global_simple * global_simple" \
	"\\^done,name=\"global_simple\",numchild=\"6\",type=\"simpleton\"" \
	"create global variable"

mi_runto do_locals_tests

# Desc: create local variables
mi_gdb_test "-var-create linteger * linteger" \
	"\\^done,name=\"linteger\",numchild=\"0\",type=\"int\"" \
	"create local variable linteger"

#
# reload the same binary
# global variable must be keeped, other invalidated
#
mi_delete_breakpoints
mi_gdb_load ${binfile_bis}
mi_runto main

# Check local variable are "invalid"
mi_gdb_test "-var-update linteger" \
	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
	"linteger not anymore in scope due to binary changes"

mi_gdb_test "-var-info-type linteger" \
	"\\^done,type=\"\"" \
	"not type for invalid variable linteger"

# Check global variable are still correct.
mi_gdb_test "-var-update global_simple" \
	"\\^done,changelist=\\\[\]" \
	"global_simple still alive"

mi_gdb_test "-var-info-type global_simple" \
	"\\^done,type=\"simpleton\"" \
	"type simpleton for valid variable global_simple"


#
# load an other binary
# all variables must be invalidated
#
mi_delete_breakpoints
mi_gdb_load ${binfile2}
# Check local variable are "invalid"
mi_gdb_test "-var-update linteger" \
	"\\^done,changelist=\\\[\{name=\"linteger\",in_scope=\"invalid\"\}\\\]" \
	"linteger not valid anymore due to binary changes"

mi_gdb_test "-var-info-type linteger" \
	"\\^done,type=\"\"" \
	"not type for invalid variable linteger"

# Check global variable are still correct.
mi_gdb_test "-var-update global_simple" \
	"\\^done,changelist=\\\[\{name=\"global_simple\",in_scope=\"invalid\"\}\\\]" \
	"global_simple not anymore in scope due to binary changes"

mi_gdb_test "-var-info-type global_simple" \
	"\\^done,type=\"\"" \
	"not type for invalid variable global_simple"

mi_gdb_exit
return 0