Re: [PATCH] MI: lvalues and variable_editable

[Vladimir Prus <ghost@cs.msu.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