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

[dje at google dot com]

Yao Qi writes:
 > On 08/24/2012 12:24 AM, Tom Tromey wrote:
 > > Doug> fwiw, I've been able to work around corrupt, or otherwise not
 > > Doug> completely useful, core files by setting registers in non-innermost
 > > Doug> frames.
 > > Doug> Granted, I knew what was happening underneath the covers, so to speak,
 > > Doug> and I could have done things differently, but I like this capability.
 > > 
 > > It does seem like a useful power user tool.
 > > I'm swayed by this argument.
 > 
 > OK, looks like I underestimate the power of it. :)
 > 
 > > 
 > > Doug> I*could*  accept a warning when changing a register in a non-innermost
 > > Doug> frame, fwiw.
 > > 
 > > That would be ok by me.
 > 
 > Anyway, this new patch is to emit warning if user modifies register on
 > non-innermost frame.  Regression tested on x86_64-linux native and
 > gdbserver.  OK to apply?
 > 
 > Note that this test case should be tuned for each target, because of
 > registers.  So far, this test is run effectively on x86 and x86_64.
 > 
 > -- 
 > Yao
 > 
 > gdb/testsuite:
 > 
 > 2012-08-29  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* gdb.base/set-reg.exp: New.
 > 	* gdb.base/set-reg.c: New.
 > 
 > gdb:
 > 
 > 2012-08-29  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* valops.c (value_assign): Emit warning if get_next_frame returns
 > 	non-NULL.

The description here is insufficient.
I look at it and think "Eh?" (e.g. it says nothing about register values).
The point is that we're warning about assigning to registers in non-innermost
frames, so say that.

	* valops.c (value_assign): Emit warning when assigning to registers
	in non-innermost frames.

