Re: [PATCH] MI: lvalues and variable_editable

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

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