[PATCH] MI: lvalues and variable_editable

[PATCH] MI: lvalues and variable_editable

[Nick Roberts]

I'm representing a patch from before the release of GDB 6.7.  The change to
c_name_of_variable is unrelated and I can commit it separately, if wished.

I also attach a test for the main change.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2007-10-27  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (c_variable_editable, cplus_variable_editable)
	(java_variable_editable, variable_editable): Delete.
	(varobj_editable_p): Replace above functions with one language
	independent function.  Check for an lvalue.
	(varobj_get_attributes, varobj_set_value): Use varobj_editable_p.
	(struct language_specific): Delete variable_editable field.
        (varobj_value_is_changeable_p): Simplify.
        (c_name_of_variable): Remove memory leak.

	* mi-var-cmd.exp: Add test for assigning to varobjs that aren't
	lvalues. 


Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.96
diff -p -c -p -r1.96 varobj.c
*** varobj.c	27 Sep 2007 18:04:12 -0000	1.96
--- varobj.c	26 Oct 2007 22:23:27 -0000
*************** static struct value *value_of_root (stru
*** 221,227 ****
  
  static struct value *value_of_child (struct varobj *parent, int index);
  
! static int variable_editable (struct varobj *var);
  
  static char *my_value_of_variable (struct varobj *var);
  
--- 221,227 ----
  
  static struct value *value_of_child (struct varobj *parent, int index);
  
! static int varobj_editable_p (struct varobj *var);
  
  static char *my_value_of_variable (struct varobj *var);
  
*************** static struct value *c_value_of_child (s
*** 248,255 ****
  
  static struct type *c_type_of_child (struct varobj *parent, int index);
  
- static int c_variable_editable (struct varobj *var);
- 
  static char *c_value_of_variable (struct varobj *var);
  
  /* C++ implementation */
--- 248,253 ----
*************** static struct value *cplus_value_of_chil
*** 270,277 ****
  
  static struct type *cplus_type_of_child (struct varobj *parent, int index);
  
- static int cplus_variable_editable (struct varobj *var);
- 
  static char *cplus_value_of_variable (struct varobj *var);
  
  /* Java implementation */
--- 268,273 ----
*************** static struct value *java_value_of_child
*** 290,297 ****
  
  static struct type *java_type_of_child (struct varobj *parent, int index);
  
- static int java_variable_editable (struct varobj *var);
- 
  static char *java_value_of_variable (struct varobj *var);
  
  /* The language specific vector */
--- 286,291 ----
*************** struct language_specific
*** 324,332 ****
    /* The type of the INDEX'th child of PARENT. */
    struct type *(*type_of_child) (struct varobj * parent, int index);
  
-   /* Is VAR editable? */
-   int (*variable_editable) (struct varobj * var);
- 
    /* The current value of VAR. */
    char *(*value_of_variable) (struct varobj * var);
  };
--- 318,323 ----
*************** static struct language_specific language
*** 343,349 ****
     c_value_of_root,
     c_value_of_child,
     c_type_of_child,
-    c_variable_editable,
     c_value_of_variable}
    ,
    /* C */
--- 334,339 ----
*************** static struct language_specific language
*** 356,362 ****
     c_value_of_root,
     c_value_of_child,
     c_type_of_child,
-    c_variable_editable,
     c_value_of_variable}
    ,
    /* C++ */
--- 346,351 ----
*************** static struct language_specific language
*** 369,375 ****
     cplus_value_of_root,
     cplus_value_of_child,
     cplus_type_of_child,
-    cplus_variable_editable,
     cplus_value_of_variable}
    ,
    /* Java */
--- 358,363 ----
*************** static struct language_specific language
*** 382,388 ****
     java_value_of_root,
     java_value_of_child,
     java_type_of_child,
-    java_variable_editable,
     java_value_of_variable}
  };
  
