Re: [PATCH] MI: lvalues and variable_editable

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

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