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

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

[Yao Qi]

Hi,
When playing with 'set $reg=0xXXX', I am thinking about the expected
behaviour of setting register when the frame is not innermost.  Current
GDB will invalidate regcaches and reinit frache_caches for any register
changes (on innermost and non-innermost frame).  However, the semantics
of 'setting register on non-innermost frame' is tricky.  On innermost
frame, 'setting register' means writing something to register in cpu,
while on non-innermost frame, it means writing something to either
real register (regcache) or memory (frame).  On the other head, I am
trying to figure out a situation that user has to set register on
non-innermost frame.  Why does this user have to do that?

Literally, IMO, 'set $reg=0xXXXX' means 'setting the cpu register $reg
to value 0xXXX', which is simple and clear.  I propose to restrict
setting register in innermost frame only.  WDYT?

Regression tested on x86_64-linux.  Surprisingly, we don't have a test
case for 'set register', so I add one.  In the future, I plan to add
more tests here for 'set register', such as 'set some bits of a register'
and 'set multiple registers in one time', to cover the different
paths in value_assign for 'lval_register'.

gdb:
	* valops.c (value_assign): Emit error if get_next_frame returns
	non-NULL.

gdb/testsuite:

 	* gdb.base/set-reg.exp: New.
---
 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
+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..abe7764
--- /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.
+set reg_prev_frame_in_reg "unknown"
+# A value in memory in previous frame.
+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.
+
+}
+
+# Test setting register on innermost frame and non-innermost frame.
+
+proc test_set_reg_on_frame {} {with_test_prefix "on frame" {
+    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
+    }
+
+    # Name of register is not set, skip the rest of test.
+    if { [string equal $reg_prev_frame_in_reg "unknown"] } {
+	return 1
+    }
+
+    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)
+	}
+    }
+
+    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 "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"
+
+   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"
+    }
+}}
+
+test_set_reg_on_frame
diff --git a/gdb/valops.c b/gdb/valops.c
index 8167cd4..14540b8 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.  */
+  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)
+	error (_("Frame in which this value is assigned to is"
+		 " not innermost."));
+    }
+
   switch (VALUE_LVAL (toval))
     {
     case lval_internalvar:
-- 
1.7.7.6

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

[Doug Evans]

On Thu, Aug 16, 2012 at 7:20 PM, Yao Qi <yao@codesourcery.com> wrote:
> Hi,
> When playing with 'set $reg=0xXXX', I am thinking about the expected
> behaviour of setting register when the frame is not innermost.  Current
> GDB will invalidate regcaches and reinit frache_caches for any register
> changes (on innermost and non-innermost frame).  However, the semantics
> of 'setting register on non-innermost frame' is tricky.  On innermost
> frame, 'setting register' means writing something to register in cpu,
> while on non-innermost frame, it means writing something to either
> real register (regcache) or memory (frame).  On the other head, I am
> trying to figure out a situation that user has to set register on
> non-innermost frame.  Why does this user have to do that?
>
> Literally, IMO, 'set $reg=0xXXXX' means 'setting the cpu register $reg
> to value 0xXXX', which is simple and clear.  I propose to restrict
> setting register in innermost frame only.  WDYT?
>
> Regression tested on x86_64-linux.  Surprisingly, we don't have a test
> case for 'set register', so I add one.  In the future, I plan to add
> more tests here for 'set register', such as 'set some bits of a register'
> and 'set multiple registers in one time', to cover the different
> paths in value_assign for 'lval_register'.

I wonder if there'll be a bit of a gnome vs kde battle here.  :-)
[the analogy isn't entirely accurate, but it's the best one I could think of]

fwiw, I've been able to work around corrupt, or otherwise not
completely useful, core files by setting registers in non-innermost
frames.
Granted, I knew what was happening underneath the covers, so to speak,
and I could have done things differently, but I like this capability.

If gdb had started out disallowing changing registers in non-innermost
frames, we mightn't be thinking of restricting it now.
"It's easier to relax restrictions than it is to impose them after the fact."
I'd like to hear more justification for this change.
I *could* accept a warning when changing a register in a non-innermost
frame, fwiw.

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

[Yao Qi]

On 08/21/2012 04:19 AM, Doug Evans wrote:
> fwiw, I've been able to work around corrupt, or otherwise not
> completely useful, core files by setting registers in non-innermost
> frames.
> Granted, I knew what was happening underneath the covers, so to speak,
> and I could have done things differently, but I like this capability.
>

Yeah, it is convenient, but sometimes it is misleading.  People who are 
clear on what is happening underneath can modify the frame/memory 
directly where the registers are saved.

> If gdb had started out disallowing changing registers in non-innermost
> frames, we mightn't be thinking of restricting it now.
> "It's easier to relax restrictions than it is to impose them after the fact."

Even if the fact is not clear or misleading sometimes?

> I'd like to hear more justification for this change.

Unfortunately, I don't have extra justification here.  I am writing a 
new MI notification when register is modified by 'set $reg=FOO' in 
console, similar to the patches on 'command parameter change' 
notification.  When the register is modified in innermost frame, a MI 
notification (with frame info) can be sent to MI frontend, and frontend 
can update its register contents, such as "register view".  However, if 
register is modified in non-innermost frame, the notification is not 
useful to frontend.  Then, I started to think of the meaning of 'setting 
register in non-innermost frame', and proposed this change finally.

-- 
Yao

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

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

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.

Doug> I *could* accept a warning when changing a register in a non-innermost
Doug> frame, fwiw.

That would be ok by me.

Tom

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

[Yao Qi]

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.
---
 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
+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.
+set reg_prev_frame_in_reg "unknown"
+# A value in memory in previous frame.
+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.
+
+}
+
+# Test setting register on innermost frame and non-innermost frame.
+
+proc test_set_reg_on_frame {} {with_test_prefix "on frame" {
+    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
+    }
+
+    # Name of register is not set, skip the rest of test.
+    if { [string equal $reg_prev_frame_in_reg "unknown"] } {
+	return 1
+    }
+
+    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)
+	}
+    }
+
+    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.  */
+  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."));
+    }
+
   switch (VALUE_LVAL (toval))
     {
     case lval_internalvar:
-- 
1.7.7.6

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

[dje]

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

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

[Yao Qi]

On 09/05/2012 06:36 AM, dje@google.com wrote:
>   >
>   > 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.
> 

OK.

> I think this is NEWS-worthy.
> 

I add a NEWS entry.


>   > +
>   > +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" ?
> 

Sure.  The set-reg.c is copied from other c file in test suite, and I
 didn't change it much.


>   > +# 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.
> 

Fixed.

>   > +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 }.
> 

