Re: [RFC] Don't allow setting register in non-innermost frame

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Mon, 10 Sep 2012 04:00:58 +0200, Yao Qi wrote:
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/set-reg.c
[...]
> +main (int argc,  char **argv)

Two spaces.


[...]
> --- a/gdb/valops.c
> +++ b/gdb/valops.c
> @@ -1241,6 +1241,22 @@ value_assign (struct value *toval, struct value *fromval)
>       and then restore the new frame afterwards.  */
>    old_frame = get_frame_id (deprecated_safe_get_selected_frame ());
>  
> +  /* TOVAL is a register, although VALUE_LVAL(TOVAL) may not be

GNU formatting applies also to comments: VALUE_LVAL (TOVAL)


> +     lval_register.  A call-saved register saved in memory will have
> +     'VALUE_REGNUM >= 0' but 'VALUE_LVAL == lval_memory'.  We also have to
> +     avoid emitting warning when assign value to some local variables which
> +     are stored in registers, TYPE_OBJFILE_OWNED helps to differentiate
> +     we are assigning to a register explicitly or to a variable saved in
> +     register.  */
> +  if (VALUE_REGNUM (toval) >= 0 && !TYPE_OBJFILE_OWNED (type))

There should be a better comment at value->regnum:

  /* Register number if the value is from a register.  */
  short regnum;

as currently it looks to me that value->regnum is not defined for
	value->lval != lval_register

This your patch IMO exploits side-effect behavior of value_of_register
function implementation, it would be good to document we depend now on this
REGNUM meaning in both value->regnum and in value_of_register.


> +    {
> +      /* Figure out which frame this is in currently.  */
> +      struct frame_info *frame = frame_find_by_id (VALUE_FRAME_ID (toval));
> +
> +      if (get_next_frame (frame) != NULL)

This is not safe, I do not have a countercase reproducer but in general
frame_find_by_id can return NULL and even the code below checks for it:
    case lval_register:
[...]
        frame = frame_find_by_id (VALUE_FRAME_ID (toval));
[...]
        if (!frame)
          error (_("Value being assigned to is no longer active."));

Something could call reinit_frame_cache in the meantime (see the issues from
PR 13866) and then frame_ids may become stale.

Either put there also the error check/call or I would find easier:
	if (frame_relative_level (frame) == 0)


> +	warning (_("Assigning to register in non-innermost frame."));

Are you / other people really against a query() here?

This way if one does the non-zero frame assignment it will print the warning.
User says oops, I did not want to do it - but the damage has been already
done, unintended memory is overwritten and there is no way back.

I was suggesting something like:

  if (!query (_("Really assign to stored register in non-innermost frame? ")))
    error (_("Not confirmed."));

I understand you are more concerned with MI but if I read correctly MI will
answer it as 'y', unaware whether the query message gets propagated to your MI
frontend so maybe you would like:

  if (query (_("Really assign to stored register in non-innermost frame? ")))
    warning (_("Assigning to register in non-innermost frame."));
  else
    error (_("Not confirmed."));


> +    }
> +
>    switch (VALUE_LVAL (toval))
>      {
>      case lval_internalvar:
> -- 
> 1.7.7.6


Thanks,
Jan