From patchwork Tue Feb 25 15:13:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 38307 Received: (qmail 73064 invoked by alias); 25 Feb 2020 15:13:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 73055 invoked by uid 89); 25 Feb 2020 15:13:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=sk:infcall, 99.9, UD:id, HX-Received:4b0f X-HELO: mail-wm1-f43.google.com Received: from mail-wm1-f43.google.com (HELO mail-wm1-f43.google.com) (209.85.128.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Feb 2020 15:13:45 +0000 Received: by mail-wm1-f43.google.com with SMTP id t23so3390534wmi.1 for ; Tue, 25 Feb 2020 07:13:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :in-reply-to:references; bh=61UBq/4N+Yw2jU9hQMbIYonCP1RhiSCr1N/Omw7Iec8=; b=GZLTPsHK2BANtkcHlkSsbxJQ4jis7bZJzM0e9KMhnttUkxmeg2WbI+QoUjP54fhiVM csUg1Jw4fxISm1baU8wCpfQUpjsbzDN+uHUI8+t5+0UJmLdcnlFy5ZgR43j1VFb4F4Uw oGYnke+MHFt+lY15WzA6pZDcSvrGJmJXifqdZNDReiJyhdoYuP6e5SVAyGx25vwiX3Q8 V7+HAheIHJFwj09GjTIgI5GiJjh2dATJi4nZ0qjjyh2y+Xkvm40F7C047pS0YBEKPY8U BkGH66xM8atWa/t2QV9JAaFcObHQSZzW1O5CCfZit/xoGZAyqG4CX+HWyIDJZzOuEN0b cqOA== Return-Path: Received: from localhost (host86-186-80-160.range86-186.btcentralplus.com. [86.186.80.160]) by smtp.gmail.com with ESMTPSA id a22sm4560860wmd.20.2020.02.25.07.13.40 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 25 Feb 2020 07:13:40 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Richard Bunt , Andrew Burgess Subject: [RFC 1/2] gdb: Unify two copies of restore_selected_frame Date: Tue, 25 Feb 2020 15:13:36 +0000 Message-Id: <354ba2265d63d1004d2d2ea7e2611d1dcac68700.1582643363.git.andrew.burgess@embecosm.com> In-Reply-To: References: In-Reply-To: References: X-IsSubscribed: yes GDB currently has two copies of restore_selected_frame, this commit unifies these two, and moves the implementation into frame.c. The only possible user visible change after this commit is if GDB is unable to restore the currently selected stack after performing an inferior call. Previously, in any situation, if GDB failed to find a frame with a matching frame id, then the following warning would be issued, and GDB would be left with the innermost stack frame selected: Unable to restore previously selected frame. After this commit, then GDB will give a slightly different warning: Couldn't restore frame #%d in current thread. Bottom (innermost) frame selected: .... Where '....' is a summary of the frame that was selected, and '%d' is the frame level GDB was looking for. Additionally, this warning is not given if the previously selected frame was at level 0, in that case GDB will just silently selected the innermost frame. If this difference is a problem then it should be easy enough to have restore_selected_frame take an 'always-warn' flag parameter, but I haven't done that in this commit. Beyond this there should be no other user visible changes with this commit. gdb/ChangeLog: * frame.c (restore_selected_frame): Moved from thread.c. * frame.h (restore_selected_frame): Declare. (struct frame_id_and_level): New struct. * gdbthread.h (scoped_restore_current_thread) : Delete. : Delete. : New member variable. * infrun.c (infcall_control_state) : Delete. : New member variable. (save_infcall_control_state): Update to use selected_frame_info. (restore_selected_frame): Delete. (restore_infcall_control_state): Update to use selected_frame_info. * thread.c (restore_selected_frame): Delete. (scoped_restore_current_thread::restore): Update for changes to member variables. (scoped_restore_current_thread::scoped_restore_current_thread): Likewise. --- gdb/ChangeLog | 19 +++++++++++++++++ gdb/frame.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++ gdb/frame.h | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++ gdb/gdbthread.h | 3 +-- gdb/infrun.c | 26 ++++++----------------- gdb/thread.c | 66 +++------------------------------------------------------ 6 files changed, 150 insertions(+), 85 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index d74d1d5c7c5..863df107009 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -2910,6 +2910,67 @@ frame_prepare_for_sniffer (struct frame_info *frame, frame->unwind = unwind; } +/* See frame.h. */ + +void +restore_selected_frame (struct frame_id a_frame_id, int frame_level) +{ + struct frame_info *frame; + int count; + + /* This means there was no selected frame. */ + if (frame_level == -1) + { + select_frame (NULL); + return; + } + + gdb_assert (frame_level >= 0); + + /* Restore by level first, check if the frame id is the same as + expected. If that fails, try restoring by frame id. If that + fails, nothing to do, just warn the user. */ + + count = frame_level; + frame = find_relative_frame (get_current_frame (), &count); + if (count == 0 + && frame != NULL + /* The frame ids must match - either both valid or both outer_frame_id. + The latter case is not failsafe, but since it's highly unlikely + the search by level finds the wrong frame, it's 99.9(9)% of + the time (for all practical purposes) safe. */ + && frame_id_eq (get_frame_id (frame), a_frame_id)) + { + /* Cool, all is fine. */ + select_frame (frame); + return; + } + + frame = frame_find_by_id (a_frame_id); + if (frame != NULL) + { + /* Cool, refound it. */ + select_frame (frame); + return; + } + + /* Nothing else to do, the frame layout really changed. Select the + innermost stack frame. */ + select_frame (get_current_frame ()); + + /* Warn the user. */ + if (frame_level > 0 && !current_uiout->is_mi_like_p ()) + { + warning (_("Couldn't restore frame #%d in " + "current thread. Bottom (innermost) frame selected:"), + frame_level); + /* For MI, we should probably have a notification about + current frame change. But this error is not very + likely, so don't bother for now. */ + print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1); + } +} + static struct cmd_list_element *set_backtrace_cmdlist; static struct cmd_list_element *show_backtrace_cmdlist; diff --git a/gdb/frame.h b/gdb/frame.h index cfc15022ed5..d8e000eda8b 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -957,5 +957,65 @@ extern void set_frame_previous_pc_masked (struct frame_info *frame); extern bool get_frame_pc_masked (const struct frame_info *frame); +/* Try to find a previously selected frame described by A_FRAME_ID and + select it. If no matching frame can be found then the innermost frame + will be selected and a warning printed. FRAME_LEVEL is the level at + which A_FRAME_ID was previously found, and can be -1 to indicate no + frame was previously selected, in which case the innermost frame will + be selected (without a warning). */ + +extern void restore_selected_frame (struct frame_id a_frame_id, int frame_level); + +/* When GDB needs to backup and then later restore the currently selected + frame this is done by storing the frame id, and then looking up a frame + with that stored frame id. + + However, if the previously selected frame can't be restored then GDB + should give the user a warning in most cases. If the previously + selected frame was level 0 then GDB will just reselect the innermost + frame silently without a warning. + + And so, when we backup and restore the currently selected frame we need + to track both the frame id, and the frame level, so GDB knows if a + warning should be given or not. */ + +struct frame_id_and_level +{ + /* Setup this structure to track FI as the previously selected frame. */ + void reset (struct frame_info *fi) + { + m_id = get_frame_id (fi); + m_level = frame_relative_level (fi); + } + + /* Reset this structure to indicate there was no previously selected + frame. */ + void reset () + { + m_level = -1; + } + + /* The frame id of the previously selected frame. This value is only + defined when LEVEL() is greater than -1. */ + struct frame_id id () const + { + return m_id; + } + + /* The level of the previously selected frame, or -1 if no frame was + previously selected. */ + int level () const + { + return m_level; + } + +private: + /* The frame id. */ + struct frame_id m_id; + + /* The level at which ID was found. Set to -1 to indicate that this + structure is uninitialised. */ + int m_level = -1; +}; #endif /* !defined (FRAME_H) */ diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 717a2ad08c2..26e572d1e98 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -681,8 +681,7 @@ private: function in the Darwin API. */ class thread_info *m_thread; inferior *m_inf; - frame_id m_selected_frame_id; - int m_selected_frame_level; + struct frame_id_and_level m_selected_frame_info; bool m_was_stopped; }; diff --git a/gdb/infrun.c b/gdb/infrun.c index d9a6f733519..3cb46041c9a 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -9104,8 +9104,9 @@ struct infcall_control_state enum stop_stack_kind stop_stack_dummy = STOP_NONE; int stopped_by_random_signal = 0; - /* ID if the selected frame when the inferior function call was made. */ - struct frame_id selected_frame_id {}; + /* ID and level of the selected frame when the inferior function call was + made. */ + struct frame_id_and_level selected_frame_info; }; /* Save all of the information associated with the inferior<==>gdb @@ -9134,27 +9135,11 @@ save_infcall_control_state () inf_status->stop_stack_dummy = stop_stack_dummy; inf_status->stopped_by_random_signal = stopped_by_random_signal; - inf_status->selected_frame_id = get_frame_id (get_selected_frame (NULL)); + inf_status->selected_frame_info.reset (get_selected_frame (NULL)); return inf_status; } -static void -restore_selected_frame (const frame_id &fid) -{ - frame_info *frame = frame_find_by_id (fid); - - /* If inf_status->selected_frame_id is NULL, there was no previously - selected frame. */ - if (frame == NULL) - { - warning (_("Unable to restore previously selected frame.")); - return; - } - - select_frame (frame); -} - /* Restore inferior session state to INF_STATUS. */ void @@ -9187,7 +9172,8 @@ restore_infcall_control_state (struct infcall_control_state *inf_status) error() trying to dereference it. */ try { - restore_selected_frame (inf_status->selected_frame_id); + restore_selected_frame (inf_status->selected_frame_info.id (), + inf_status->selected_frame_info.level ()); } catch (const gdb_exception_error &ex) { diff --git a/gdb/thread.c b/gdb/thread.c index 54b59e22448..d0c795c5864 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1356,65 +1356,6 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) switch_to_thread (thr); } -static void -restore_selected_frame (struct frame_id a_frame_id, int frame_level) -{ - struct frame_info *frame = NULL; - int count; - - /* This means there was no selected frame. */ - if (frame_level == -1) - { - select_frame (NULL); - return; - } - - gdb_assert (frame_level >= 0); - - /* Restore by level first, check if the frame id is the same as - expected. If that fails, try restoring by frame id. If that - fails, nothing to do, just warn the user. */ - - count = frame_level; - frame = find_relative_frame (get_current_frame (), &count); - if (count == 0 - && frame != NULL - /* The frame ids must match - either both valid or both outer_frame_id. - The latter case is not failsafe, but since it's highly unlikely - the search by level finds the wrong frame, it's 99.9(9)% of - the time (for all practical purposes) safe. */ - && frame_id_eq (get_frame_id (frame), a_frame_id)) - { - /* Cool, all is fine. */ - select_frame (frame); - return; - } - - frame = frame_find_by_id (a_frame_id); - if (frame != NULL) - { - /* Cool, refound it. */ - select_frame (frame); - return; - } - - /* Nothing else to do, the frame layout really changed. Select the - innermost stack frame. */ - select_frame (get_current_frame ()); - - /* Warn the user. */ - if (frame_level > 0 && !current_uiout->is_mi_like_p ()) - { - warning (_("Couldn't restore frame #%d in " - "current thread. Bottom (innermost) frame selected:"), - frame_level); - /* For MI, we should probably have a notification about - current frame change. But this error is not very - likely, so don't bother for now. */ - print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1); - } -} - void scoped_restore_current_thread::restore () { @@ -1439,7 +1380,8 @@ scoped_restore_current_thread::restore () && target_has_registers && target_has_stack && target_has_memory) - restore_selected_frame (m_selected_frame_id, m_selected_frame_level); + restore_selected_frame (m_selected_frame_info.id (), + m_selected_frame_info.level ()); } scoped_restore_current_thread::~scoped_restore_current_thread () @@ -1488,9 +1430,7 @@ scoped_restore_current_thread::scoped_restore_current_thread () else frame = NULL; - m_selected_frame_id = get_frame_id (frame); - m_selected_frame_level = frame_relative_level (frame); - + m_selected_frame_info.reset (frame); tp->incref (); m_thread = tp; }