This blank line is removed.

>   > +
>   > +}
>   > +
>   > +# 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?]
> 

At this moment, it doesn't.  However, I plan to add other tests in 
set-reg.exp in the future, so I add this prefix to avoid duplicated 
result.

>   > +    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".
> 

OK.

>   > +    }
>   > +
>   > +    # 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.]
> 

This check can be moved to the very beginning of test, even before compilation step.

>   > +
>   > +    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.
> 

OK, it is handled in new version.

> 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.
> 

The original intention of this test is to verify that an error or
 warning is emitted when changing register on non-inner most frame.  So 
it doesn't cover 'checking register content is really changed', as you 
pointed out.  I am fine to add them.  Note that modifying 'ebp' on 
non-inner most frame will get registers in mess, so I don't check the 
contents of 'ebp'.

>   > +
>   > +    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.
> 

Copied your sentence to the comment directly :).

>   > +  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."
> 

That is better.

-- 
Yao

gdb/testsuite:

2012-09-07  Yao Qi  <yao@codesourcery.com>

	* gdb.base/set-reg.exp: New.
	* gdb.base/set-reg.c: New.

gdb:

2012-09-07  Yao Qi  <yao@codesourcery.com>
	    Doug Evans  <dje@google.com>

	* valops.c (value_assign): Emit warning when assigning to
	registers in non-innermost frames.
	* NEWS: Mention it.
