Fix 'selected frame' varobjs

Fix 'selected frame' varobjs

[Vladimir Prus]

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


At the moment, the 'selected frame' varobjs (created with -var-create NAME @) are
somewhat broken.

I've reported that in:

	http://article.gmane.org/gmane.comp.gdb.devel/16381

to which Dan posted a preliminary patch to which I replied with another bug:

	http://article.gmane.org/gmane.comp.gdb.devel/16398

and the story ended there. This patch fixed both problems and causes no
regressions. OK?

- Volodya

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

	Fix 'selected frame' varobjs.
	* varobj.c (varobj_update): If we get error getting new
	value of root, reset the value.
	(c_value_of_root): Always reevaluate the value
	of selected frame varobjs in the selected frame.
	Dont call reinit_frame_cache.

[-- Attachment #2: at_varobjs__gdb_mainline.diff --]
[-- Type: text/x-diff, Size: 936 bytes --]

--- gdb/varobj.c	(/mirrors/gdb_mainline)	(revision 3011)
+++ gdb/varobj.c	(/patches/gdb/at_varobjs/gdb_mainline)	(revision 3011)
@@ -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;
     }
 
@@ -1950,11 +1958,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 */

Re: Fix 'selected frame' varobjs

[Daniel Jacobowitz]

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.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix 'selected frame' varobjs

[Vladimir Prus]

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

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: at_varobjs__gdb_mainline.diff --]
[-- Type: text/x-diff; name="at_varobjs__gdb_mainline.diff", Size: 4011 bytes --]

--- 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;
 }
 \f
@@ -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

Re: Fix 'selected frame' varobjs

[Daniel Jacobowitz]

On Wed, Jan 10, 2007 at 12:20:55AM +0300, Vladimir Prus wrote:
> 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.

Looks good to me - this is OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix 'selected frame' varobjs

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Wed, Jan 10, 2007 at 12:20:55AM +0300, Vladimir Prus wrote:
>> 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.
> 
> Looks good to me - this is OK.

Checked in, thanks.

- Volodya