Re: Fix 'selected frame' varobjs

[Vladimir Prus <ghost at cs dot msu dot su>]

Daniel Jacobowitz wrote:

> On Sun, Dec 31, 2006 at 03:01:42AM +0300, Vladimir Prus wrote:
>> @@ -1061,6 +1061,14 @@ varobj_update (struct varobj **varp, str
>>    if (new == NULL)
>>      {
>>        (*varp)->error = 1;
>> +      /* Also set the value to NULL, so that
>> +     when the value becomes valid in future,
>> +     -var-update notice the change.  */
>> +      if ((*varp)->value != NULL)
>> +    {
>> +      value_free ((*varp)->value);
>> +      (*varp)->value = NULL;
>> +    }
>>        return -1;
>>      }
>>  
> 
> "in the future", "notices the change".  Should we be doing this
> everywhere that we set var->error (probably add a new helper to do
> that)?  There's about a half-dozen places.

I believe this was the only place where we don't set value to NULL on
errors. This reminded me that I wanted to remove var->error for quite some
time -- it's used in a single place, and using 'value == NULL' to indicate
error is quite as reasonable.

What about the attached?

- Volodya

2006-12-31  Daniel Jacobowitz  <dan@codesourcery.com>
            Vladimir Prus  <vladimir@codesourcery.com>

        Fix 'selected frame' varobjs.
        * varobj.c (struct varobj): Remove the error field.
        (varobj_set_value): Don't check var->error.
        (install_new_value): Don't set var->error.
        (varobj_update): Always pass the new value
        of the root via install_new_value.
        (create_child): Don't set error field.
        (new_variable): Likewise.
        (c_value_of_root): Always reevaluate the value
        of selected frame varobjs in the selected frame.
        Don't call reinit_frame_cache.

--- gdb/varobj.c	(/patches/gdb/varobj_update_memleak/gdb_mainline)	(revision 3137)
+++ gdb/varobj.c	(/patches/gdb/at_varobjs/gdb_mainline)	(revision 3137)
@@ -107,14 +107,12 @@ struct varobj
   /* The type of this variable. This may NEVER be NULL. */
   struct type *type;
 
-  /* The value of this expression or subexpression.  This may be NULL. 
+  /* The value of this expression or subexpression.  A NULL value
+     indicates there was an error getting this value.
      Invariant: if varobj_value_is_changeable_p (this) is non-zero, 
      the value is either NULL, or not lazy.  */
   struct value *value;
 
-  /* Did an error occur evaluating the expression or getting its value? */
-  int error;
-
   /* The number of (immediate) children this variable has */
   int num_children;
 
@@ -811,7 +809,7 @@ varobj_set_value (struct varobj *var, ch
   struct value *value;
   int saved_input_radix = input_radix;
 
-  if (var->value != NULL && variable_editable (var) && !var->error)
+  if (var->value != NULL && variable_editable (var))
     {
       char *s = expression;
       int i;
@@ -909,7 +907,6 @@ install_new_value (struct varobj *var, s
   int need_to_fetch;
   int changed = 0;
 
-  var->error = 0;
   /* We need to know the varobj's type to decide if the value should
      be fetched or not.  C++ fake children (public/protected/private) don't have
      a type. */
@@ -946,14 +943,11 @@ install_new_value (struct varobj *var, s
     {
       if (!gdb_value_fetch_lazy (value))
 	{
-	  var->error = 1;
 	  /* Set the value to NULL, so that for the next -var-update,
 	     we don't try to compare the new value with this value,
 	     that we couldn't even read.  */
 	  value = NULL;
 	}
-      else
-	var->error = 0;
     }
 
   /* If the type is changeable, compare the old and the new values.
@@ -1078,12 +1072,6 @@ varobj_update (struct varobj **varp, str
   if (fi)
     select_frame (fi);
 
-  if (new == NULL)
-    {
-      (*varp)->error = 1;
-      return -1;
-    }
-
   /* If this is a "use_selected_frame" varobj, and its type has changed,
      them note that it's changed. */
   if (type_changed)
@@ -1097,6 +1085,14 @@ varobj_update (struct varobj **varp, str
       VEC_safe_push (varobj_p, result, *varp);
     }
 
+  if (new == NULL)
+    {
+      /* This means the varobj itself is out of scope.
+	 Report it.  */
+      VEC_free (varobj_p, result);
+      return -1;
+    }
+
   VEC_safe_push (varobj_p, stack, *varp);
 
   /* Walk through the children, reconstructing them all. */
@@ -1373,9 +1369,6 @@ create_child (struct varobj *parent, int
 						       child->index);
   install_new_value (child, value, 1);
 
-  if ((!CPLUS_FAKE_CHILD (child) && child->value == NULL) || parent->error)
-    child->error = 1;
-
   return child;
 }
 
@@ -1396,7 +1389,6 @@ new_variable (void)
   var->index = -1;
   var->type = NULL;
   var->value = NULL;
-  var->error = 0;
   var->num_children = -1;
   var->parent = NULL;
   var->children = NULL;
@@ -1975,11 +1967,10 @@ c_value_of_root (struct varobj **var_han
 
 
   /* Determine whether the variable is still around. */
-  if (var->root->valid_block == NULL)
+  if (var->root->valid_block == NULL || var->root->use_selected_frame)
     within_scope = 1;
   else
     {
-      reinit_frame_cache ();
       fi = frame_find_by_id (var->root->frame);
       within_scope = fi != NULL;
       /* FIXME: select_frame could fail */
@@ -2001,11 +1992,8 @@ c_value_of_root (struct varobj **var_han
          go on */
       if (gdb_evaluate_expression (var->root->exp, &new_val))
 	{
-	  var->error = 0;
 	  release_value (new_val);
 	}
-      else
-	var->error = 1;
 
       return new_val;
     }

Property changes on: 
___________________________________________________________________
Name: csl:base
 +/all/patches/gdb/varobj_update_memleak/gdb_mainline
Name: svk:merge
 +d48a11ec-ee1c-0410-b3f5-c20844f99675:/patches/gdb/varobj_update_memleak/gdb_mainline:3134
 +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:159718