Re: [PATCH] MI: lvalues and variable_editable

[Nick Roberts <nickrob@snap.net.nz>]

 > >  > 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