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

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

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

Denis PILAT wrote:
> Daniel Jacobowitz wrote:
>> On Tue, Jan 23, 2007 at 01:32:22PM +0100, Denis PILAT wrote:
>>  
>>> Hi,
>>>
>>> We have a bug in one of our gdb target, when the binary changed 
>>> while beeing debugged it appears that some of our varobj refers to 
>>> invalid symbols or type.
>>>
>>> I propose a patch that delete all varobj when symbols are reloaded. 
>>> May be there is a better place to do that but I think we must do 
>>> that somewhere, don't you ?
>>>     
>>
>> The right thing to do is probably to figure out where the
>> invalid references come from and fix them - probably by re-evaluating
>> expressions at the next -var-update.  Deleting things behind the front
>> end's back is a bad policy.
>>
>>
>>   
> Deleting all is a little bit radical, I can understand that.
> Re-evaluating expression would give the expected thing for globale 
> variables, but for varobj that has been set on local variable (so 
> within a "valid_block"), it should have side effects: the evaluation 
> of the expression might tied the varobj to a variable different than 
> the one the user originally decided.
>
> I propose to distinguish 2 cases
> - varobj on global variable (valid_block == null) must have their 
> expression re-evaluated.
> - other one must be put in a kind of invalid state that will gives 
> "in_scope="false" to the user.
> This last point will involve a new field in varobj structure, I 
> haven't seen something to mark varobj as invalid.
>
> Denis
>
Hi

Attached is a new patch proposal when I implemented what I explained above.

During the symbol re-loading I invalidate varobjs linked to locals 
variable and try to re-evaluate the expression for globals.
No more varobjs are deleted, and during varobj_update invalidated 
symbols are skipped so displayed as not in scope.

If you're fine with this implementation I'll write 2 more test cases in 
the testsuite that lead to segmentation fault in the current GDB:
- one where the binary file completely changed so any varobjs defined on 
global variable are invalidated.
- one where the binary file is just updated so varobjs on globals can 
still be evaluated.

I check the testsuite with i386-linux target with no regression and I 
passed a valgrind as well.

-- 
Denis PILAT
STMicroelectronics


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

Index: symfile.c
===================================================================
--- symfile.c	(revision 552)
+++ 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>
@@ -2585,6 +2586,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
===================================================================
--- varobj.c	(revision 552)
+++ varobj.c	(working copy)
@@ -77,6 +77,10 @@ struct varobj_root
 
   /* Next root variable */
   struct varobj_root *next;
+
+  /* Flag that indicates validity: set to 0 when this varobj_root refers 
+     to symbols that do not exist anymore.  */
+  int is_valid;
 };
 
 /* Every variable in the system has a structure of this type defined
@@ -912,6 +916,9 @@ varobj_update (struct varobj **varp, str
     /* Not a root var */
     return -1;
 
+  if (!(*varp)->root->is_valid)
+    return -1;
+
   /* Save the selected stack frame, since we will need to change it
      in order to evaluate expressions. */
   old_fid = get_frame_id (deprecated_selected_frame);
@@ -1361,6 +1368,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;
 }
@@ -2569,3 +2577,45 @@ 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 success we replace
+             the old varobj, othewize we invalidate it.  */      
+          tmp_var = varobj_create (NULL, (*varp)->name, (CORE_ADDR) 0, USE_CURRENT_FRAME);
+          if (tmp_var != NULL) 
+            { 
+	      tmp_var->obj_name =
+		savestring ((*varp)->obj_name, strlen ((*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 552)
+++ varobj.h	(working copy)
@@ -99,4 +99,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 */