---
 gdb/NEWS                           |    3 +
 gdb/testsuite/gdb.base/set-reg.c   |   40 ++++++++++
 gdb/testsuite/gdb.base/set-reg.exp |  139 ++++++++++++++++++++++++++++++++++++
 gdb/valops.c                       |   12 +++
 4 files changed, 194 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/NEWS b/gdb/NEWS
index dba6937..5f1eb67 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -17,6 +17,9 @@
 * The 'cd' command now defaults to using '~' (the home directory) if not
   given an argument.
 
+* GDB will display a warning when assigning to registers in
+  non-innermost frames.
+
 * New configure options
 
 --enable-libmcheck/--disable-libmcheck
diff --git a/gdb/testsuite/gdb.base/set-reg.c b/gdb/testsuite/gdb.base/set-reg.c
new file mode 100644
index 0000000..03938eb
--- /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/>.  */
+
+static int
+inner (int x)
+{
+  return x + 2;
+}
+
+static int
+middle (int x)
+{
+  return 2 * inner (x);
+}
+
+static int
+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..5ea65da
--- /dev/null
+++ b/gdb/testsuite/gdb.base/set-reg.exp
@@ -0,0 +1,139 @@
+# 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
+
+# A value in register in previous frame.  E.g,. call-clobbered register.
+set reg_prev_frame_in_reg "unknown"
+# 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.
+}
+
+# Name of register is not set, skip the rest of test.
+if { [string equal $reg_prev_frame_in_reg "unknown"] } {
+    return
+}
+
+if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable {debug}] != "" } {
+    untested "Failed to compile"
+    return -1
+}
+
+proc set_register { register check_value } {with_test_prefix "${register} ${check_value}" {
+    global decimal
+    global gdb_prompt
+
+    set reg_val ""
+    gdb_test_multiple "p/d \$${register}" "print register ${register}" {
+	-re ".*${decimal} = ($decimal).*$gdb_prompt $" {
+	    set reg_val $expect_out(1,string)
+	}
+    }
+    if { $reg_val == "" } {
+	fail "get the value of register ${register}"
+	return
+    }
+
+    # Assign the same value to register.
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (unchanged)"
+
+    # Assign the different value to register.
+    set reg_val [expr $reg_val + 4]
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (changed)"
+    gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	"register ${register} is changed"
+
+    # Restore register.
+    set reg_val [expr $reg_val - 4]
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (restored)"
+    gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	"register ${register} is restored"
+
+    gdb_test "up"
+
+    gdb_test_multiple "p/d \$${register}" "print register ${register}" {
+	-re ".*${decimal} = ($decimal).*$gdb_prompt $" {
+	    set reg_val $expect_out(1,string)
+	}
+    }
+    if { $reg_val == "" } {
+	fail "get the value of register ${register}"
+	return
+    }
+    # Assign the same value to register.
+    gdb_test "set \$${register}=${reg_val}" \
+	"Assigning to register in non-innermost frame.*" \
+	"set register ${register} on non-inner frame"
+
+    if { $check_value } {
+	# Assign the different value to register.
+	set reg_val [expr $reg_val + 4]
+	gdb_test "set \$${register}=${reg_val}" \
+	    "Assigning to register in non-innermost frame.*" \
+	    "set register ${register} on non-inner frame (changed)"
+	gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	    "register ${register} is changed"
+	# Restore register.
+	set reg_val [expr $reg_val - 4]
+	gdb_test "set \$${register}=${reg_val}" \
+	    "Assigning to register in non-innermost frame.*" \
+	    "set register ${register} on non-inner frame (restored)"
+	gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	    "register ${register} is restored"
+    }
+    gdb_test "down"
+}}
+
+# Test setting register on innermost frame and non-innermost frame.
+
+proc test_set_reg_on_frame {} {with_test_prefix "on frame" {
+    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
+    }
+
+    gdb_test "break inner" "Breakpoint.*at.* file .*, line.*"
+    gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*inner.*at.*" \
+	"continue to inner"
+
+    set_register ${reg_prev_frame_in_reg} 1
+    # Change the register saved on frame may clobber the frame, and get
+    # the contents of registers in mess.  Don't check register value.
+    set_register ${reg_prev_frame_in_memory} 0
+}}
+
+test_set_reg_on_frame
diff --git a/gdb/valops.c b/gdb/valops.c
index 17696ee..ae19601 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.  A call-saved register saved in memory will have
+     'VALUE_REGNUM >= 0' but 'VALUE_LVAL == lval_memory'.  */
+  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 (_("Assigning to register in non-innermost frame."));
+    }
+
   switch (VALUE_LVAL (toval))
     {
     case lval_internalvar:
-- 
1.7.7.6

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

[Eli Zaretskii]

> Date: Fri, 7 Sep 2012 18:00:37 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: Tom Tromey <tromey@redhat.com>, <gdb-patches@sourceware.org>
> 
> > I think this is NEWS-worthy.
> > 
> 
> I add a NEWS entry.

Thanks.  But its text says "when assigning" without explaining enough
for the reader to understand what "assigning" are we talking about.
So I suggest to mention the commands that are affected ('set', IIUC),
and perhaps give an example.

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

[Yao Qi]

On 09/07/2012 06:10 PM, Eli Zaretskii wrote:
>> Date: Fri, 7 Sep 2012 18:00:37 +0800
>> From: Yao Qi <yao@codesourcery.com>
>> CC: Tom Tromey <tromey@redhat.com>, <gdb-patches@sourceware.org>
>>
>>> I think this is NEWS-worthy.
>>>
>>
>> I add a NEWS entry.
>
> Thanks.  But its text says "when assigning" without explaining enough
> for the reader to understand what "assigning" are we talking about.
> So I suggest to mention the commands that are affected ('set', IIUC),
> and perhaps give an example.
>

How about them below?

* GDB will display a warning when assigning to registers, for example
   "set $eax=0x1", in non-innermost frames.

or

* GDB will display a warning when assigning to registers by command
   "set" in non-innermost frames.

-- 
Yao

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

[Eli Zaretskii]

> Date: Fri, 7 Sep 2012 18:19:28 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <dje@google.com>, <tromey@redhat.com>, <gdb-patches@sourceware.org>
> 
> On 09/07/2012 06:10 PM, Eli Zaretskii wrote:
> >> Date: Fri, 7 Sep 2012 18:00:37 +0800
> >> From: Yao Qi <yao@codesourcery.com>
> >> CC: Tom Tromey <tromey@redhat.com>, <gdb-patches@sourceware.org>
> >>
> >>> I think this is NEWS-worthy.
> >>>
> >>
> >> I add a NEWS entry.
> >
> > Thanks.  But its text says "when assigning" without explaining enough
> > for the reader to understand what "assigning" are we talking about.
> > So I suggest to mention the commands that are affected ('set', IIUC),
> > and perhaps give an example.
> >
> 
> How about them below?
> 
> * GDB will display a warning when assigning to registers, for example
>    "set $eax=0x1", in non-innermost frames.
> 
> or
> 
> * GDB will display a warning when assigning to registers by command
>    "set" in non-innermost frames.

If "set" is the only command, then the second variant is better.  But
isn't the same true of the MI equivalent?

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

[Yao Qi]

On 09/07/2012 07:27 PM, Eli Zaretskii wrote:
>> >How about them below?
>> >
>> >* GDB will display a warning when assigning to registers, for example
>> >    "set $eax=0x1", in non-innermost frames.
>> >
>> >or
>> >
>> >* GDB will display a warning when assigning to registers by command
>> >    "set" in non-innermost frames.
> If "set" is the only command, then the second variant is better.  But
> isn't the same true of the MI equivalent?

It is true to the MI command "-var-assign".  How about this?

* GDB will display a warning when assigning to registers by CLI command
   "set" or MI command "-var-assign" in non-innermost frames.

-- 
Yao

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

[Eli Zaretskii]

> Date: Fri, 7 Sep 2012 21:12:54 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <dje@google.com>, <tromey@redhat.com>, <gdb-patches@sourceware.org>
> 
> On 09/07/2012 07:27 PM, Eli Zaretskii wrote:
> >> >How about them below?
> >> >
> >> >* GDB will display a warning when assigning to registers, for example
> >> >    "set $eax=0x1", in non-innermost frames.
> >> >
> >> >or
> >> >
> >> >* GDB will display a warning when assigning to registers by command
> >> >    "set" in non-innermost frames.
> > If "set" is the only command, then the second variant is better.  But
> > isn't the same true of the MI equivalent?
> 
> It is true to the MI command "-var-assign".  How about this?
> 
> * GDB will display a warning when assigning to registers by CLI command
>    "set" or MI command "-var-assign" in non-innermost frames.

Fine, thanks.

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

[Jan Kratochvil]

On Thu, 23 Aug 2012 18:24:46 +0200, Tom Tromey wrote:
> >>>>> "Doug" == Doug Evans <dje@google.com> writes:
> Doug> I *could* accept a warning when changing a register in a non-innermost
> Doug> frame, fwiw.
> 
> That would be ok by me.

I do not find correct even the warning.

It is absolutely normal user operation:

volatile int v = 42;
void __attribute__ ((noinline, noclone))
f (void)
{
  v++;
}
int
main (void)
{
  int i = v;
  f ();
  return i;
}
gcc-4.7.1-5.fc18.x86_64
gcc -Wall -g -O2
readelf -wio
 <2><61>: Abbrev Number: 4 (DW_TAG_variable)
    <62>   DW_AT_name        : i
    <6a>   DW_AT_location    : 0x0      (location list)
    Offset   Begin    End      Expression
    00000000 0000000000400407 000000000040040f (DW_OP_reg3 (rbx))
    00000000 000000000040040f 0000000000400410 (DW_OP_reg0 (rax))
    00000000 <End of list>

(gdb) b f
Breakpoint 1 at 0x400500: file 70.c, line 5.
(gdb) run
Breakpoint 1, f () at 70.c:5
(gdb) up
#1  0x000000000040040c in main () at 70.c:11
11	  f ();
(gdb) set variable i=20
warning: Assigning to register in non-innermost frame.

Why?

I would find correct to print such warning for:
	(gdb) set $rbx=20
There it should happen only if user has explicitly specified the register
itself.

Also I would find more appropriate to call 'query' in such case but that is
a nitpick.


Thanks,
Jan

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

[Yao Qi]

On 09/08/2012 12:45 AM, Jan Kratochvil wrote:
> (gdb) set variable i=20
> warning: Assigning to register in non-innermost frame.
>
> Why?
>

That is a surprise to me.

> I would find correct to print such warning for:
> 	(gdb) set $rbx=20
> There it should happen only if user has explicitly specified the register
> itself.

That is my original motivation of this patch.  I'll go back to see how 
to differentiate register and variables in evaluation.

-- 
Yao

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

[Yao Qi]

On 09/08/2012 12:45 AM, Jan Kratochvil wrote:
> (gdb) set variable i=20
> warning: Assigning to register in non-innermost frame.
> 
> Why?
> 
> I would find correct to print such warning for:
> 	(gdb) set $rbx=20
> There it should happen only if user has explicitly specified the register
> itself.

I checked the difference of value of register and value of variable
(saved in register), TYPE_OBJFILE_OWNED (value_type (toval)) is the
only difference between them.  IIUC, type of variable is owned by an
objfile, while type of register is not (alloc_type vs. alloc_type_arch),
so looks we can use it to differentiate values in register and values
in variables.

This new patch includes TYPE_OBJFILE_OWNED into the condition, and
adjust the NEWS entry discussed with Eli before.

-- 
Yao

gdb/testsuite:

2012-09-10  Yao Qi  <yao@codesourcery.com>

	* gdb.base/set-reg.exp: New.
	* gdb.base/set-reg.c: New.

gdb:

2012-09-10  Yao Qi  <yao@codesourcery.com>
	    Doug Evans  <dje@google.com>

	* valops.c (value_assign): Emit warning when assigning to
	registers in non-innermost frames.
	* NEWS: Mention it.
---
 gdb/NEWS                           |    3 +
 gdb/testsuite/gdb.base/set-reg.c   |   40 ++++++++++
 gdb/testsuite/gdb.base/set-reg.exp |  139 ++++++++++++++++++++++++++++++++++++
 gdb/valops.c                       |   16 ++++
 4 files changed, 198 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/NEWS b/gdb/NEWS
index dba6937..f7476e4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -17,6 +17,9 @@
 * The 'cd' command now defaults to using '~' (the home directory) if not
   given an argument.
 
+* GDB will display a warning when assigning to registers by CLI command
+  "set" or MI command "-var-assign" in non-innermost frames.
+
 * New configure options
 
 --enable-libmcheck/--disable-libmcheck
diff --git a/gdb/testsuite/gdb.base/set-reg.c b/gdb/testsuite/gdb.base/set-reg.c
new file mode 100644
index 0000000..03938eb
--- /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/>.  */
+
+static int
+inner (int x)
+{
+  return x + 2;
+}
+
+static int
+middle (int x)
+{
+  return 2 * inner (x);
+}
+
+static int
+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..5ea65da
--- /dev/null
+++ b/gdb/testsuite/gdb.base/set-reg.exp
@@ -0,0 +1,139 @@
+# 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
+
+# A value in register in previous frame.  E.g,. call-clobbered register.
+set reg_prev_frame_in_reg "unknown"
+# 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.
+}
+
+# Name of register is not set, skip the rest of test.
+if { [string equal $reg_prev_frame_in_reg "unknown"] } {
+    return
+}
+
+if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable {debug}] != "" } {
+    untested "Failed to compile"
+    return -1
+}
+
+proc set_register { register check_value } {with_test_prefix "${register} ${check_value}" {
+    global decimal
+    global gdb_prompt
+
+    set reg_val ""
+    gdb_test_multiple "p/d \$${register}" "print register ${register}" {
+	-re ".*${decimal} = ($decimal).*$gdb_prompt $" {
+	    set reg_val $expect_out(1,string)
+	}
+    }
+    if { $reg_val == "" } {
+	fail "get the value of register ${register}"
+	return
+    }
+
+    # Assign the same value to register.
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (unchanged)"
+
+    # Assign the different value to register.
+    set reg_val [expr $reg_val + 4]
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (changed)"
+    gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	"register ${register} is changed"
+
+    # Restore register.
+    set reg_val [expr $reg_val - 4]
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (restored)"
+    gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	"register ${register} is restored"
+
+    gdb_test "up"
+
+    gdb_test_multiple "p/d \$${register}" "print register ${register}" {
+	-re ".*${decimal} = ($decimal).*$gdb_prompt $" {
+	    set reg_val $expect_out(1,string)
+	}
+    }
+    if { $reg_val == "" } {
+	fail "get the value of register ${register}"
+	return
+    }
+    # Assign the same value to register.
+    gdb_test "set \$${register}=${reg_val}" \
+	"Assigning to register in non-innermost frame.*" \
+	"set register ${register} on non-inner frame"
+
+    if { $check_value } {
+	# Assign the different value to register.
+	set reg_val [expr $reg_val + 4]
+	gdb_test "set \$${register}=${reg_val}" \
+	    "Assigning to register in non-innermost frame.*" \
+	    "set register ${register} on non-inner frame (changed)"
+	gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	    "register ${register} is changed"
+	# Restore register.
+	set reg_val [expr $reg_val - 4]
+	gdb_test "set \$${register}=${reg_val}" \
+	    "Assigning to register in non-innermost frame.*" \
+	    "set register ${register} on non-inner frame (restored)"
+	gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	    "register ${register} is restored"
+    }
+    gdb_test "down"
+}}
+
+# Test setting register on innermost frame and non-innermost frame.
+
+proc test_set_reg_on_frame {} {with_test_prefix "on frame" {
+    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
+    }
+
+    gdb_test "break inner" "Breakpoint.*at.* file .*, line.*"
+    gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*inner.*at.*" \
+	"continue to inner"
+
+    set_register ${reg_prev_frame_in_reg} 1
+    # Change the register saved on frame may clobber the frame, and get
+    # the contents of registers in mess.  Don't check register value.
+    set_register ${reg_prev_frame_in_memory} 0
+}}
+
+test_set_reg_on_frame
diff --git a/gdb/valops.c b/gdb/valops.c
index 17696ee..7ac23e3 100644
--- 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
+     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))
+    {
+      /* 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 (_("Assigning to register in non-innermost frame."));
+    }
+
   switch (VALUE_LVAL (toval))
     {
     case lval_internalvar:
-- 
1.7.7.6

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

[Jan Kratochvil]

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

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

[Jan Kratochvil]

On Mon, 10 Sep 2012 09:46:39 +0200, Jan Kratochvil wrote:
> 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."));

or to prevent double-warning in CLI mode:

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


Thanks,
Jan

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

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> I checked the difference of value of register and value of variable
Yao> (saved in register), TYPE_OBJFILE_OWNED (value_type (toval)) is the
Yao> only difference between them.  IIUC, type of variable is owned by an
Yao> objfile, while type of register is not (alloc_type vs. alloc_type_arch),
Yao> so looks we can use it to differentiate values in register and values
Yao> in variables.

I would rather not use this approach.
My reason is that there is no obvious connection between
TYPE_OBJFILE_OWNED and register-ness -- and it is the sort of invariant
that is very difficult to ensure will remain true over time.

If lval_register can't work, then another choice would be a new flag on
struct value.  This would be somewhat ugly but, I think, more robust.

As Jan points out, it does seem strange to warn about the set rather
than query.  A warning comes too late.

After re-reading the whole thread I wonder whether pressing forward with
this patch is best.  IIUC this didn't arise from any user complaint but
rather just thinking about the problem.  And, while I agree it could be
confusing in some situations, in practice it seems that perhaps it has
never actually bitten anybody.

Tom

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

[Jan Kratochvil]

On Tue, 11 Sep 2012 19:12:31 +0200, Tom Tromey wrote:
> I would rather not use this approach.
> My reason is that there is no obvious connection between
> TYPE_OBJFILE_OWNED and register-ness -- and it is the sort of invariant
> that is very difficult to ensure will remain true over time.
> 
> If lval_register can't work, then another choice would be a new flag on
> struct value.  This would be somewhat ugly but, I think, more robust.

I also did not like TYPE_OBJFILE_OWNED too much, I was thinking putting it
possibly to
	evaluate_subexp_standard <BINOP_ASSIGN>
checking there LHS (left hand side) expression, not the LHS value.

Sure one can easily create an expression escaping such check but it should
catch the normal problematic case; if there is any.


Thanks,
Jan

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

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I also did not like TYPE_OBJFILE_OWNED too much, I was thinking putting it
Jan> possibly to
Jan> 	evaluate_subexp_standard <BINOP_ASSIGN>
Jan> checking there LHS (left hand side) expression, not the LHS value.

Yeah, I almost mentioned that approach as well.
I agree it would probably be ok; but it also requires checking whether
any language overrides this opcode.

Tom

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

[Yao Qi]

On 09/12/2012 01:12 AM, Tom Tromey wrote:
> After re-reading the whole thread I wonder whether pressing forward with
> this patch is best.  IIUC this didn't arise from any user complaint but
> rather just thinking about the problem.  And, while I agree it could be
> confusing in some situations, in practice it seems that perhaps it has
> never actually bitten anybody.

I also realize that this patch becomes "unnatural" :) so I drop it.
I'll revisit it when we do have the need to fix this problem.  The test
case is still useful, as we don't have test for setting registers in
testsuite.  Is the test case OK?

-- 
Yao

gdb/testsuite

2012-09-12  Yao Qi  <yao@codesourcery.com>

	* gdb.base/set-reg.exp: New.
	* gdb.base/set-reg.c: New.
---
 gdb/testsuite/gdb.base/set-reg.c   |   40 +++++++++++
 gdb/testsuite/gdb.base/set-reg.exp |  136 ++++++++++++++++++++++++++++++++++++
 2 files changed, 176 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..86d6496
--- /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/>.  */
+
+static int
+inner (int x)
+{
+  return x + 2;
+}
+
+static int
+middle (int x)
+{
+  return 2 * inner (x);
+}
+
+static int
+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..3353a10
--- /dev/null
+++ b/gdb/testsuite/gdb.base/set-reg.exp
@@ -0,0 +1,136 @@
+# 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
+
+# A value in register in previous frame.  E.g,. call-clobbered register.
+set reg_prev_frame_in_reg "unknown"
+# 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.
+}
+
+# Name of register is not set, skip the rest of test.
+if { [string equal $reg_prev_frame_in_reg "unknown"] } {
+    return
+}
+
+if { [gdb_compile $srcdir/$subdir/$srcfile $binfile executable {debug}] != "" } {
+    untested "Failed to compile"
+    return -1
+}
+
+proc set_register { register check_value } {with_test_prefix "${register} ${check_value}" {
+    global decimal
+    global gdb_prompt
+
+    set reg_val ""
+    gdb_test_multiple "p/d \$${register}" "print register ${register}" {
+	-re ".*${decimal} = ($decimal).*$gdb_prompt $" {
+	    set reg_val $expect_out(1,string)
+	}
+    }
+    if { $reg_val == "" } {
+	fail "get the value of register ${register}"
+	return
+    }
+
+    # Assign the same value to register.
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (unchanged)"
+
+    # Assign the different value to register.
+    set reg_val [expr $reg_val + 4]
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (changed)"
+    gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	"register ${register} is changed"
+
+    # Restore register.
+    set reg_val [expr $reg_val - 4]
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on inner frame (restored)"
+    gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	"register ${register} is restored"
+
+    gdb_test "up"
+
+    gdb_test_multiple "p/d \$${register}" "print register ${register}" {
+	-re ".*${decimal} = ($decimal).*$gdb_prompt $" {
+	    set reg_val $expect_out(1,string)
+	}
+    }
+    if { $reg_val == "" } {
+	fail "get the value of register ${register}"
+	return
+    }
+    # Assign the same value to register.
+    gdb_test_no_output "set \$${register}=${reg_val}" \
+	"set register ${register} on non-inner frame"
+
+    if { $check_value } {
+	# Assign the different value to register.
+	set reg_val [expr $reg_val + 4]
+	gdb_test_no_output "set \$${register}=${reg_val}" \
+	    "set register ${register} on non-inner frame (changed)"
+	gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	    "register ${register} is changed"
+	# Restore register.
+	set reg_val [expr $reg_val - 4]
+	gdb_test_no_output "set \$${register}=${reg_val}" \
+	    "set register ${register} on non-inner frame (restored)"
+	gdb_test "p/d \$${register}" ".*${decimal} = $reg_val.*" \
+	    "register ${register} is restored"
+    }
+    gdb_test "down"
+}}
+
+# Test setting register on innermost frame and non-innermost frame.
+
+proc test_set_reg_on_frame {} {with_test_prefix "on frame" {
+    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
+    }
+
+    gdb_test "break inner" "Breakpoint.*at.* file .*, line.*"
+    gdb_test "continue" "Continuing.\r\n\r\nBreakpoint.*inner.*at.*" \
+	"continue to inner"
+
+    set_register ${reg_prev_frame_in_reg} 1
+    # Change the register saved on frame may clobber the frame, and get
+    # the contents of registers in mess.  Don't check register value.
+    set_register ${reg_prev_frame_in_memory} 0
+}}
+
+test_set_reg_on_frame
-- 
1.7.7.6

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

[Jan Kratochvil]

On Wed, 12 Sep 2012 02:50:17 +0200, Yao Qi wrote:
> I also realize that this patch becomes "unnatural" :) so I drop it.
> I'll revisit it when we do have the need to fix this problem.  The test
> case is still useful, as we don't have test for setting registers in
> testsuite.  Is the test case OK?

In such case could it be more arch-independent?  It could either use
	register int var;
and 'info addr var' to set that register, independently of arch.

Or it could 'info reg' and also set some of the register(s) printed.

Currently the testcase seems pretty complicated to me
for a single 'set $reg=num' test.


Thanks,
Jan