--- 370,375 ----
*************** varobj_get_attributes (struct varobj *va
*** 856,862 ****
  {
    int attributes = 0;
  
!   if (var->root->is_valid && variable_editable (var))
      /* FIXME: define masks for attributes */
      attributes |= 0x00000001;	/* Editable */
  
--- 843,849 ----
  {
    int attributes = 0;
  
!   if (var->root->is_valid && varobj_editable_p (var))
      /* FIXME: define masks for attributes */
      attributes |= 0x00000001;	/* Editable */
  
*************** varobj_set_value (struct varobj *var, ch
*** 887,893 ****
    struct value *value;
    int saved_input_radix = input_radix;
  
!   if (var->value != NULL && variable_editable (var))
      {
        char *s = expression;
        int i;
--- 874,880 ----
    struct value *value;
    int saved_input_radix = input_radix;
  
!   if (var->value != NULL && varobj_editable_p (var))
      {
        char *s = expression;
        int i;
*************** value_of_child (struct varobj *parent, i
*** 1801,1814 ****
    return value;
  }
  
- /* Is this variable editable? Use the variable's type to make
-    this determination. */
- static int
- variable_editable (struct varobj *var)
- {
-   return (*var->root->lang->variable_editable) (var);
- }
- 
  /* GDB already has a command called "value_of_variable". Sigh. */
  static char *
  my_value_of_variable (struct varobj *var)
--- 1788,1793 ----
*************** value_get_print_value (struct value *val
*** 1840,1845 ****
--- 1819,1862 ----
    return thevalue;
  }
  
+ 
+ static int
+ varobj_editable_p (struct varobj *var)
+ {
+   struct type *type;
+   struct expression *exp;
+   struct value *value;
+ 
+   if (CPLUS_FAKE_CHILD (var))
+     return 0;
+ 
+   if (!gdb_evaluate_expression (var->root->exp, &value))
+     {
+       /* We cannot proceed without a valid expression. */
+       xfree (exp);
+       return 0;
+     }
+   if (!VALUE_LVAL(value))
+     return 0;
+ 
+   type = get_value_type (var);
+ 
+   switch (TYPE_CODE (type))
+     {
+     case TYPE_CODE_STRUCT:
+     case TYPE_CODE_UNION:
+     case TYPE_CODE_ARRAY:
+     case TYPE_CODE_FUNC:
+     case TYPE_CODE_METHOD:
+       return 0;
+       break;
+ 
+     default:
+       return 1;
+       break;
+     }
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report
*************** value_get_print_value (struct value *val
*** 1852,1858 ****
  static int
  varobj_value_is_changeable_p (struct varobj *var)
  {
-   int r;
    struct type *type;
  
    if (CPLUS_FAKE_CHILD (var))
--- 1869,1874 ----
*************** varobj_value_is_changeable_p (struct var
*** 1865,1878 ****
      case TYPE_CODE_STRUCT:
      case TYPE_CODE_UNION:
      case TYPE_CODE_ARRAY:
!       r = 0;
        break;
  
      default:
!       r = 1;
      }
- 
-   return r;
  }
  
  /* Given the value and the type of a variable object,
--- 1881,1893 ----
      case TYPE_CODE_STRUCT:
      case TYPE_CODE_UNION:
      case TYPE_CODE_ARRAY:
!       return 0;
        break;
  
      default:
!       return 1;
!       break;
      }
  }
  
  /* Given the value and the type of a variable object,
*************** c_number_of_children (struct varobj *var
*** 1983,1991 ****
  }
  
  static char *
! c_name_of_variable (struct varobj *parent)
  {
!   return savestring (parent->name, strlen (parent->name));
  }
  
  /* Return the value of element TYPE_INDEX of a structure
--- 1998,2006 ----
  }
  
  static char *
! c_name_of_variable (struct varobj *var)
  {
!   return var->name;
  }
  
  /* Return the value of element TYPE_INDEX of a structure
*************** c_type_of_child (struct varobj *parent, 
*** 2212,2236 ****
    return type;
  }
  
- static int
- c_variable_editable (struct varobj *var)
- {
-   switch (TYPE_CODE (get_value_type (var)))
-     {
-     case TYPE_CODE_STRUCT:
-     case TYPE_CODE_UNION:
-     case TYPE_CODE_ARRAY:
-     case TYPE_CODE_FUNC:
-     case TYPE_CODE_METHOD:
-       return 0;
-       break;
- 
-     default:
-       return 1;
-       break;
-     }
- }
- 
  static char *
  c_value_of_variable (struct vWrite failed flushing stdout buffer.
write stdout: Broken pipe
arobj *var)
  {
--- 2227,2232 ----
*************** cplus_type_of_child (struct varobj *pare
*** 2602,2616 ****
    return type;
  }
  
- static int
- cplus_variable_editable (struct varobj *var)
- {
-   if (CPLUS_FAKE_CHILD (var))
-     return 0;
- 
-   return c_variable_editable (var);
- }
- 
  static char *
  cplus_value_of_variable (struct varobj *var)
  {
--- 2598,2603 ----
*************** java_type_of_child (struct varobj *paren
*** 2694,2705 ****
    return cplus_type_of_child (parent, index);
  }
  
- static int
- java_variable_editable (struct varobj *var)
- {
-   return cplus_variable_editable (var);
- }
- 
  static char *
  java_value_of_variable (struct varobj *var)
  {
--- 2681,2686 ----


Index: mi-var-cmd.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-cmd.exp,v
retrieving revision 1.33
diff -p -c -p -r1.33 mi-var-cmd.exp
*** mi-var-cmd.exp	1 Oct 2007 14:07:45 -0000	1.33
--- mi-var-cmd.exp	26 Oct 2007 22:25:07 -0000
*************** mi_gdb_test "113-var-create argc * argc"
*** 68,73 ****
--- 68,77 ----
  	"&\"mi_cmd_var_create: unable to create variable object\\\\n\".*113\\^error,msg=\"mi_cmd_var_create: unable to create variable object\"" \
  	"create out of scope variable"
  
+ mi_gdb_test "-var-create castfloat * (float)global_simple.integer" \
+ 	"\\^done,name=\"castfloat\",numchild=\"0\",value=\".*\",type=\"float\"" \
+ 	"create float cast of an int"
+ 
  mi_runto do_locals_tests
  
  set line_dlt_first_real [gdb_get_line_number "linteger = 1234;"]
*************** mi_gdb_test "-var-assign global_simple 0
*** 278,283 ****
--- 282,291 ----
  	"&\"mi_cmd_var_assign: Variable object is not editable\\\\n\".*\\^error,msgWrite failed flushing stdout buffer.
write stdout: Broken pipe
=\"mi_cmd_var_assign: Variable object is not editable\"" \
  	"assign to global_simple"
  
+ mi_gdb_test "-var-assign castfloat 5.0" \
+ 	"&\"mi_cmd_var_assign: Variable object is not editable\\\\n\".*\\^error,msg=\"mi_cmd_var_assign: Variable object is not editable\"" \
+ 	"assign to a cast"
+ 
  mi_gdb_test "-var-assign linteger 3333" \
  	"\\^done,value=\"3333\"" \
  	"assign to linteger"

Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus]

Nick Roberts wrote:

> 
> I'm representing a patch from before the release of GDB 6.7.  The change
> to c_name_of_variable is unrelated and I can commit it separately, if
> wished.
> 
> I also attach a test for the main change.

I think the checking of lvalue-ness is a very good change. I have some comments,
however:

1. In varobj_editable_p you call gdb_evaluate_expression, and I believe this
to be wrong. We call gdb_evaluate_expression when we create varobj, and it
either succeeds, eventually setting varobj->value to something, or it does
not. There's no point to call gdb_evaluate_expression again. Further,
in varobj_create, gdb_evaluate_expression is called in specific frame,
and varobj_editable_p calls it in current frame. Also, if gdb_evaluate_expression
fails, you xfree(exp). Where is 'exp' assigned a value? 

2. In varobj_value_is_changeable_p, you have changed from returning 'r' at the
end of function, to returning in several places. I don't think this change has
any effect on logic and therefore, if committed, should be committed separately.
And, I actually prefer the original code -- return in one place makes logic simpler.

3. I think your change to c_name_of_variable should be a separate patch. I
also not sure it's right. Consider java_name_of_variable -- it calls 
cplus_name_of_variable and then does some quoting. With your change 
cplus_name_of_variable will return varobj->name, the the following code will
directly modify it. Is it intended?

4. I don't think your test actually tests that the 'editable' attribute comes
out as 'false'.

Thanks,
Volodya

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > I think the checking of lvalue-ness is a very good change. I have some
 > comments, however:
 > 
 > 1. In varobj_editable_p you call gdb_evaluate_expression, and I believe this
 > to be wrong. We call gdb_evaluate_expression when we create varobj, and it
 > either succeeds, eventually setting varobj->value to something, or it does
 > not. There's no point to call gdb_evaluate_expression again. 

If gdb_evaluate_expression fails in varobj_create, a variable object is still
created, but just with an undefined value.  It needs to be called to get
value for VALUE_LVAL.

 >                                                               Further,
 > in varobj_create, gdb_evaluate_expression is called in specific frame,
 > and varobj_editable_p calls it in current frame. 

The current frame should be the frame in which the variable object is defined.
Can you explain why that's a problem?

 >                                                  Also, if
 > gdb_evaluate_expression fails, you xfree(exp). Where is 'exp' assigned a
 > value?

It's not needed.  I'll remove it from the patch.

 > 2. In varobj_value_is_changeable_p, you have changed from returning 'r' at
 > the end of function, to returning in several places. I don't think this
 > change has any effect on logic and therefore, if committed, should be
 > committed separately.  And, I actually prefer the original code -- return in
 > one place makes logic simpler.

It used to be a bigger change.  It's a consistent style with other functions
in varobj.c but maybe it's gratuitous.  I don't mind leaving it out.

 > 3. I think your change to c_name_of_variable should be a separate patch. I
 > also not sure it's right. Consider java_name_of_variable -- it calls 
 > cplus_name_of_variable and then does some quoting. With your change 
 > cplus_name_of_variable will return varobj->name, the the following code will
 > directly modify it. Is it intended?

Perhaps the right place for savestring, or better xsprintf, is in
java_name_of_variable.  Alternatively when varobj_get_expression is called
in mi-cmd-var.c, it's value should be freed.  I'll remove this change for now.

 > 4. I don't think your test actually tests that the 'editable' attribute comes
 > out as 'false'.

I'm not sure what to say.  It shows that if you try to assign a value to a 
cast GDB says "Variable object is not editable".

The error message for a variable object of a cast used to be:

&"mi_cmd_var_assign: Could not assign expression to variable object\n"
^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
(gdb) 

I could add another test for -var-show-attributes which will now give:

^done,attr="noneditable"

for a cast but I never use this command.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus]

On Saturday 27 October 2007 16:05:32 Nick Roberts wrote:
>  > I think the checking of lvalue-ness is a very good change. I have some
>  > comments, however:
>  > 
>  > 1. In varobj_editable_p you call gdb_evaluate_expression, and I believe this
>  > to be wrong. We call gdb_evaluate_expression when we create varobj, and it
>  > either succeeds, eventually setting varobj->value to something, or it does
>  > not. There's no point to call gdb_evaluate_expression again. 
> 
> If gdb_evaluate_expression fails in varobj_create, a variable object is still
> created, but just with an undefined value.  It needs to be called to get
> value for VALUE_LVAL.

Why would a second call to gdb_evaluate_expression succeed? We already tried
to evaluate expression, we failed, varobj has no value whatsoever. So,
I'd say we should refuse to assign anything in this case.

>  >                                                               Further,
>  > in varobj_create, gdb_evaluate_expression is called in specific frame,
>  > and varobj_editable_p calls it in current frame. 
> 
> The current frame should be the frame in which the variable object is defined.
> Can you explain why that's a problem?

Here's the code in question:

      /* When the frame is different from the current frame, 
         we must select the appropriate frame before parsing
         the expression, otherwise the value will not be current.
         Since select_frame is so benign, just call it for all cases. */
      if (fi != NULL)
	{
	  var->root->frame = get_frame_id (fi);
	  old_fi = get_selected_frame (NULL);
	  select_frame (fi);
	}

      /* We definitively need to catch errors here.
         If evaluate_expression succeeds we got the value we wanted.
         But if it fails, we still go on with a call to evaluate_type()  */
      if (!gdb_evaluate_expression (var->root->exp, &value))

So it seems to be varobj is not necessary defined in the current frame.

>  > 2. In varobj_value_is_changeable_p, you have changed from returning 'r' at
>  > the end of function, to returning in several places. I don't think this
>  > change has any effect on logic and therefore, if committed, should be
>  > committed separately.  And, I actually prefer the original code -- return in
>  > one place makes logic simpler.
> 
> It used to be a bigger change.  It's a consistent style with other functions
> in varobj.c but maybe it's gratuitous.  I don't mind leaving it out.

Ok, good ;-)

>  > 3. I think your change to c_name_of_variable should be a separate patch. I
>  > also not sure it's right. Consider java_name_of_variable -- it calls 
>  > cplus_name_of_variable and then does some quoting. With your change 
>  > cplus_name_of_variable will return varobj->name, the the following code will
>  > directly modify it. Is it intended?
> 
> Perhaps the right place for savestring, or better xsprintf, is in
> java_name_of_variable.  Alternatively when varobj_get_expression is called
> in mi-cmd-var.c, it's value should be freed.  I'll remove this change for now.

Yeah, it's somewhat nasty, as for c varobj we don't need any processing and
can just return varobj->name, but for java we need this dash-to-dot replacement.
I was thinking that maybe we should have 'java_name' field in varobj -- but
it would just waste space. Really, this all is just crying for C++.

>  > 4. I don't think your test actually tests that the 'editable' attribute comes
>  > out as 'false'.
> 
> I'm not sure what to say.  It shows that if you try to assign a value to a 
> cast GDB says "Variable object is not editable".
> 
> The error message for a variable object of a cast used to be:
> 
> &"mi_cmd_var_assign: Could not assign expression to variable object\n"
> ^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
> (gdb) 

Is this test failing with current GDB, and passing after your change? I think
GDB will refuse to assign value to rvalue anyway -- at least here's what I
get without your patch:

	(gdb)
	-var-create V * (float)argc
	^done,name="V",numchild="0",value="0",type="float"
	(gdb)
	-var-assign V 10
	&"mi_cmd_var_assign: Could not assign expression to variable object\n"
	^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
	(gdb)

> I could add another test for -var-show-attributes which will now give:
> 
> ^done,attr="noneditable"
> 
> for a cast but I never use this command.

I think the biggest value of this patch is that we get this attribute, *before*
trying to assign the value, so yes, I think a test for this specific behaviour
would be great.

- Volodya

 

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > >  > 1. In varobj_editable_p you call gdb_evaluate_expression, and I believe
 > >  > this to be wrong. We call gdb_evaluate_expression when we create
 > >  > varobj, and it either succeeds, eventually setting varobj->value to
 > >  > something, or it does not. There's no point to call
 > >  > gdb_evaluate_expression again.
 > > 
 > > If gdb_evaluate_expression fails in varobj_create, a variable object is
 > > still created, but just with an undefined value.  It needs to be called to
 > > get value for VALUE_LVAL.
 > 
 > Why would a second call to gdb_evaluate_expression succeed? We already tried
 > to evaluate expression, we failed, varobj has no value whatsoever. So,
 > I'd say we should refuse to assign anything in this case.

The second call succeeds if the first one did (when the varobj does have
a value) - but we seem to be going backwards.
 
 > >  >                                                               Further,
 > >  > in varobj_create, gdb_evaluate_expression is called in specific frame,
 > >  > and varobj_editable_p calls it in current frame. 
 > > 
 > > The current frame should be the frame in which the variable object is defined.
 > > Can you explain why that's a problem?
 > 
 > Here's the code in question:
 > 
 >       /* When the frame is different from the current frame, 
 >          we must select the appropriate frame before parsing
 >          the expression, otherwise the value will not be current.
 >          Since select_frame is so benign, just call it for all cases. */
 >       if (fi != NULL)
 > 	{
 > 	  var->root->frame = get_frame_id (fi);
 > 	  old_fi = get_selected_frame (NULL);
 > 	  select_frame (fi);
 > 	}
 > 
 >       /* We definitively need to catch errors here.
 >          If evaluate_expression succeeds we got the value we wanted.
 >          But if it fails, we still go on with a call to evaluate_type()  */
 >       if (!gdb_evaluate_expression (var->root->exp, &value))
 > 
 > So it seems to be varobj is not necessary defined in the current frame.

When it's created it can be in a different frame according to the syntax.
However, in Emacs I only create watch expressions in the selected frame (the
manual suggests that "*" sets the value in the current frame but there
are many anomalies like this).

Likewise I think variable assignment should only be done in the selected frame.
Perhaps GDB should only allow this scenario, I don't know.  I have used this
patch for several months now without a problem and I'd rather focus on real
use cases than hypothetical pathological ones.

 >...  
 
 > > > 4. I don't think your test actually tests that the 'editable' attribute
 > > > comes out as 'false'.
 > > 
 > > I'm not sure what to say.  It shows that if you try to assign a value to a 
 > > cast GDB says "Variable object is not editable".
 > > 
 > > The error message for a variable object of a cast used to be:
 > > 
 > > &"mi_cmd_var_assign: Could not assign expression to variable object\n"
 > > ^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
 > > (gdb) 
 > 
 > Is this test failing with current GDB, and passing after your change?

The "not editable" message is new for this case.

 >                                                                     I think
 > GDB will refuse to assign value to rvalue anyway -- at least here's what I
 > get without your patch:
 > 
 > 	(gdb)
 > 	-var-create V * (float)argc
 > 	^done,name="V",numchild="0",value="0",type="float"
 > 	(gdb)
 > 	-var-assign V 10
 > 	&"mi_cmd_var_assign: Could not assign expression to variable object\n"
 > 	^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
 > 	(gdb)

That's what I've just said.

 > > I could add another test for -var-show-attributes which will now give:
 > > 
 > > ^done,attr="noneditable"
 > > 
 > > for a cast but I never use this command.
 > 
 > I think the biggest value of this patch is that we get this attribute,
 > *before* trying to assign the value, so yes, I think a test for this
 > specific behaviour would be great.

There's no before or after.  It would basically be the same test.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus]

On Tuesday 30 October 2007 07:39:34 Nick Roberts wrote:
>  > >  > 1. In varobj_editable_p you call gdb_evaluate_expression, and I believe
>  > >  > this to be wrong. We call gdb_evaluate_expression when we create
>  > >  > varobj, and it either succeeds, eventually setting varobj->value to
>  > >  > something, or it does not. There's no point to call
>  > >  > gdb_evaluate_expression again.
>  > > 
>  > > If gdb_evaluate_expression fails in varobj_create, a variable object is
>  > > still created, but just with an undefined value.  It needs to be called to
>  > > get value for VALUE_LVAL.
>  > 
>  > Why would a second call to gdb_evaluate_expression succeed? We already tried
>  > to evaluate expression, we failed, varobj has no value whatsoever. So,
>  > I'd say we should refuse to assign anything in this case.
> 
> The second call succeeds if the first one did (when the varobj does have
> a value) - but we seem to be going backwards.

Probably. Let me try again:

1. If the first call succeeds, we already have a value, and we can directly
check if that's lvalue. Why call gdb_evaluate_expression in that case?

2. If the first call fails, we get a varobj with no associated value at all.
I believe that var->value will be NULL. Why should varobj_editable_p try
to evaluate expression again? Specifically:

	- Why would this second call to gdb_evaluate succeed, if it failed
	previously?
	- If we reevaluate the expression in a frame different from
	frame where -var-create was invoked, we can get value that bears
	no relation to what was meant in -var-create, do we want this?
	- If var->value is NULL, then varobj_set_value will return
	immediately, doing nothing. So, actually, varobj with value of NULL
	are not editable anyway.

>  > >  >                                                               Further,
>  > >  > in varobj_create, gdb_evaluate_expression is called in specific frame,
>  > >  > and varobj_editable_p calls it in current frame. 
>  > > 
>  > > The current frame should be the frame in which the variable object is defined.
>  > > Can you explain why that's a problem?
>  > 
>  > Here's the code in question:
>  > 
>  >       /* When the frame is different from the current frame, 
>  >          we must select the appropriate frame before parsing
>  >          the expression, otherwise the value will not be current.
>  >          Since select_frame is so benign, just call it for all cases. */
>  >       if (fi != NULL)
>  > 	{
>  > 	  var->root->frame = get_frame_id (fi);
>  > 	  old_fi = get_selected_frame (NULL);
>  > 	  select_frame (fi);
>  > 	}
>  > 
>  >       /* We definitively need to catch errors here.
>  >          If evaluate_expression succeeds we got the value we wanted.
>  >          But if it fails, we still go on with a call to evaluate_type()  */
>  >       if (!gdb_evaluate_expression (var->root->exp, &value))
>  > 
>  > So it seems to be varobj is not necessary defined in the current frame.
> 
> When it's created it can be in a different frame according to the syntax.
> However, in Emacs I only create watch expressions in the selected frame (the
> manual suggests that "*" sets the value in the current frame but there
> are many anomalies like this).

Are you suggesting that GDB code should only work when used from Emacs? GDB,
as it stands, allows you to create a variable object in any frame, and your patch
does not work with that. So, either creating variable objects in any frame should
be disallowed, or you patch improved.

In fact, note that creating variable objects in specific frame is not necessary to
trigger problem. If you create variable in current frame, then change frame,
and then invoke varobj_editable_p, you'll evaluate expression in a different frame.
I don't see anyting wrong about this use case.

> Likewise I think variable assignment should only be done in the selected frame.

Err, why? I might have varobj for global variable called like 'g'. When I assign
value to that varobj, I don't really want any local variable called 'g' to get in.
Of course, real program would have the variable named differently, but still.

> Perhaps GDB should only allow this scenario, I don't know.  I have used this
> patch for several months now without a problem and I'd rather focus on real
> use cases than hypothetical pathological ones.

It's true that all new features should handle real use cases. However, it's
also necessary that MI interface is consistent. In current MI, an ordinary
(not "use selected frame") varobj uses the original frame for all operations --
for example -var-update. If any other operation starts to use selected frame,
it's inconsistent.

>  > > > 4. I don't think your test actually tests that the 'editable' attribute
>  > > > comes out as 'false'.
>  > > 
>  > > I'm not sure what to say.  It shows that if you try to assign a value to a 
>  > > cast GDB says "Variable object is not editable".
>  > > 
>  > > The error message for a variable object of a cast used to be:
>  > > 
>  > > &"mi_cmd_var_assign: Could not assign expression to variable object\n"
>  > > ^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
>  > > (gdb) 
>  > 
>  > Is this test failing with current GDB, and passing after your change?
> 
> The "not editable" message is new for this case.
> 
>  >                                                                     I think
>  > GDB will refuse to assign value to rvalue anyway -- at least here's what I
>  > get without your patch:
>  > 
>  > 	(gdb)
>  > 	-var-create V * (float)argc
>  > 	^done,name="V",numchild="0",value="0",type="float"
>  > 	(gdb)
>  > 	-var-assign V 10
>  > 	&"mi_cmd_var_assign: Could not assign expression to variable object\n"
>  > 	^error,msg="mi_cmd_var_assign: Could not assign expression to variable object"
>  > 	(gdb)
> 
> That's what I've just said.

So, in essence, the primary think you test is that this message now
say "Variable object is not editable"?

I personally think that the fact that -var-show-attributes reports rvalues as non-editable
is equally important, but if you don't feel like writing a test for that, up to you --
in which case I'll probably submit a patch myself (and maybe even make the 'editable' field
into regular varobj printing, as opposed to this 'attribute' thing).

- Volodya

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > I believe that var->value will be NULL.

Yes, that looks good.  I'll use:

  if (!var->value || !VALUE_LVAL(var->value))
    return 0;

and that should keep us both happy.

 > > When it's created it can be in a different frame according to the syntax.
 > > However, in Emacs I only create watch expressions in the selected frame
 > > (the manual suggests that "*" sets the value in the current frame but
 > > there are many anomalies like this).
 > 
 > Are you suggesting that GDB code should only work when used from Emacs? 

If I was, I would probably say something like "I suggest that GDB code should
only work when used from Emacs".  You're free read it how you like, but I was
just giving an example of real use.

 >                                                                        GDB,
 > as it stands, allows you to create a variable object in any frame, and your
 > patch does not work with that. So, either creating variable objects in any
 > frame should be disallowed, 

Actually I think that would be a good idea.  Generally a user would create a
watch expression in the frame he's examining.  If not, the frontend would have
to prompt for the frame.  Does KDevelop do this?

 >...
 > > Likewise I think variable assignment should only be done in the selected
 > > frame.
 > 
 > Err, why? I might have varobj for global variable called like 'g'. When I
 > assign value to that varobj, I don't really want any local variable called
 > 'g' to get in.  Of course, real program would have the variable named
 > differently, but still.

I'm not talking about global variables as they're valid in all frames.

 >...
 > > That's what I've just said.
 > 
 > So, in essence, the primary think you test is that this message now
 > say "Variable object is not editable"?

It's not just the message but that it's now triggered by:

  if (!(varobj_get_attributes (var) & 0x00000001))

instead of:

  if (!varobj_set_value (var, expression))

 > I personally think that the fact that -var-show-attributes reports rvalues
 > as non-editable is equally important, but if you don't feel like writing a
 > test for that, up to you -- in which case I'll probably submit a patch
 > myself (and maybe even make the 'editable' field into regular varobj
 > printing, as opposed to this 'attribute' thing).

mi_cmd_var_show_attributes calls varobj_get_attributes.  Like I said:
it's basically the same test.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Daniel Jacobowitz]

On Wed, Oct 31, 2007 at 12:04:53AM +1300, Nick Roberts wrote:
>  >                                                                        GDB,
>  > as it stands, allows you to create a variable object in any frame, and your
>  > patch does not work with that. So, either creating variable objects in any
>  > frame should be disallowed, 
> 
> Actually I think that would be a good idea.  Generally a user would create a
> watch expression in the frame he's examining.  If not, the frontend would have
> to prompt for the frame.  Does KDevelop do this?

If you have a GUI with a stack pane, it may be showing a lot of frames
at once.  Why force the front end to select a frmae before creating
the varobj?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > >  >                                                                    GDB,
 > >  > as it stands, allows you to create a variable object in any frame, and
 > >  > your patch does not work with that. So, either creating variable
 > >  > objects in any frame should be disallowed,
 > > 
 > > Actually I think that would be a good idea.  Generally a user would create
 > > a watch expression in the frame he's examining.  If not, the frontend
 > > would have to prompt for the frame.  Does KDevelop do this?
 > 
 > If you have a GUI with a stack pane, it may be showing a lot of frames
 > at once.  Why force the front end to select a frmae before creating
 > the varobj?

For a start -stack-list-frames (unlike `bt') doesn't list arguments or locals
so you couldn't create such a watch expression with a single mouse click.  But
I'm not suggesting removing such functionality, and we're moving away from the
original focus which was the submission of a patch with differentiates more
clearly between the editable and changeable properties of variable objects.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Daniel Jacobowitz]

On Wed, Oct 31, 2007 at 07:19:05AM +1300, Nick Roberts wrote:
> For a start -stack-list-frames (unlike `bt') doesn't list arguments or locals
> so you couldn't create such a watch expression with a single mouse
> click.

Of course you could.  The front end probably fetched the list of
locals for you when it built its GUI frame display.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: lvalues and variable_editable

[Daniel Jacobowitz]

On Wed, Oct 31, 2007 at 07:19:05AM +1300, Nick Roberts wrote:
> For a start -stack-list-frames (unlike `bt') doesn't list arguments or locals
> so you couldn't create such a watch expression with a single mouse
> click.

Of course you could.  The front end probably fetched the list of
locals for you when it built its GUI frame display.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus]

On Tuesday 30 October 2007 14:04:53 Nick Roberts wrote:
>  > I believe that var->value will be NULL.
> 
> Yes, that looks good.  I'll use:
> 
>   if (!var->value || !VALUE_LVAL(var->value))
>     return 0;
> 
> and that should keep us both happy.

As long there which code *replaces* gdb_evaluate_expression, I'm 
perfectly happy. Thanks!

>  >                                                                        GDB,
>  > as it stands, allows you to create a variable object in any frame, and your
>  > patch does not work with that. So, either creating variable objects in any
>  > frame should be disallowed, 
> 
> Actually I think that would be a good idea.  Generally a user would create a
> watch expression in the frame he's examining.  If not, the frontend would have
> to prompt for the frame.  Does KDevelop do this?

At the moment, KDevelop does not have any command that would create varobj
in non-current frame (although it's possible to create varobj in one frame,
and then assign to it while the current frame is different).
 
Daniel Jacobowitz later wrote:

> > Actually I think that would be a good idea.  Generally a user would
> > create a watch expression in the frame he's examining.  If not, the
> > frontend would have to prompt for the frame.  Does KDevelop do this?
>
> If you have a GUI with a stack pane, it may be showing a lot of frames
> at once.  Why force the front end to select a frmae before creating
> the varobj?

Yeah. For example, if stack widgets shows variables it would be quite
reasonable to allow expanding a variable, and creating varobj for
some nested field, and there's no need to switch frames.

(The problem with switching frames is that GUI and gdb will be temporary
out-of-sync, and generally all cases where GUI and gdb are out of sync
lead to bugs).

- Volodya

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > > For a start -stack-list-frames (unlike `bt') doesn't list arguments or
 > > locals so you couldn't create such a watch expression with a single mouse
 > > click.
 > 
 > Of course you could.  The front end probably fetched the list of
 > locals for you when it built its GUI frame display.

I'm not sure of your terminology but "GUI frame display" to me would mean
the locals (and possibly function arguments) of the _selected_ frame.

In any case, I'm proposing a patch that would not change the possibility of
doing what you are suggesting may be done.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > On Tuesday 30 October 2007 14:04:53 Nick Roberts wrote:
 > >  > I believe that var->value will be NULL.
 > > 
 > > Yes, that looks good.  I'll use:
 > > 
 > >   if (!var->value || !VALUE_LVAL(var->value))
 > >     return 0;
 > > 
 > > and that should keep us both happy.
 > 
 > As long there which code *replaces* gdb_evaluate_expression, I'm 
 > perfectly happy. Thanks!

Using var->value == NULL means that variable objects of pointers that cannot
be dereferenced are "noneditable".  This means that such variable objects
may change from being "editable" to "noneditable" during execution.  This
may not be a bad thing as discussion started from posts about not being
able to assign values to variable objects that were described as "editable".

I'll look at combining varobj_editable_p with the test in
mi_cmd_var_assign:

  if (!varobj_set_value (var, expression))
    error (_("mi_cmd_var_assign: Could not assign expression to variable object"

to give one message.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus]

On Thursday 01 November 2007 07:51:58 Nick Roberts wrote:
>  > On Tuesday 30 October 2007 14:04:53 Nick Roberts wrote:
>  > >  > I believe that var->value will be NULL.
>  > > 
>  > > Yes, that looks good.  I'll use:
>  > > 
>  > >   if (!var->value || !VALUE_LVAL(var->value))
>  > >     return 0;
>  > > 
>  > > and that should keep us both happy.
>  > 
>  > As long there which code *replaces* gdb_evaluate_expression, I'm 
>  > perfectly happy. Thanks!
> 
> Using var->value == NULL means that variable objects of pointers that cannot
> be dereferenced are "noneditable".  This means that such variable objects
> may change from being "editable" to "noneditable" during execution.  This
> may not be a bad thing as discussion started from posts about not being
> able to assign values to variable objects that were described as "editable".
> 
> I'll look at combining varobj_editable_p with the test in
> mi_cmd_var_assign:
> 
>   if (!varobj_set_value (var, expression))
>     error (_("mi_cmd_var_assign: Could not assign expression to variable object"
> 
> to give one message.

I'm not sure that's possible. If you create varobj for *foo, and foo changes
to point to inaccessible memory, then assignment to *foo will fail, but I
don't know any mechanism in gdb that will tell you that without actually
trying assignment.

- Volodya

Re: [PATCH] MI: lvalues and variable_editable

[Daniel Jacobowitz]

On Wed, Oct 31, 2007 at 10:47:40PM +0300, Vladimir Prus wrote:
> I'm not sure that's possible. If you create varobj for *foo, and foo changes
> to point to inaccessible memory, then assignment to *foo will fail, but I
> don't know any mechanism in gdb that will tell you that without actually
> trying assignment.

Yes, there is no way to be sure without trying to write.  Which we
shouldn't unless the user asked us to, of course.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > On Wed, Oct 31, 2007 at 10:47:40PM +0300, Vladimir Prus wrote:
 > > I'm not sure that's possible. If you create varobj for *foo, and foo changes
 > > to point to inaccessible memory, then assignment to *foo will fail, but I
 > > don't know any mechanism in gdb that will tell you that without actually
 > > trying assignment.
 > 
 > Yes, there is no way to be sure without trying to write.  Which we
 > shouldn't unless the user asked us to, of course.

In that case for such a varobj, var->value == NULL must indicate that the
memory was inaccessible at the _last_ update, or creation, and it may no longer
be inaccessible.  Describing it as "noneditable" may then be a bit misleading.

I'm surprised that there is "no way to be sure without trying to write".  I
know that 0x0 is inaccessible memory, and presumably the OS has a better idea
about what is and isn't accessible.  Are you saying that in some systems, for
example, you could replace a RAM chip with a ROM one, and only way to know
about it is by trying to write to it?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Daniel Jacobowitz]

On Fri, Nov 02, 2007 at 05:21:52PM +1300, Nick Roberts wrote:
> In that case for such a varobj, var->value == NULL must indicate that the
> memory was inaccessible at the _last_ update, or creation, and it may no longer
> be inaccessible.  Describing it as "noneditable" may then be a bit misleading.

The next time it's -var-update'd it may get a new value; presumably
the IDE does that periodically.

> I'm surprised that there is "no way to be sure without trying to write".  I
> know that 0x0 is inaccessible memory, and presumably the OS has a better idea
> about what is and isn't accessible.

It won't tell us, though.  Most systems have no interface to ask this
question.  Unix ptrace doesn't, hardware JTAG doesn't... whether you
can always write to RAM from a hardware JTAG device depends on whether
you're going through the MMU (memory management unit) or not for
writes.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > > In that case for such a varobj, var->value == NULL must indicate that the
 > > memory was inaccessible at the _last_ update, or creation, and it may no
 > > longer be inaccessible.  Describing it as "noneditable" may then be a bit
 > > misleading.
 > 
 > The next time it's -var-update'd it may get a new value; presumably
 > the IDE does that periodically.

I guess, in that respect it's no different to -var-evaluate-expression.  Here's
my latest patch for varobj.c.  I've removed the test for var->value != NULL in
varobj_set_value as it's already in varobj_editable_p.  I've also moved the
test for var->root->is_valid from varobj_get_attributes to varobj_editable_p.
This patch would require two x two changes to the testsuite:

FAIL: gdb.mi/mi2-var-child.exp: is weird.int_ptr_ptr.*int_ptr_ptr editable
FAIL: gdb.mi/mi2-var-child.exp: is weird.int_ptr_ptr.*int_ptr_ptr.**int_ptr_ptr 
editable

editable -> noneditable

likewise for mi-var-child.exp

There's also an independent change to mi_cmd_var_assign in mi-cmd-var.c that I
think makes it simpler.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


2007-11-03  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (c_variable_editable, cplus_variable_editable)
	(java_variable_editable, variable_editable): Delete.
	(varobj_editable_p): Replace above functions with one language
	independent function.  Check for an lvalue.
	(varobj_get_attributes, varobj_set_value): Use varobj_editable_p.
	(struct language_specific): Delete variable_editable field.

	* mi-cmd-var.c (mi_cmd_var_assign): Simplify.


*** varobj.c	27 Oct 2007 09:51:19 +1300	1.96
--- varobj.c	03 Nov 2007 21:50:19 +1300	
*************** static struct value *value_of_root (stru
*** 221,228 ****
  
  static struct value *value_of_child (struct varobj *parent, int index);
  
- static int variable_editable (struct varobj *var);
- 
  static char *my_value_of_variable (struct varobj *var);
  
  static char *value_get_print_value (struct value *value,
--- 221,226 ----
*************** static struct value *c_value_of_child (s
*** 248,255 ****
  
  static struct type *c_type_of_child (struct varobj *parent, int index);
  
- static int c_variable_editable (struct varobj *var);
- 
  static char *c_value_of_variable (struct varobj *var);
  
  /* C++ implementation */
--- 246,251 ----
*************** static struct value *cplus_value_of_chil
*** 270,277 ****
  
  static struct type *cplus_type_of_child (struct varobj *parent, int index);
  
- static int cplus_variable_editable (struct varobj *var);
- 
  static char *cplus_value_of_variable (struct varobj *var);
  
  /* Java implementation */
--- 266,271 ----
*************** static struct value *java_value_of_child
*** 290,297 ****
  
  static struct type *java_type_of_child (struct varobj *parent, int index);
  
- static int java_variable_editable (struct varobj *var);
- 
  static char *java_value_of_variable (struct varobj *var);
  
  /* The language specific vector */
--- 284,289 ----
*************** struct language_specific
*** 324,332 ****
    /* The type of the INDEX'th child of PARENT. */
    struct type *(*type_of_child) (struct varobj * parent, int index);
  
-   /* Is VAR editable? */
-   int (*variable_editable) (struct varobj * var);
- 
    /* The current value of VAR. */
    char *(*value_of_variable) (struct varobj * var);
  };
--- 316,321 ----
*************** static struct language_specific language
*** 343,349 ****
     c_value_of_root,
     c_value_of_child,
     c_type_of_child,
-    c_variable_editable,
     c_value_of_variable}
    ,
    /* C */
--- 332,337 ----
*************** static struct language_specific language
*** 356,362 ****
     c_value_of_root,
     c_value_of_child,
     c_type_of_child,
-    c_variable_editable,
     c_value_of_variable}
    ,
    /* C++ */
--- 344,349 ----
*************** static struct language_specific language
*** 369,375 ****
     cplus_value_of_root,
     cplus_value_of_child,
     cplus_type_of_child,
-    cplus_variable_editable,
     cplus_value_of_variable}
    ,
    /* Java */
--- 356,361 ----
*************** static struct language_specific language
*** 382,388 ****
     java_value_of_root,
     java_value_of_child,
     java_type_of_child,
-    java_variable_editable,
     java_value_of_variable}
  };
  
--- 368,373 ----
*************** varobj_create (char *objname,
*** 523,529 ****
  	  select_frame (fi);
  	}
  
!       /* We definitively need to catch errors here.
           If evaluate_expression succeeds we got the value we wanted.
           But if it fails, we still go on with a call to evaluate_type()  */
        if (!gdb_evaluate_expression (var->root->exp, &value))
--- 508,514 ----
  	  select_frame (fi);
  	}
  
!       /* We definitely need to catch errors here.
           If evaluate_expression succeeds we got the value we wanted.
           But if it fails, we still go on with a call to evaluate_type()  */
        if (!gdb_evaluate_expression (var->root->exp, &value))
*************** varobj_get_attributes (struct varobj *va
*** 856,862 ****
  {
    int attributes = 0;
  
!   if (var->root->is_valid && variable_editable (var))
      /* FIXME: define masks for attributes */
      attributes |= 0x00000001;	/* Editable */
  
--- 841,847 ----
  {
    int attributes = 0;
  
!   if (varobj_editable_p (var))
      /* FIXME: define masks for attributes */
      attributes |= 0x00000001;	/* Editable */
  
*************** varobj_set_value (struct varobj *var, ch
*** 887,940 ****
    struct value *value;
    int saved_input_radix = input_radix;
  
!   if (var->value != NULL && variable_editable (var))
!     {
!       char *s = expression;
!       int i;
  
!       input_radix = 10;		/* ALWAYS reset to decimal temporarily */
!       exp = parse_exp_1 (&s, 0, 0);
!       if (!gdb_evaluate_expression (exp, &value))
! 	{
! 	  /* We cannot proceed without a valid expression. */
! 	  xfree (exp);
! 	  return 0;
! 	}
! 
!       /* All types that are editable must also be changeable.  */
!       gdb_assert (varobj_value_is_changeable_p (var));
! 
!       /* The value of a changeable variable object must not be lazy.  */
!       gdb_assert (!value_lazy (var->value));
! 
!       /* Need to coerce the input.  We want to check if the
! 	 value of the variable object will be different
! 	 after assignment, and the first thing value_assign
! 	 does is coerce the input.
! 	 For example, if we are assigning an array to a pointer variable we
! 	 should compare the pointer with the the array's address, not with the
! 	 array's content.  */
!       value = coerce_array (value);
! 
!       /* The new value may be lazy.  gdb_value_assign, or 
! 	 rather value_contents, will take care of this.
! 	 If fetching of the new value will fail, gdb_value_assign
! 	 with catch the exception.  */
!       if (!gdb_value_assign (var->value, value, &val))
! 	return 0;
!      
!       /* If the value has changed, record it, so that next -var-update can
! 	 report this change.  If a variable had a value of '1', we've set it
! 	 to '333' and then set again to '1', when -var-update will report this
! 	 variable as changed -- because the first assignment has set the
! 	 'updated' flag.  There's no need to optimize that, because return value
! 	 of -var-update should be considered an approximation.  */
!       var->updated = install_new_value (var, val, 0 /* Compare values. */);
!       input_radix = saved_input_radix;
!       return 1;
      }
  
!   return 0;
  }
  
  /* Returns a malloc'ed list with all root variable objects */
--- 872,920 ----
    struct value *value;
    int saved_input_radix = input_radix;
  
!   char *s = expression;
!   int i;
  
!   input_radix = 10;		/* ALWAYS reset to decimal temporarily */
!   exp = parse_exp_1 (&s, 0, 0);
!   if (!gdb_evaluate_expression (exp, &value))
!     {
!       /* We cannot proceed without a valid expression. */
!       xfree (exp);
!       return 0;
      }
  
!   /* All types that are editable must also be changeable.  */
!   gdb_assert (varobj_value_is_changeable_p (var));
! 
!   /* The value of a changeable variable object must not be lazy.  */
!   gdb_assert (!value_lazy (var->value));
! 
!   /* Need to coerce the input.  We want to check if the
!      value of the variable object will be different
!      after assignment, and the first thing value_assign
!      does is coerce the input.
!      For example, if we are assigning an array to a pointer variable we
!      should compare the pointer with the the array's address, not with the
!      array's content.  */
!   value = coerce_array (value);
! 
!   /* The new value may be lazy.  gdb_value_assign, or 
!      rather value_contents, will take care of this.
!      If fetching of the new value will fail, gdb_value_assign
!      with catch the exception.  */
!   if (!gdb_value_assign (var->value, value, &val))
!     return 0;
!      
!   /* If the value has changed, record it, so that next -var-update can
!      report this change.  If a variable had a value of '1', we've set it
!      to '333' and then set again to '1', when -var-update will report this
!      variable as changed -- because the first assignment has set the
!      'updated' flag.  There's no need to optimize that, because return value
!      of -var-update should be considered an approximation.  */
!   var->updated = install_new_value (var, val, 0 /* Compare values. */);
!   input_radix = saved_input_radix;
!   return 1;
  }
  
  /* Returns a malloc'ed list with all root variable objects */
*************** value_of_child (struct varobj *parent, i
*** 1801,1814 ****
    return value;
  }
  
- /* Is this variable editable? Use the variable's type to make
-    this determination. */
- static int
- variable_editable (struct varobj *var)
- {
-   return (*var->root->lang->variable_editable) (var);
- }
- 
  /* GDB already has a command called "value_of_variable". Sigh. */
  static char *
  my_value_of_variable (struct varobj *var)
--- 1781,1786 ----
*************** value_get_print_value (struct value *val
*** 1840,1845 ****
--- 1812,1847 ----
    return thevalue;
  }
  
+ int
+ varobj_editable_p (struct varobj *var)
+ {
+   struct type *type;
+   struct value *value;
+ 
+   if (CPLUS_FAKE_CHILD (var))
+     return 0;
+ 
+   if (!(var->root->is_valid && var->value && VALUE_LVAL(var->value)))
+     return 0;
+ 
+   type = get_value_type (var);
+ 
+   switch (TYPE_CODE (type))
+     {
+     case TYPE_CODE_STRUCT:
+     case TYPE_CODE_UNION:
+     case TYPE_CODE_ARRAY:
+     case TYPE_CODE_FUNC:
+     case TYPE_CODE_METHOD:
+       return 0;
+       break;
+ 
+     default:
+       return 1;
+       break;
+     }
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report
*************** c_type_of_child (struct varobj *parent, 
*** 2212,2236 ****
    return type;
  }
  
- static int
- c_variable_editable (struct varobj *var)
- {
-   switch (TYPE_CODE (get_value_type (var)))
-     {
-     case TYPE_CODE_STRUCT:
-     case TYPE_CODE_UNION:
-     case TYPE_CODE_ARRAY:
-     case TYPE_CODE_FUNC:
-     case TYPE_CODE_METHOD:
-       return 0;
-       break;
- 
-     default:
-       return 1;
-       break;
-     }
- }
- 
  static char *
  c_value_of_variable (struct varobj *var)
  {
--- 2214,2219 ----
*************** cplus_type_of_child (struct varobj *pare
*** 2602,2616 ****
    return type;
  }
  
- static int
- cplus_variable_editable (struct varobj *var)
- {
-   if (CPLUS_FAKE_CHILD (var))
-     return 0;
- 
-   return c_variable_editable (var);
- }
- 
  static char *
  cplus_value_of_variable (struct varobj *var)
  {
--- 2585,2590 ----
*************** java_type_of_child (struct varobj *paren
*** 2694,2705 ****
    return cplus_type_of_child (parent, index);
  }
  
- static int
- java_variable_editable (struct varobj *var)
- {
-   return cplus_variable_editable (var);
- }
- 
  static char *
  java_value_of_variable (struct varobj *var)
  {
--- 2668,2673 ----


*** mi-cmd-var.c	03 Nov 2007 22:17:04 +1300	1.40
--- mi-cmd-var.c	03 Nov 2007 12:24:24 +1300	
*************** mi_cmd_var_assign (char *command, char *
*** 512,527 ****
      error (_("mi_cmd_var_assign: Variable object not found"));
  
    /* FIXME: define masks for attributes */
!   if (!(varobj_get_attributes (var) & 0x00000001))
      error (_("mi_cmd_var_assign: Variable object is not editable"));
  
!   expression = xstrdup (argv[1]);
! 
!   if (!varobj_set_value (var, expression))
!     error (_("mi_cmd_var_assign: Could not assign expression to variable object"));
  
!   ui_out_field_string (uiout, "value", varobj_get_value (var));
!   return MI_CMD_DONE;
  }
  
  enum mi_cmd_result
--- 512,529 ----
      error (_("mi_cmd_var_assign: Variable object not found"));
  
    /* FIXME: define masks for attributes */
!   if (!varobj_editable_p (var))
      error (_("mi_cmd_var_assign: Variable object is not editable"));
+   else
+     {
+       expression = xstrdup (argv[1]);
  
!       if (!varobj_set_value (var, expression))
! 	error (_("mi_cmd_var_assign: Could not assign expression to variable object"));
  
!       ui_out_field_string (uiout, "value", varobj_get_value (var));
!       return MI_CMD_DONE;
!     }
  }
  
  enum mi_cmd_result

Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus]

On Saturday 03 November 2007 12:22:56 Nick Roberts wrote:
>  > > In that case for such a varobj, var->value == NULL must indicate that the
>  > > memory was inaccessible at the _last_ update, or creation, and it may no
>  > > longer be inaccessible.  Describing it as "noneditable" may then be a bit
>  > > misleading.
>  > 
>  > The next time it's -var-update'd it may get a new value; presumably
>  > the IDE does that periodically.
> 
> I guess, in that respect it's no different to -var-evaluate-expression.  Here's
> my latest patch for varobj.c.  I've removed the test for var->value != NULL in
> varobj_set_value as it's already in varobj_editable_p.  I've also moved the
> test for var->root->is_valid from varobj_get_attributes to varobj_editable_p.
> This patch would require two x two changes to the testsuite:
> 
> FAIL: gdb.mi/mi2-var-child.exp: is weird.int_ptr_ptr.*int_ptr_ptr editable
> FAIL: gdb.mi/mi2-var-child.exp: is weird.int_ptr_ptr.*int_ptr_ptr.**int_ptr_ptr 
> editable
> 
> editable -> noneditable
> 
> likewise for mi-var-child.exp
> 
> There's also an independent change to mi_cmd_var_assign in mi-cmd-var.c that I
> think makes it simpler.
> 
> -- 
> Nick                                           http://www.inet.net.nz/~nickrob
> 
> 
> 2007-11-03  Nick Roberts  <nickrob@snap.net.nz>
> 
>         * varobj.c (c_variable_editable, cplus_variable_editable)
>         (java_variable_editable, variable_editable): Delete.
>         (varobj_editable_p): Replace above functions with one language
>         independent function.  Check for an lvalue.
>         (varobj_get_attributes, varobj_set_value): Use varobj_editable_p.
>         (struct language_specific): Delete variable_editable field.
> 
>         * mi-cmd-var.c (mi_cmd_var_assign): Simplify.
> 
> 
> *** varobj.c    27 Oct 2007 09:51:19 +1300      1.96
> --- varobj.c    03 Nov 2007 21:50:19 +1300      

> *************** varobj_set_value (struct varobj *var, ch
> *** 887,940 ****
>     struct value *value;
>     int saved_input_radix = input_radix;
>   
> !   if (var->value != NULL && variable_editable (var))
> !     {
> !       char *s = expression;
> !       int i;
>   
> !       input_radix = 10;               /* ALWAYS reset to decimal temporarily */
> !       exp = parse_exp_1 (&s, 0, 0);
> !       if (!gdb_evaluate_expression (exp, &value))
> !       {
> !         /* We cannot proceed without a valid expression. */
> !         xfree (exp);
> !         return 0;
> !       }
> ! 
> !       /* All types that are editable must also be changeable.  */
> !       gdb_assert (varobj_value_is_changeable_p (var));
> ! 
> !       /* The value of a changeable variable object must not be lazy.  */
> !       gdb_assert (!value_lazy (var->value));
> ! 
> !       /* Need to coerce the input.  We want to check if the
> !        value of the variable object will be different
> !        after assignment, and the first thing value_assign
> !        does is coerce the input.
> !        For example, if we are assigning an array to a pointer variable we
> !        should compare the pointer with the the array's address, not with the
> !        array's content.  */
> !       value = coerce_array (value);
> ! 
> !       /* The new value may be lazy.  gdb_value_assign, or 
> !        rather value_contents, will take care of this.
> !        If fetching of the new value will fail, gdb_value_assign
> !        with catch the exception.  */
> !       if (!gdb_value_assign (var->value, value, &val))
> !       return 0;
> !      
> !       /* If the value has changed, record it, so that next -var-update can
> !        report this change.  If a variable had a value of '1', we've set it
> !        to '333' and then set again to '1', when -var-update will report this
> !        variable as changed -- because the first assignment has set the
> !        'updated' flag.  There's no need to optimize that, because return value
> !        of -var-update should be considered an approximation.  */
> !       var->updated = install_new_value (var, val, 0 /* Compare values. */);
> !       input_radix = saved_input_radix;
> !       return 1;
>       }
>   
> !   return 0;
>   }
>   
>   /* Returns a malloc'ed list with all root variable objects */
> --- 872,920 ----
>     struct value *value;
>     int saved_input_radix = input_radix;

I suggest


	gdb_assert (varobj_is_editable_p (var));

here, so clearly document the assumptions the trailing
code makes.


> !   char *s = expression;
> !   int i;
>   
> !   input_radix = 10;           /* ALWAYS reset to decimal temporarily */
> !   exp = parse_exp_1 (&s, 0, 0);
> !   if (!gdb_evaluate_expression (exp, &value))
> !     {
> !       /* We cannot proceed without a valid expression. */
> !       xfree (exp);
> !       return 0;
>       }
>   
> !   /* All types that are editable must also be changeable.  */
> !   gdb_assert (varobj_value_is_changeable_p (var));
> ! 
> !   /* The value of a changeable variable object must not be lazy.  */
> !   gdb_assert (!value_lazy (var->value));
> ! 
> !   /* Need to coerce the input.  We want to check if the
> !      value of the variable object will be different
> !      after assignment, and the first thing value_assign
> !      does is coerce the input.
> !      For example, if we are assigning an array to a pointer variable we
> !      should compare the pointer with the the array's address, not with the
> !      array's content.  */
> !   value = coerce_array (value);
> ! 
> !   /* The new value may be lazy.  gdb_value_assign, or 
> !      rather value_contents, will take care of this.
> !      If fetching of the new value will fail, gdb_value_assign
> !      with catch the exception.  */
> !   if (!gdb_value_assign (var->value, value, &val))
> !     return 0;
> !      
> !   /* If the value has changed, record it, so that next -var-update can
> !      report this change.  If a variable had a value of '1', we've set it
> !      to '333' and then set again to '1', when -var-update will report this
> !      variable as changed -- because the first assignment has set the
> !      'updated' flag.  There's no need to optimize that, because return value
> !      of -var-update should be considered an approximation.  */
> !   var->updated = install_new_value (var, val, 0 /* Compare values. */);
> !   input_radix = saved_input_radix;
> !   return 1;
>   }
>   
>   /* Returns a malloc'ed list with all root variable objects */

> + int
> + varobj_editable_p (struct varobj *var)
> + {
> +   struct type *type;
> +   struct value *value;
> + 
> +   if (CPLUS_FAKE_CHILD (var))
> +     return 0;
> + 
> +   if (!(var->root->is_valid && var->value && VALUE_LVAL(var->value)))
> +     return 0;

I believe this test captures CPLUS_FAKE_CHILD as well, so you don't
need to have separate test for that.

> +   type = get_value_type (var);
> + 
> +   switch (TYPE_CODE (type))
> +     {
> +     case TYPE_CODE_STRUCT:
> +     case TYPE_CODE_UNION:
> +     case TYPE_CODE_ARRAY:
> +     case TYPE_CODE_FUNC:
> +     case TYPE_CODE_METHOD:
> +       return 0;
> +       break;
> + 
> +     default:
> +       return 1;
> +       break;
> +     }
> + }
> + 
>   /* Return non-zero if changes in value of VAR
>      must be detected and reported by -var-update.
>      Return zero is -var-update should never report


> *** mi-cmd-var.c        03 Nov 2007 22:17:04 +1300      1.40
> --- mi-cmd-var.c        03 Nov 2007 12:24:24 +1300      
> *************** mi_cmd_var_assign (char *command, char *
> *** 512,527 ****
>       error (_("mi_cmd_var_assign: Variable object not found"));
>   
>     /* FIXME: define masks for attributes */
> !   if (!(varobj_get_attributes (var) & 0x00000001))
>       error (_("mi_cmd_var_assign: Variable object is not editable"));
>   
> !   expression = xstrdup (argv[1]);
> ! 
> !   if (!varobj_set_value (var, expression))
> !     error (_("mi_cmd_var_assign: Could not assign expression to variable object"));
>   
> !   ui_out_field_string (uiout, "value", varobj_get_value (var));
> !   return MI_CMD_DONE;
>   }
>   
>   enum mi_cmd_result
> --- 512,529 ----
>       error (_("mi_cmd_var_assign: Variable object not found"));
>   
>     /* FIXME: define masks for attributes */
> !   if (!varobj_editable_p (var))
>       error (_("mi_cmd_var_assign: Variable object is not editable"));

I presume the FIXME is now stale?

> +   else
> +     {
> +       expression = xstrdup (argv[1]);
>   
> !       if (!varobj_set_value (var, expression))
> !       error (_("mi_cmd_var_assign: Could not assign expression to variable object"));
>   
> !       ui_out_field_string (uiout, "value", varobj_get_value (var));
> !       return MI_CMD_DONE;
> !     }
>   }
>   
>   enum mi_cmd_result

Aside from the minor points above, I don't have any objections to this patch. Dan,
what would you say?

- Volodya

 

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

Plus this change, of course.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


*** varobj.h	01 Sep 2007 22:35:10 +1200	1.12
--- varobj.h	03 Nov 2007 12:13:27 +1300	
*************** extern int varobj_update (struct varobj 
*** 114,117 ****
--- 114,119 ----
  
  extern void varobj_invalidate (void);
  
+ extern int varobj_editable_p (struct varobj *var);
+ 
  #endif /* VAROBJ_H */

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > >   /* Returns a malloc'ed list with all root variable objects */
 > > --- 872,920 ----
 > >     struct value *value;
 > >     int saved_input_radix = input_radix;
 > 
 > I suggest
 > 
 > 
 > 	gdb_assert (varobj_is_editable_p (var));
 > 
 > here, so clearly document the assumptions the trailing
 > code makes.

Sure.

 >...
 > > +   if (CPLUS_FAKE_CHILD (var))
 > > +     return 0;
 > > + 
 > > +   if (!(var->root->is_valid && var->value && VALUE_LVAL(var->value)))
 > > +     return 0;
 > 
 > I believe this test captures CPLUS_FAKE_CHILD as well, so you don't
 > need to have separate test for that.

I'll remove it.

 >...
 > >     /* FIXME: define masks for attributes */
 > > !   if (!varobj_editable_p (var))
 > >       error (_("mi_cmd_var_assign: Variable object is not editable"));
 > 
 > I presume the FIXME is now stale?

I'll remove that too

 >...
 >  Aside from the minor points above, I don't have any objections to this
 > patch. Dan, what would you say?
 > 
 > - Volodya

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus]

Nick Roberts <nickrob <at> snap.net.nz> writes:

>  > I believe this test captures CPLUS_FAKE_CHILD as well, so you don't
>  > need to have separate test for that.
> 
> I'll remove it.
...
>  > >     /* FIXME: define masks for attributes */
>  > > !   if (!varobj_editable_p (var))
>  > >       error (_("mi_cmd_var_assign: Variable object is not editable"));
>  > 
>  > I presume the FIXME is now stale?
> 
> I'll remove that too
...
>  >  Aside from the minor points above, I don't have any objections to this
>  > patch. Dan, what would you say?

Just to clarify -- who's got the ball now? Nick, are you planning to post a 
revised patch, or are you waiting for Dan to approve the reminder of the patch?

- Volodya

Re: [PATCH] MI: lvalues and variable_editable

[Daniel Jacobowitz]

On Sat, Nov 03, 2007 at 10:22:56PM +1300, Nick Roberts wrote:
> +   if (!(var->root->is_valid && var->value && VALUE_LVAL(var->value)))
> +     return 0;

Space before parenthesis.
>     /* FIXME: define masks for attributes */
> !   if (!varobj_editable_p (var))
>       error (_("mi_cmd_var_assign: Variable object is not editable"));
> +   else
> +     {
> +       expression = xstrdup (argv[1]);

No need to move all this code into an else; error will never return.

The patch seems fine to me, with Vladimir's comments.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts]

 > > +   if (!(var->root->is_valid && var->value && VALUE_LVAL(var->value)))
 > > +     return 0;
 > 
 > Space before parenthesis.
 > >     /* FIXME: define masks for attributes */
 > > !   if (!varobj_editable_p (var))
 > >       error (_("mi_cmd_var_assign: Variable object is not editable"));
 > > +   else
 > > +     {
 > > +       expression = xstrdup (argv[1]);
 > 
 > No need to move all this code into an else; error will never return.
 > 
 > The patch seems fine to me, with Vladimir's comments.

OK.  Below are the changes that I committed.

Thanks,

Nick                                           http://www.inet.net.nz/~nickrob


2007-11-21  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (c_variable_editable, cplus_variable_editable)
	(java_variable_editable, variable_editable): Delete.
	(varobj_editable_p): Replace above functions with one language
	independent function.  Check for an lvalue.
	(varobj_get_attributes, varobj_set_value): Use varobj_editable_p.
	(struct language_specific): Delete variable_editable field.

	* mi-cmd-var.c (mi_cmd_var_assign): Simplify.

	* varobj.h: Add extern for varobj_editable_p.


*** varobj.c.~1.97~	2007-11-12 19:44:40.000000000 +1300
--- varobj.c	2007-11-21 08:17:05.000000000 +1300
*************** static struct value *value_of_root (stru
*** 221,228 ****
  
  static struct value *value_of_child (struct varobj *parent, int index);
  
- static int variable_editable (struct varobj *var);
- 
  static char *my_value_of_variable (struct varobj *var);
  
  static char *value_get_print_value (struct value *value,
--- 221,226 ----
*************** static struct value *c_value_of_child (s
*** 248,255 ****
  
  static struct type *c_type_of_child (struct varobj *parent, int index);
  
- static int c_variable_editable (struct varobj *var);
- 
  static char *c_value_of_variable (struct varobj *var);
  
  /* C++ implementation */
--- 246,251 ----
*************** static struct value *cplus_value_of_chil
*** 270,277 ****
  
  static struct type *cplus_type_of_child (struct varobj *parent, int index);
  
- static int cplus_variable_editable (struct varobj *var);
- 
  static char *cplus_value_of_variable (struct varobj *var);
  
  /* Java implementation */
--- 266,271 ----
*************** static struct value *java_value_of_child
*** 290,297 ****
  
  static struct type *java_type_of_child (struct varobj *parent, int index);
  
- static int java_variable_editable (struct varobj *var);
- 
  static char *java_value_of_variable (struct varobj *var);
  
  /* The language specific vector */
--- 284,289 ----
*************** struct language_specific
*** 324,332 ****
    /* The type of the INDEX'th child of PARENT. */
    struct type *(*type_of_child) (struct varobj * parent, int index);
  
-   /* Is VAR editable? */
-   int (*variable_editable) (struct varobj * var);
- 
    /* The current value of VAR. */
    char *(*value_of_variable) (struct varobj * var);
  };
--- 316,321 ----
*************** static struct language_specific language
*** 343,349 ****
     c_value_of_root,
     c_value_of_child,
     c_type_of_child,
-    c_variable_editable,
     c_value_of_variable}
    ,
    /* C */
--- 332,337 ----
*************** static struct language_specific language
*** 356,362 ****
     c_value_of_root,
     c_value_of_child,
     c_type_of_child,
-    c_variable_editable,
     c_value_of_variable}
    ,
    /* C++ */
--- 344,349 ----
*************** static struct language_specific language
*** 369,375 ****
     cplus_value_of_root,
     cplus_value_of_child,
     cplus_type_of_child,
-    cplus_variable_editable,
     cplus_value_of_variable}
    ,
    /* Java */
--- 356,361 ----
*************** static struct language_specific language
*** 382,388 ****
     java_value_of_root,
     java_value_of_child,
     java_type_of_child,
-    java_variable_editable,
     java_value_of_variable}
  };
  
--- 368,373 ----
*************** varobj_create (char *objname,
*** 523,529 ****
  	  select_frame (fi);
  	}
  
!       /* We definitively need to catch errors here.
           If evaluate_expression succeeds we got the value we wanted.
           But if it fails, we still go on with a call to evaluate_type()  */
        if (!gdb_evaluate_expression (var->root->exp, &value))
--- 508,514 ----
  	  select_frame (fi);
  	}
  
!       /* We definitely need to catch errors here.
           If evaluate_expression succeeds we got the value we wanted.
           But if it fails, we still go on with a call to evaluate_type()  */
        if (!gdb_evaluate_expression (var->root->exp, &value))
*************** varobj_get_attributes (struct varobj *va
*** 856,862 ****
  {
    int attributes = 0;
  
!   if (var->root->is_valid && variable_editable (var))
      /* FIXME: define masks for attributes */
      attributes |= 0x00000001;	/* Editable */
  
--- 841,847 ----
  {
    int attributes = 0;
  
!   if (varobj_editable_p (var))
      /* FIXME: define masks for attributes */
      attributes |= 0x00000001;	/* Editable */
  
*************** varobj_set_value (struct varobj *var, ch
*** 886,940 ****
    struct expression *exp;
    struct value *value;
    int saved_input_radix = input_radix;
  
!   if (var->value != NULL && variable_editable (var))
!     {
!       char *s = expression;
!       int i;
  
!       input_radix = 10;		/* ALWAYS reset to decimal temporarily */
!       exp = parse_exp_1 (&s, 0, 0);
!       if (!gdb_evaluate_expression (exp, &value))
! 	{
! 	  /* We cannot proceed without a valid expression. */
! 	  xfree (exp);
! 	  return 0;
! 	}
! 
!       /* All types that are editable must also be changeable.  */
!       gdb_assert (varobj_value_is_changeable_p (var));
! 
!       /* The value of a changeable variable object must not be lazy.  */
!       gdb_assert (!value_lazy (var->value));
! 
!       /* Need to coerce the input.  We want to check if the
! 	 value of the variable object will be different
! 	 after assignment, and the first thing value_assign
! 	 does is coerce the input.
! 	 For example, if we are assigning an array to a pointer variable we
! 	 should compare the pointer with the the array's address, not with the
! 	 array's content.  */
!       value = coerce_array (value);
! 
!       /* The new value may be lazy.  gdb_value_assign, or 
! 	 rather value_contents, will take care of this.
! 	 If fetching of the new value will fail, gdb_value_assign
! 	 with catch the exception.  */
!       if (!gdb_value_assign (var->value, value, &val))
! 	return 0;
!      
!       /* If the value has changed, record it, so that next -var-update can
! 	 report this change.  If a variable had a value of '1', we've set it
! 	 to '333' and then set again to '1', when -var-update will report this
! 	 variable as changed -- because the first assignment has set the
! 	 'updated' flag.  There's no need to optimize that, because return value
! 	 of -var-update should be considered an approximation.  */
!       var->updated = install_new_value (var, val, 0 /* Compare values. */);
!       input_radix = saved_input_radix;
!       return 1;
      }
  
!   return 0;
  }
  
  /* Returns a malloc'ed list with all root variable objects */
--- 871,921 ----
    struct expression *exp;
    struct value *value;
    int saved_input_radix = input_radix;
+   char *s = expression;
+   int i;
  
!   gdb_assert (varobj_editable_p (var));
  
!   input_radix = 10;		/* ALWAYS reset to decimal temporarily */
!   exp = parse_exp_1 (&s, 0, 0);
!   if (!gdb_evaluate_expression (exp, &value))
!     {
!       /* We cannot proceed without a valid expression. */
!       xfree (exp);
!       return 0;
      }
  
!   /* All types that are editable must also be changeable.  */
!   gdb_assert (varobj_value_is_changeable_p (var));
! 
!   /* The value of a changeable variable object must not be lazy.  */
!   gdb_assert (!value_lazy (var->value));
! 
!   /* Need to coerce the input.  We want to check if the
!      value of the variable object will be different
!      after assignment, and the first thing value_assign
!      does is coerce the input.
!      For example, if we are assigning an array to a pointer variable we
!      should compare the pointer with the the array's address, not with the
!      array's content.  */
!   value = coerce_array (value);
! 
!   /* The new value may be lazy.  gdb_value_assign, or 
!      rather value_contents, will take care of this.
!      If fetching of the new value will fail, gdb_value_assign
!      with catch the exception.  */
!   if (!gdb_value_assign (var->value, value, &val))
!     return 0;
!      
!   /* If the value has changed, record it, so that next -var-update can
!      report this change.  If a variable had a value of '1', we've set it
!      to '333' and then set again to '1', when -var-update will report this
!      variable as changed -- because the first assignment has set the
!      'updated' flag.  There's no need to optimize that, because return value
!      of -var-update should be considered an approximation.  */
!   var->updated = install_new_value (var, val, 0 /* Compare values. */);
!   input_radix = saved_input_radix;
!   return 1;
  }
  
  /* Returns a malloc'ed list with all root variable objects */
*************** value_of_child (struct varobj *parent, i
*** 1803,1816 ****
    return value;
  }
  
- /* Is this variable editable? Use the variable's type to make
-    this determination. */
- static int
- variable_editable (struct varobj *var)
- {
-   return (*var->root->lang->variable_editable) (var);
- }
- 
  /* GDB already has a command called "value_of_variable". Sigh. */
  static char *
  my_value_of_variable (struct varobj *var)
--- 1784,1789 ----
*************** value_get_print_value (struct value *val
*** 1842,1847 ****
--- 1815,1847 ----
    return thevalue;
  }
  
+ int
+ varobj_editable_p (struct varobj *var)
+ {
+   struct type *type;
+   struct value *value;
+ 
+   if (!(var->root->is_valid && var->value && VALUE_LVAL (var->value)))
+     return 0;
+ 
+   type = get_value_type (var);
+ 
+   switch (TYPE_CODE (type))
+     {
+     case TYPE_CODE_STRUCT:
+     case TYPE_CODE_UNION:
+     case TYPE_CODE_ARRAY:
+     case TYPE_CODE_FUNC:
+     case TYPE_CODE_METHOD:
+       return 0;
+       break;
+ 
+     default:
+       return 1;
+       break;
+     }
+ }
+ 
  /* Return non-zero if changes in value of VAR
     must be detected and reported by -var-update.
     Return zero is -var-update should never report
*************** c_type_of_child (struct varobj *parent, 
*** 2214,2238 ****
    return type;
  }
  
- static int
- c_variable_editable (struct varobj *var)
- {
-   switch (TYPE_CODE (get_value_type (var)))
-     {
-     case TYPE_CODE_STRUCT:
-     case TYPE_CODE_UNION:
-     case TYPE_CODE_ARRAY:
-     case TYPE_CODE_FUNC:
-     case TYPE_CODE_METHOD:
-       return 0;
-       break;
- 
-     default:
-       return 1;
-       break;
-     }
- }
- 
  static char *
  c_value_of_variable (struct varobj *var)
  {
--- 2214,2219 ----
*************** cplus_type_of_child (struct varobj *pare
*** 2604,2618 ****
    return type;
  }
  
- static int
- cplus_variable_editable (struct varobj *var)
- {
-   if (CPLUS_FAKE_CHILD (var))
-     return 0;
- 
-   return c_variable_editable (var);
- }
- 
  static char *
  cplus_value_of_variable (struct varobj *var)
  {
--- 2585,2590 ----
*************** java_type_of_child (struct varobj *paren
*** 2696,2707 ****
    return cplus_type_of_child (parent, index);
  }
  
- static int
- java_variable_editable (struct varobj *var)
- {
-   return cplus_variable_editable (var);
- }
- 
  static char *
  java_value_of_variable (struct varobj *var)
  {


*** mi-cmd-var.c.~1.40~	2007-11-03 22:17:04.000000000 +1300
--- mi-cmd-var.c	2007-11-21 08:19:11.000000000 +1300
*************** mi_cmd_var_assign (char *command, char *
*** 511,518 ****
    if (var == NULL)
      error (_("mi_cmd_var_assign: Variable object not found"));
  
!   /* FIXME: define masks for attributes */
!   if (!(varobj_get_attributes (var) & 0x00000001))
      error (_("mi_cmd_var_assign: Variable object is not editable"));
  
    expression = xstrdup (argv[1]);
--- 511,517 ----
    if (var == NULL)
      error (_("mi_cmd_var_assign: Variable object not found"));
  
!   if (!varobj_editable_p (var))
      error (_("mi_cmd_var_assign: Variable object is not editable"));
  
    expression = xstrdup (argv[1]);


*** varobj.h.~1.12.~	2007-09-01 22:35:10.000000000 +1200
--- varobj.h	2007-11-03 12:13:27.000000000 +1300
*************** extern int varobj_update (struct varobj 
*** 114,117 ****
--- 114,119 ----
  
  extern void varobj_invalidate (void);
  
+ extern int varobj_editable_p (struct varobj *var);
+ 
  #endif /* VAROBJ_H */