From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38493 invoked by alias); 22 May 2018 21:33:43 -0000 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 Received: (qmail 38483 invoked by uid 89); 22 May 2018 21:33:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:sk:y15-v6s, 20197, HX-Received:sk:f10-v6m, sk:simark X-HELO: mail-wr0-f175.google.com Received: from mail-wr0-f175.google.com (HELO mail-wr0-f175.google.com) (209.85.128.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 22 May 2018 21:33:37 +0000 Received: by mail-wr0-f175.google.com with SMTP id y15-v6so22709906wrg.11 for ; Tue, 22 May 2018 14:33:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=mKAAwdcABZNlEjLzZGX6VO93zk4z+lmDqPNfhn6zZ98=; b=bS8HhYiaj7c9UkKPTG4pDOB88RAlOmiLYjThvOrO9NKhwiVfUA4KXglesmBzxEDtJi kACHsHV/YAqd5CTKGX2DW1r85saDv85IEMywudZN2sczi9NZGFzG16wPaevYLLQ4OUnV kCT7tVuh5qLciG+Lpuru1HrmJpD0ULUUlb+1E/x+V9ezbW17faisY2XV3Yx7k70OLr+f AyInTddiCCfyAoUYYjrdGuAMMLpjRfAHUnyY5B1qwJeWT/CseVBXHHbj/bvtuWXzfXyH UFG10zg9x8EmopS9ef+9c69m80SLS0vVkb85nLLPD5T7/Vt9FdJikAoJE2SGGO7/x2Nq iuNg== X-Gm-Message-State: ALKqPwe3CWVuArbm+U9yLmsnroS3DQFqtradCyGAtjyFjpoC2sEnC/N+ 0NQOSMDnIBlMpDs3/pV3gfsfjE+3 X-Google-Smtp-Source: AB8JxZrM69We1dKmu6F9eqkO2qMqC4TIBJn5qQfV64ZGVTrOd/XprBws90SPBiwrBY+sGt/bfKh3fA== X-Received: by 2002:adf:9f4a:: with SMTP id f10-v6mr106246wrg.216.1527024814556; Tue, 22 May 2018 14:33:34 -0700 (PDT) Received: from localhost ([176.123.41.197]) by smtp.gmail.com with ESMTPSA id b16-v6sm17912445wrm.89.2018.05.22.14.33.32 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 22 May 2018 14:33:33 -0700 (PDT) Date: Tue, 22 May 2018 21:53:00 -0000 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Restore selected frame in print_frame_local_vars Message-ID: <20180522213331.GA3304@embecosm.com> References: <20180519220831.14412-1-andrew.burgess@embecosm.com> <7b8e71b3-8913-07dc-9a0e-37e732c630c5@simark.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7b8e71b3-8913-07dc-9a0e-37e732c630c5@simark.ca> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-05/txt/msg00567.txt.bz2 Simon, Thanks for your feedback. I've addressed most of your feedback in this revision, there's a couple of points I've replied too inline. * Simon Marchi [2018-05-20 23:00:13 -0400]: > On 2018-05-19 06:08 PM, Andrew Burgess wrote: > > PR gdb/23203 reports 'bt full' causing the currently selected frame to > > change, this issue is fixed in this commit. > > > > Add a new class scoped_restore_selected_frame that saves and restores > > the selected frame. Make use of this in print_frame_local_vars to > > restore the selected frame on exit. > > Hi Andrew, > > Thanks for the patch. I noted some comments and questions below, but > overall this looks great, thanks for taking the time to do this. > > > gdb/ChangeLog: > > > > PR gdb/23203 > > * frame.c > > (scoped_restore_selected_frame::scoped_restore_selected_frame): > > Define. > > (scoped_restore_selected_frame::~scoped_restore_selected_frame): > > Define. > > * frame.h (class scoped_restore_selected_frame): New class. > > * stack.c (print_frame_local_vars): Remove catching and rethrowing > > of any exception, use scoped_restore_selected_frame to restore the > > frame instead. > > > > gdb/testsuite/ChangeLog: > > > > PR gdb/23203 > > * gdb.base/bt-full.c: New file. > > * gdb.base/bt-full.exp: New file. > > --- > > gdb/ChangeLog | 13 +++++++++ > > gdb/frame.c | 16 ++++++++++ > > gdb/frame.h | 16 ++++++++++ > > gdb/stack.c | 24 +++------------ > > gdb/testsuite/ChangeLog | 6 ++++ > > gdb/testsuite/gdb.base/bt-full.c | 35 ++++++++++++++++++++++ > > gdb/testsuite/gdb.base/bt-full.exp | 60 ++++++++++++++++++++++++++++++++++++++ > > 7 files changed, 150 insertions(+), 20 deletions(-) > > create mode 100644 gdb/testsuite/gdb.base/bt-full.c > > create mode 100644 gdb/testsuite/gdb.base/bt-full.exp > > > > diff --git a/gdb/frame.c b/gdb/frame.c > > index 07fa2bc77d6..3e42aa82398 100644 > > --- a/gdb/frame.c > > +++ b/gdb/frame.c > > @@ -269,6 +269,22 @@ frame_stash_invalidate (void) > > htab_empty (frame_stash); > > } > > > > +/* Save the currently selected frame. */ > > Please put the doc in the header file, and put /* See frame.h. */ here. > > > +scoped_restore_selected_frame::scoped_restore_selected_frame () > > +{ > > + m_fid = get_frame_id (get_selected_frame (NULL)); > > +} > > + > > +/* Restore the currently selected frame. */ > > Here too. > > > +scoped_restore_selected_frame::~scoped_restore_selected_frame () > > +{ > > + frame_info *frame = frame_find_by_id (m_fid); > > + if (frame == NULL) > > + warning (_("Unable to restore previously selected frame.")); > > + else > > + select_frame (frame); > > +} > > + > > /* Flag to control debugging. */ > > > > unsigned int frame_debug; > > diff --git a/gdb/frame.h b/gdb/frame.h > > index d5800b78c25..2c8f815a4ae 100644 > > --- a/gdb/frame.h > > +++ b/gdb/frame.h > > @@ -164,6 +164,22 @@ struct frame_id > > int artificial_depth; > > }; > > > > +/* Save and restore the currently selected frame. */ > > + > > +class scoped_restore_selected_frame > > +{ > > +public: > > + scoped_restore_selected_frame (); > > + ~scoped_restore_selected_frame (); > > + > > + DISABLE_COPY_AND_ASSIGN (scoped_restore_selected_frame); > > + > > +private: > > + > > + /* The ID of the previously selected frame. */ > > + struct frame_id m_fid; > > +}; > > + > > /* Methods for constructing and comparing Frame IDs. */ > > > > /* For convenience. All fields are zero. This means "there is no frame". */ > > diff --git a/gdb/stack.c b/gdb/stack.c > > index ecf1ee83793..7e377764865 100644 > > --- a/gdb/stack.c > > +++ b/gdb/stack.c > > @@ -2019,7 +2019,6 @@ print_frame_local_vars (struct frame_info *frame, int num_tabs, > > struct print_variable_and_value_data cb_data; > > const struct block *block; > > CORE_ADDR pc; > > - struct gdb_exception except = exception_none; > > > > if (!get_frame_pc_if_available (frame, &pc)) > > { > > @@ -2043,27 +2042,12 @@ print_frame_local_vars (struct frame_info *frame, int num_tabs, > > /* Temporarily change the selected frame to the given FRAME. > > This allows routines that rely on the selected frame instead > > of being given a frame as parameter to use the correct frame. */ > > + scoped_restore_selected_frame restore_selected_frame; > > Should we instead put this scoped_restore in the caller, so we only save/restore the > frame once during the execution of "bt full"? I'm inclined not to, so that print_frame_local_vars doesn't end up changing the frame. However, I'm not that fussed, so if you'd really prefer it done differently then I'm happy to make that change. > > > select_frame (frame); > > > > - TRY > > - { > > - iterate_over_block_local_vars (block, > > - do_print_variable_and_value, > > - &cb_data); > > - } > > - CATCH (ex, RETURN_MASK_ALL) > > - { > > - except = ex; > > - } > > - END_CATCH > > - > > - /* Restore the selected frame, and then rethrow if there was a problem. */ > > - select_frame (frame_find_by_id (cb_data.frame_id)); > > - if (except.reason < 0) > > - throw_exception (except); > > - > > - /* do_print_variable_and_value invalidates FRAME. */ > > - frame = NULL; > > + iterate_over_block_local_vars (block, > > + do_print_variable_and_value, > > + &cb_data); > > > > if (!cb_data.values_printed) > > fprintf_filtered (stream, _("No locals.\n")); > > diff --git a/gdb/testsuite/gdb.base/bt-full.c b/gdb/testsuite/gdb.base/bt-full.c > > new file mode 100644 > > index 00000000000..708dc69b250 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/bt-full.c > > @@ -0,0 +1,35 @@ > > +/* This testcase is part of GDB, the GNU debugger. > > + > > + Copyright 2018 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 . */ > > + > > +void > > +breakpt () > > +{ > > + asm ("" ::: "memory"); > > +} > > + > > +void > > +func () > > +{ > > + breakpt (); > > +} > > + > > +int > > +main () > > +{ > > + func (); > > + return 0; > > +} > > diff --git a/gdb/testsuite/gdb.base/bt-full.exp b/gdb/testsuite/gdb.base/bt-full.exp > > new file mode 100644 > > index 00000000000..7b64293dcfb > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/bt-full.exp > > @@ -0,0 +1,60 @@ > > +# Copyright 2018 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 > > + > > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } { > > + return -1 > > +} > > + > > +if ![runto_main] then { > > + fail "can't run to main" > > + return 0 > > +} > > + > > +gdb_breakpoint "breakpt" > > +gdb_continue_to_breakpoint "breakpt" > > + > > +# Return the frame number for the currently selected frame > > +proc get_current_frame_number {{test_name ""}} { > > + global gdb_prompt > > + > > + if { $test_name == "" } { > > + set test_name "get current frame number" > > + } > > + set frame_num -1 > > + gdb_test_multiple "frame" $test_name { > > + -re "#(\[0-9\]+) .*$gdb_prompt $" { > > + set frame_num $expect_out(1,string) > > + } > > + } > > + return $frame_num > > +} > > This could go in lib/gdb.exp I suppose. > > > + > > +# Visit each frame in the stack and ensure that 'bt full' doesn't > > +# change the selected stack frame. > > +for {set i 0} {$i < 3} {incr i} { > > I have a personal preference for this style: > > foreach_with_prefix frame {1 2 3} { > ... > } > > I think it expresses the intent better than a simple for loop, and it automatically > scopes the test messages (with frame=X), so you don't need to put $i in every test > message. > > > + if { $i == 0 } { > > + gdb_assert {[get_current_frame_number] == $i} "check frame $i is selected" > > + } else { > > + gdb_test "frame $i" "#$i .*" "select frame $i" > > + } > > Is there a particular reason for this if? Could we not do both "frame $i" and check > the current frame at every iteration? I'd prefer to keep things as they are. The code as it currently is checks that we are in frame 0 when we stop at the breakpoint. Sure this is checked many times elsewhere, but it doesn't hurt to check it again. We could move the gdb_assert outside the if/then/else, but we already check that the frame number changes after the 'frame ' in the regexp, so I don't think moving the gdb_assert adds anything. Again, I'm not that fussed, so if you feel strongly, let me know and I'll change it as you suggested. > > > + > > + gdb_test "bt full" "#0 \[^\n\r\]+\[\n\r\]+No locals\.\[\n\r\]+#1 \[^\n\r\]+\[\n\r\]+No locals\.\[\n\r\]+#2 \[^\n\r\]+\[\n\r\]+No locals\.\[\n\r\]+" \ > > + "perform 'bt full' in frame $i" > > Did you try using the multi_line proc? It might help make this more readable. > > > + > > + gdb_assert {[get_current_frame_number] == $i} "check frame $i is still selected" > > +} > > + > > > > Is there already a test that does the equivalent of this, but for "bt" not-full? It might > not be really necessary, but I thought it would be a low-hanging fruit to test with both > "bt" and "bt-fulL" at this point. Obviously, the test wouldn't be > named bt-full.exp then :). I renamed the test, and extended it to cover 'bt' as well as 'bt full'. Thanks, Andrew --- gdb: Restore selected frame in print_frame_local_vars PR gdb/23203 reports 'bt full' causing the currently selected frame to change, this issue is fixed in this commit. Add a new class scoped_restore_selected_frame that saves and restores the selected frame. Make use of this in print_frame_local_vars to restore the selected frame on exit. gdb/ChangeLog: PR gdb/23203 * frame.c (scoped_restore_selected_frame::scoped_restore_selected_frame): Define. (scoped_restore_selected_frame::~scoped_restore_selected_frame): Define. * frame.h (class scoped_restore_selected_frame): New class. * stack.c (print_frame_local_vars): Remove catching and rethrowing of any exception, use scoped_restore_selected_frame to restore the frame instead. gdb/testsuite/ChangeLog: PR gdb/23203 * gdb.base/bt-selected-frame.c: New file. * gdb.base/bt-selected-frame.exp: New file. * lib/gdb.exp (get_current_frame_number): New function. --- gdb/ChangeLog | 13 ++++++ gdb/frame.c | 16 +++++++ gdb/frame.h | 19 ++++++++ gdb/stack.c | 24 ++-------- gdb/testsuite/ChangeLog | 7 +++ gdb/testsuite/gdb.base/bt-selected-frame.c | 35 ++++++++++++++ gdb/testsuite/gdb.base/bt-selected-frame.exp | 70 ++++++++++++++++++++++++++++ gdb/testsuite/lib/gdb.exp | 16 +++++++ 8 files changed, 180 insertions(+), 20 deletions(-) create mode 100644 gdb/testsuite/gdb.base/bt-selected-frame.c create mode 100644 gdb/testsuite/gdb.base/bt-selected-frame.exp diff --git a/gdb/frame.c b/gdb/frame.c index 07fa2bc77d6..1d0eb725145 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -269,6 +269,22 @@ frame_stash_invalidate (void) htab_empty (frame_stash); } +/* See frame.h */ +scoped_restore_selected_frame::scoped_restore_selected_frame () +{ + m_fid = get_frame_id (get_selected_frame (NULL)); +} + +/* See frame.h */ +scoped_restore_selected_frame::~scoped_restore_selected_frame () +{ + frame_info *frame = frame_find_by_id (m_fid); + if (frame == NULL) + warning (_("Unable to restore previously selected frame.")); + else + select_frame (frame); +} + /* Flag to control debugging. */ unsigned int frame_debug; diff --git a/gdb/frame.h b/gdb/frame.h index d5800b78c25..a6f7fd8947d 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -164,6 +164,25 @@ struct frame_id int artificial_depth; }; +/* Save and restore the currently selected frame. */ + +class scoped_restore_selected_frame +{ +public: + /* Save the currently selected frame. */ + scoped_restore_selected_frame (); + + /* Restore the currently selected frame. */ + ~scoped_restore_selected_frame (); + + DISABLE_COPY_AND_ASSIGN (scoped_restore_selected_frame); + +private: + + /* The ID of the previously selected frame. */ + struct frame_id m_fid; +}; + /* Methods for constructing and comparing Frame IDs. */ /* For convenience. All fields are zero. This means "there is no frame". */ diff --git a/gdb/stack.c b/gdb/stack.c index ecf1ee83793..7e377764865 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -2019,7 +2019,6 @@ print_frame_local_vars (struct frame_info *frame, int num_tabs, struct print_variable_and_value_data cb_data; const struct block *block; CORE_ADDR pc; - struct gdb_exception except = exception_none; if (!get_frame_pc_if_available (frame, &pc)) { @@ -2043,27 +2042,12 @@ print_frame_local_vars (struct frame_info *frame, int num_tabs, /* Temporarily change the selected frame to the given FRAME. This allows routines that rely on the selected frame instead of being given a frame as parameter to use the correct frame. */ + scoped_restore_selected_frame restore_selected_frame; select_frame (frame); - TRY - { - iterate_over_block_local_vars (block, - do_print_variable_and_value, - &cb_data); - } - CATCH (ex, RETURN_MASK_ALL) - { - except = ex; - } - END_CATCH - - /* Restore the selected frame, and then rethrow if there was a problem. */ - select_frame (frame_find_by_id (cb_data.frame_id)); - if (except.reason < 0) - throw_exception (except); - - /* do_print_variable_and_value invalidates FRAME. */ - frame = NULL; + iterate_over_block_local_vars (block, + do_print_variable_and_value, + &cb_data); if (!cb_data.values_printed) fprintf_filtered (stream, _("No locals.\n")); diff --git a/gdb/testsuite/gdb.base/bt-selected-frame.c b/gdb/testsuite/gdb.base/bt-selected-frame.c new file mode 100644 index 00000000000..708dc69b250 --- /dev/null +++ b/gdb/testsuite/gdb.base/bt-selected-frame.c @@ -0,0 +1,35 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2018 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 . */ + +void +breakpt () +{ + asm ("" ::: "memory"); +} + +void +func () +{ + breakpt (); +} + +int +main () +{ + func (); + return 0; +} diff --git a/gdb/testsuite/gdb.base/bt-selected-frame.exp b/gdb/testsuite/gdb.base/bt-selected-frame.exp new file mode 100644 index 00000000000..931c91018c9 --- /dev/null +++ b/gdb/testsuite/gdb.base/bt-selected-frame.exp @@ -0,0 +1,70 @@ +# Copyright 2018 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 + +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } { + return -1 +} + +proc check_selected_frame_after_bt { bt_cmd stack_pattern } { + global binfile + + clean_restart $binfile + + with_test_prefix $bt_cmd { + + if ![runto_main] then { + fail "can't run to main" + return 0 + } + + gdb_breakpoint "breakpt" + gdb_continue_to_breakpoint "breakpt" + + # Visit each frame in the stack and ensure that the selected + # frame doesn't change after executing the backtrace command + # in BT_CMD. + foreach_with_prefix frame {0 1 2} { + if { $frame == 0 } { + gdb_assert {[get_current_frame_number] == $frame} \ + "check frame" + } else { + gdb_test "frame $frame" "#$frame .*" "select frame" + } + + gdb_test "$bt_cmd" $stack_pattern \ + "perform backtrace command" + + gdb_assert {[get_current_frame_number] == $frame} \ + "check frame is still selected" + } + } +} + +check_selected_frame_after_bt "bt" \ + [multi_line \ + "#0 \[^\n\r\]+" \ + "#1 \[^\n\r\]+" \ + "#2 \[^\n\r\]+"] + +check_selected_frame_after_bt "bt full" \ + [multi_line \ + "#0 \[^\n\r\]+" \ + "No locals\." \ + "#1 \[^\n\r\]+" \ + "No locals\." \ + "#2 \[^\n\r\]+" \ + "No locals\."] diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 0f05d043f25..ee66a38e08f 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -5805,6 +5805,22 @@ proc get_var_address { var } { return "" } +# Return the frame number for the currently selected frame +proc get_current_frame_number {{test_name ""}} { + global gdb_prompt + + if { $test_name == "" } { + set test_name "get current frame number" + } + set frame_num -1 + gdb_test_multiple "frame" $test_name { + -re "#(\[0-9\]+) .*$gdb_prompt $" { + set frame_num $expect_out(1,string) + } + } + return $frame_num +} + # Get the current value for remotetimeout and return it. proc get_remotetimeout { } { global gdb_prompt -- 2.14.3