From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20656 invoked by alias); 7 Sep 2012 10:01:49 -0000 Received: (qmail 20637 invoked by uid 22791); 7 Sep 2012 10:01:47 -0000 X-SWARE-Spam-Status: No, hits=-4.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_BM X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 Sep 2012 10:01:32 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1T9vNR-0002CM-CE from Yao_Qi@mentor.com ; Fri, 07 Sep 2012 03:01:29 -0700 Received: from SVR-ORW-FEM-05.mgc.mentorg.com ([147.34.97.43]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Fri, 7 Sep 2012 03:01:29 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.1.289.1; Fri, 7 Sep 2012 03:01:27 -0700 Message-ID: <5049C5C5.6000004@codesourcery.com> Date: Fri, 07 Sep 2012 10:01:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120717 Thunderbird/14.0 MIME-Version: 1.0 To: CC: Tom Tromey , Subject: Re: [RFC] Don't allow setting register in non-innermost frame References: <1345170040-25959-1-git-send-email-yao@codesourcery.com> <87hartpodt.fsf@fleche.redhat.com> <503DE604.5070904@codesourcery.com> <20550.33413.240311.774616@ruffy2.mtv.corp.google.com> In-Reply-To: <20550.33413.240311.774616@ruffy2.mtv.corp.google.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-09/txt/msg00060.txt.bz2 On 09/05/2012 06:36 AM, dje@google.com wrote: > > > > gdb/testsuite: > > > > 2012-08-29 Yao Qi > > > > * gdb.base/set-reg.exp: New. > > * gdb.base/set-reg.c: New. > > > > gdb: > > > > 2012-08-29 Yao Qi > > > > * 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 * gdb.base/set-reg.exp: New. * gdb.base/set-reg.c: New. gdb: 2012-09-07 Yao Qi Doug Evans * 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 . */ + +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 . + +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