I think this is NEWS-worthy.

 > ---
 >  gdb/testsuite/gdb.base/set-reg.c   |   40 +++++++++++++
 >  gdb/testsuite/gdb.base/set-reg.exp |  108 ++++++++++++++++++++++++++++++++++++
 >  gdb/valops.c                       |   12 ++++
 >  3 files changed, 160 insertions(+), 0 deletions(-)
 >  create mode 100644 gdb/testsuite/gdb.base/set-reg.c
 >  create mode 100644 gdb/testsuite/gdb.base/set-reg.exp
 > 
 > diff --git a/gdb/testsuite/gdb.base/set-reg.c b/gdb/testsuite/gdb.base/set-reg.c
 > new file mode 100644
 > index 0000000..7ce350d
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.base/set-reg.c
 > @@ -0,0 +1,40 @@
 > +/* This test is part of GDB, the GNU debugger.
 > +
 > +   Copyright 2012 Free Software Foundation, Inc.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 3 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > +
 > +int
 > +inner (int x)
 > +{
 > +  return x + 2;
 > +}
 > +
 > +static short

The mixture short vs int, extern vs static, is excessive.
How about make inner,middle,top all "static int" ?

 > +middle (int x)
 > +{
 > +  return 2 * inner (x);
 > +}
 > +
 > +short
 > +top (int x)
 > +{
 > +  return 2 * middle (x);
 > +}
 > +
 > +int
 > +main (int argc,  char **argv)
 > +{
 > +  return top (argc);
 > +}
 > diff --git a/gdb/testsuite/gdb.base/set-reg.exp b/gdb/testsuite/gdb.base/set-reg.exp
 > new file mode 100644
 > index 0000000..3eace34
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.base/set-reg.exp
 > @@ -0,0 +1,108 @@
 > +# Copyright 2012 Free Software Foundation, Inc.
 > +#
 > +# This program is free software; you can redistribute it and/or modify
 > +# it under the terms of the GNU General Public License as published by
 > +# the Free Software Foundation; either version 3 of the License, or
 > +# (at your option) any later version.
 > +#
 > +# This program is distributed in the hope that it will be useful,
 > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +# GNU General Public License for more details.
 > +#
 > +# You should have received a copy of the GNU General Public License
 > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
 > +
 > +standard_testfile
 > +set executable $testfile
 > +
 > +if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable {debug}] != "" } {
 > +    untested "Failed to compile"
 > +    return -1
 > +}
 > +
 > +# A value in register in previous frame.

-->
# A value in register in previous frame.  E.g,. call-clobbered register.

 > +set reg_prev_frame_in_reg "unknown"
 > +# A value in memory in previous frame.

-->
# A (register) value in memory in previous frame.  E.g., call-saved register.

 > +set reg_prev_frame_in_memory "unknown"
 > +
 > +if [is_amd64_regs_target] {
 > +    set reg_prev_frame_in_reg "rax"
 > +    set reg_prev_frame_in_memory "rbp"
 > +} elseif [is_x86_like_target] {
 > +    set reg_prev_frame_in_reg "eax"
 > +    set reg_prev_frame_in_memory "ebp"
 > +} else {
 > +    # set reg_prev_frame_in_reg for other arch.

Remove extra blank line before }.

 > +
 > +}
 > +
 > +# Test setting register on innermost frame and non-innermost frame.
 > +
 > +proc test_set_reg_on_frame {} {with_test_prefix "on frame" {

Does prefix "on frame" provide anything useful here?
[IOW, what would we lose if we remove it?]

 > +    global executable
 > +    global reg_prev_frame_in_reg
 > +    global reg_prev_frame_in_memory
 > +    global decimal
 > +    global hex
 > +    global expect_out
 > +    global gdb_prompt
 > +
 > +    clean_restart $executable
 > +
 > +    if ![runto_main] {
 > +	return -1

We don't put much effort into caring about return values when, e.g.,
runto_main fails (throughout the testsuite).
But here, how about just "return".

 > +    }
 > +
 > +    # Name of register is not set, skip the rest of test.
 > +    if { [string equal $reg_prev_frame_in_reg "unknown"] } {
 > +	return 1
 > +    }

Why have this exit test here?  Seems pretty inefficient.
How about doing the test in the caller (top level)?
[And even skip the compilation step if there's no point.]

 > +
 > +    gdb_test "break inner" "Breakpoint.*at.* file .*, line.*"
 > +    gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*inner.*at.*" \
 > +	"continue to inner"
 > +
 > +    gdb_test_sequence "bt" "bt before setting reg" {
 > +	"\[\r\n\]#0 .* inner \\(x=1\\) at "
 > +	"\[\r\n\]#1 .* middle \\(x=1\\) at "
 > +	"\[\r\n\]#2 .* top \\(x=1\\) at "
 > +	"\[\r\n\]#3 .* main \\(.*\\) at"
 > +    }
 > +
 > +    set reg_val ""
 > +    gdb_test_multiple "p/x \$${reg_prev_frame_in_reg}" "print register ${reg_prev_frame_in_reg}" {
 > +	-re ".*${decimal} = ($hex).*$gdb_prompt $" {
 > +	    set reg_val $expect_out(1,string)
 > +	}
 > +    }

If reg_val is "" at this point the test is in trouble.
Better watch for it and handle it appropriately.

Plus you assign reg_val to both the in_reg and in_memory regs.
Given that one is rax and the other is rbp, that seems awkward.

One thing that's missing is that if we're going to test assigning
to registers, IWBN to test that the assignment worked.
If we assign the value it already had, the assignment may have failed
but we wouldn't (as easily) find out.

 > +
 > +    gdb_test_no_output "set \$${reg_prev_frame_in_reg}=${reg_val}" \
 > +	"set register ${reg_prev_frame_in_reg} on inner frame"
 > +
 > +    # Stack is unchanged.
 > +    gdb_test_sequence "bt" "bt after setting reg" {
 > +	"\[\r\n\]#0 .* inner \\(x=1\\) at "
 > +	"\[\r\n\]#1 .* middle \\(x=1\\) at "
 > +	"\[\r\n\]#2 .* top \\(x=1\\) at "
 > +	"\[\r\n\]#3 .* main \\(.*\\) at"
 > +    }
 > +
 > +    gdb_test "up"
 > +    gdb_test "set \$${reg_prev_frame_in_reg}=${reg_val}" \
 > +	"Frame in which this value is assigned to is not innermost.*" \
 > +	"set register ${reg_prev_frame_in_reg} on non-inner frame"
 > +
 > +   gdb_test_sequence "bt" "bt after up" {
 > +	"\[\r\n\]#0 .* inner \\(x=1\\) at "
 > +	"\[\r\n\]#1 .* middle \\(x=1\\) at "
 > +	"\[\r\n\]#2 .* top \\(x=1\\) at "
 > +	"\[\r\n\]#3 .* main \\(.*\\) at"
 > +    }
 > +
 > +    gdb_test "set \$${reg_prev_frame_in_memory}=${reg_val}" \
 > +	"Frame in which this value is assigned to is not innermost.*" \
 > +	"set register ${reg_prev_frame_in_memory} on non-inner frame"
 > +}}
 > +
 > +test_set_reg_on_frame
 > diff --git a/gdb/valops.c b/gdb/valops.c
 > index 17696ee..cb84c87 100644
 > --- a/gdb/valops.c
 > +++ b/gdb/valops.c
 > @@ -1241,6 +1241,18 @@ 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
 > +     lval_register.  */

IWBN to expand on this a bit.

E.g.,
"A call-saved register saved in memory will have VALUE_REGNUM >= 0
but VALUE_LVAL == lval_memory."
Or some such.

 > +  if (VALUE_REGNUM (toval) >= 0)
 > +    {
 > +      /* 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)
 > +	warning (_("Frame in which this value is assigned to is"
 > +		   " not innermost."));

IWBN to mention that this warning explicitly applies to registers.
Maybe something like:
"Assigning to register in non-innermost frame."

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