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

Jan Kratochvil
2012-09-10T07:47:10Z

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