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

[Denis PILAT <denis dot pilat at st dot com>]

Nick,
Thanks you for the documentation update.
Hereby is 2 patches I would like to commit, the C part and the new . exp 
file.
I change the .exp file with your comment, with a newer copyright notice, 
and with including value field in output of -var-create.
I also changed a comment plus typos in the C code.

Daniel already approved the sources, Eli is near to be fine with the doc.
Could have a look once more into the  following, at least for the 
ChangeLog entry ?
Then how could we synchronize our commit ?

--
Denis

ChangeLog for the testsuite:

2007-02-12  Denis Pilat  <denis.pilat@st.com>

	* gdb.mi/mi-var-invalidate.exp: New files.

   

2007-02-12  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: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.180
diff -u -p -r1.180 symfile.c
--- symfile.c	21 Jan 2007 01:02:03 -0000	1.180
+++ symfile.c	12 Feb 2007 09:54:22 -0000
@@ -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: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.83
diff -u -p -r1.83 varobj.c
--- varobj.c	8 Feb 2007 17:39:48 -0000	1.83
+++ varobj.c	12 Feb 2007 09:54:30 -0000
@@ -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)
@@ -1046,34 +1050,37 @@ varobj_update (struct varobj **varp, str
   struct frame_id old_fid;
   struct frame_info *fi;
 
-  /* sanity check: have we been passed a pointer? */
+  /* sanity check: have we been passed a pointer?  */
   if (changelist == NULL)
-    return -1;
+    return WRONG_PARAM;
 
-  /*  Only root variables can be updated... */
+  /*  Only root variables can be updated...  */
   if (!is_root_p (*varp))
-    /* Not a root var */
-    return -1;
+    /* Not a root var.  */
+    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. */
+     in order to evaluate expressions.  */
   old_fid = get_frame_id (deprecated_selected_frame);
 
   /* Update the root variable. value_of_root can return NULL
      if the variable is no longer around, i.e. we stepped out of
      the frame in which a local existed. We are letting the 
      value_of_root variable dispose of the varobj if the type
-     has changed. */
+     has changed.  */
   type_changed = 1;
   new = value_of_root (varp, &type_changed);
 
-  /* Restore selected frame */
+  /* Restore selected frame.  */
   fi = frame_find_by_id (old_fid);
   if (fi)
     select_frame (fi);
 
   /* If this is a "use_selected_frame" varobj, and its type has changed,
-     them note that it's changed. */
+     them note that it's changed.  */
   if (type_changed)
     VEC_safe_push (varobj_p, result, *varp);
 
@@ -1090,12 +1097,12 @@ 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);
 
-  /* Walk through the children, reconstructing them all. */
+  /* Walk through the children, reconstructing them all.  */
   while (!VEC_empty (varobj_p, stack))
     {
       v = VEC_pop (varobj_p, stack);
@@ -1126,7 +1133,7 @@ varobj_update (struct varobj **varp, str
 	}
     }
 
-  /* Alloc (changed + 1) list entries */
+  /* Alloc (changed + 1) list entries.  */
   changed = VEC_length (varobj_p, result);
   *changelist = xmalloc ((changed + 1) * sizeof (struct varobj *));
   cv = *changelist;
@@ -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="invalid".  */
+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
===================================================================
RCS file: /cvs/src/src/gdb/varobj.h,v
retrieving revision 1.7
diff -u -p -r1.7 varobj.h
--- varobj.h	9 Jan 2007 17:58:59 -0000	1.7
+++ varobj.h	12 Feb 2007 09:54:33 -0000
@@ -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: mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.29
diff -u -p -r1.29 mi-cmd-var.c
--- mi/mi-cmd-var.c	8 Feb 2007 19:46:30 -0000	1.29
+++ mi/mi-cmd-var.c	12 Feb 2007 09:54:33 -0000
@@ -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;
 }

# 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 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\",value=\".*\",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\",value=\".*\",type=\"int\"" \
	"create local variable linteger"

#
# Reload the same binary.
# Global variable should remain, local should be invalidated.
#
mi_delete_breakpoints
mi_gdb_load ${binfile_bis}
mi_runto main

# Check local variable is "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=\"\"" \
	"no type for invalid variable linteger (1)"

# Check global variable is 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=\"\"" \
	"no type for invalid variable linteger (2)"

# 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=\"\"" \
	"no type for invalid variable global_simple"

mi_gdb_exit
return 0