From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by sourceware.org (Postfix) with ESMTPS id 714EF38618F9 for ; Wed, 8 Jul 2020 23:31:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 714EF38618F9 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f67.google.com with SMTP id j4so347818wrp.10 for ; Wed, 08 Jul 2020 16:31: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:from:to:subject:date:message-id:in-reply-to :references; bh=yiC6wydqeOeHXrwjHJgYXVfmRuy/o15H1UpylIgq64Q=; b=mxxojCVggb7DsN7UTdLnMLPN0fW4iYjC0N0aKSMIteqZ12YZIK2Afzlcl+sNCHw+ra YVoSmxPPihaSglB6fA28j8coMCJnsfuJvOrqStUIZyyrpc2TWQpHXlEm84M58knfj8RY G53hAdM/jvuK1hS7S09/bLq6NYcyCcHGppVoEcl5I3yzHpYKzasRxzbrvcrm31oJenXZ odaqndD0agCSHYxXCeuVJjJubTPSzGQ6krXN3PqVOa8X2dvhhruo9+8D2qTrjU/NkGJx c35YhBG9YkAwGp/7cTlweOlV1fm59T5VLI94IeqZYKdkhDM4TNxHYtaBLq7D3z7XL3+Z q67w== X-Gm-Message-State: AOAM530kJkvDeo+g7eRE9O/3YW2jkHlTLOvh25FJin0/dK8ZjTh8dM4W Uj3UgTVWBqxyyWqeHUkD0xuUItryJEE= X-Google-Smtp-Source: ABdhPJzWS6+rJMPA+DShXdI/Vfl5jKTCXc3CfTcwJV3ZumBi0GeM5hv1NYWn7+5BNF7q+mAzhggpdw== X-Received: by 2002:adf:e50a:: with SMTP id j10mr64622520wrm.71.1594251094728; Wed, 08 Jul 2020 16:31:34 -0700 (PDT) Received: from localhost ([83.223.242.101]) by smtp.gmail.com with ESMTPSA id q4sm2081228wmc.1.2020.07.08.16.31.33 for (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Wed, 08 Jul 2020 16:31:33 -0700 (PDT) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) Date: Thu, 9 Jul 2020 00:31:25 +0100 Message-Id: <20200708233125.1030-4-pedro@palves.net> X-Mailer: git-send-email 2.14.5 In-Reply-To: <20200708233125.1030-1-pedro@palves.net> References: <20200708233125.1030-1-pedro@palves.net> X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_ABUSEAT, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jul 2020 23:31:38 -0000 If the remote target closes while we're reading registers/memory for restoring the selected frame in scoped_restore_current_thread's dtor, the corresponding TARGET_CLOSE_ERROR error is swallowed by the scoped_restore_current_thread's dtor, because letting exceptions escape from a dtor is bad. It isn't great to lose that errors like that, though. I've been thinking about how to avoid it, and I came up with this patch. The idea here is to make scoped_restore_current_thread's dtor do as little as possible, to avoid any work that might throw in the first place. And to do that, instead of having the dtor call restore_selected_frame, which re-finds the previously selected frame, just record the frame_id/level of the desired selected frame, and have get_selected_frame find the frame the next time it is called. In effect, this implements most of Cagney's suggestion, here: /* On demand, create the selected frame and then return it. If the selected frame can not be created, this function prints then throws an error. When MESSAGE is non-NULL, use it for the error message, otherwize use a generic error message. */ /* FIXME: cagney/2002-11-28: At present, when there is no selected frame, this function always returns the current (inner most) frame. It should instead, when a thread has previously had its frame selected (but not resumed) and the frame cache invalidated, find and then return that thread's previously selected frame. */ extern struct frame_info *get_selected_frame (const char *message); The only thing missing to fully implement that would be to make reinit_frame_cache just clear selected_frame instead of calling select_frame(NULL), and the call select_frame(NULL) explicitly in the places where we really wanted reinit_frame_cache to go back to the current frame too. That can done separately, though, I'm not proposing to do that in this patch. restore_selected_frame should really move from thread.c to frame.c, but I didn't do that here, just to avoid churn in the patch while it collects comments. I will do that as a preparatory patch if people agree with this approach. Incidentally, this patch alone would fix the crashes fixed by the previous patches in the series, because with this, scoped_restore_current_thread's constructor doesn't throw either. --- gdb/blockframe.c | 6 +---- gdb/frame.c | 73 ++++++++++++++++++++++++++++++++++++++++---------------- gdb/frame.h | 22 +++++++++++++---- gdb/gdbthread.h | 4 ++++ gdb/stack.c | 9 ++++--- gdb/thread.c | 67 ++++++++++++++++----------------------------------- 6 files changed, 98 insertions(+), 83 deletions(-) diff --git a/gdb/blockframe.c b/gdb/blockframe.c index 05c26bc2c2..706b6db92c 100644 --- a/gdb/blockframe.c +++ b/gdb/blockframe.c @@ -448,14 +448,10 @@ find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr) struct frame_info * block_innermost_frame (const struct block *block) { - struct frame_info *frame; - if (block == NULL) return NULL; - frame = get_selected_frame_if_set (); - if (frame == NULL) - frame = get_current_frame (); + frame_info *frame = get_selected_frame (NULL); while (frame != NULL) { const struct block *frame_block = get_frame_block (frame, NULL); diff --git a/gdb/frame.c b/gdb/frame.c index ff27b9f00e..e7dffd204b 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -297,17 +297,15 @@ frame_stash_invalidate (void) /* See frame.h */ scoped_restore_selected_frame::scoped_restore_selected_frame () { - m_fid = get_frame_id (get_selected_frame (NULL)); + get_selected_frame_info (&m_fid, &m_level); } /* 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); + /* Use the lazy variant because we don't want to do any work here + that might throw, since we're in a dtor. */ + select_frame_lazy (m_fid, m_level); } /* Flag to control debugging. */ @@ -1641,9 +1639,24 @@ get_current_frame (void) } /* The "selected" stack frame is used by default for local and arg - access. May be zero, for no selected frame. */ - + access. */ + +/* If SELECTED_FRAME is NULL, then the selected frame is found in + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL. Those are used by + get_selected_frame to re-find the selected frame. If + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, there + is no selected frame, in which case the next time + get_selected_frame is called, it selects the current frame. */ static struct frame_info *selected_frame; +static frame_id selected_frame_id = null_frame_id; +static int selected_frame_level = -1; + +void +get_selected_frame_info (frame_id *frame_id, int *level) +{ + *frame_id = selected_frame_id; + *level = selected_frame_level; +} int has_stack_frames (void) @@ -1671,6 +1684,8 @@ has_stack_frames (void) return 1; } +void restore_selected_frame (struct frame_id a_frame_id, int frame_level); + /* Return the selected frame. Always non-NULL (unless there isn't an inferior sufficient for creating a frame) in which case an error is thrown. */ @@ -1682,24 +1697,14 @@ get_selected_frame (const char *message) { if (message != NULL && !has_stack_frames ()) error (("%s"), message); - /* Hey! Don't trust this. It should really be re-finding the - last selected frame of the currently selected thread. This, - though, is better than nothing. */ - select_frame (get_current_frame ()); + + restore_selected_frame (selected_frame_id, selected_frame_level); } /* There is always a frame. */ gdb_assert (selected_frame != NULL); return selected_frame; } -/* If there is a selected frame, return it. Otherwise, return NULL. */ - -struct frame_info * -get_selected_frame_if_set (void) -{ - return selected_frame; -} - /* This is a variant of get_selected_frame() which can be called when the inferior does not have a frame; in that case it will return NULL instead of calling error(). */ @@ -1712,12 +1717,26 @@ deprecated_safe_get_selected_frame (void) return get_selected_frame (NULL); } -/* Select frame FI (or NULL - to invalidate the current frame). */ +/* Select frame FI (or NULL - to invalidate the selected frame). */ void select_frame (struct frame_info *fi) { selected_frame = fi; + selected_frame_level = frame_relative_level (fi); + if (selected_frame_level == 0) + { + /* Treat the current frame especially -- we want to always + save/restore it without warning, even if the frame ID changes + (see restore_selected_frame). Also get_frame_id may access + the target's registers/memory, and thus skipping get_frame_id + optimizes the common case. */ + selected_frame_level = -1; + selected_frame_id = null_frame_id; + } + else + selected_frame_id = get_frame_id (fi); + /* NOTE: cagney/2002-05-04: FI can be NULL. This occurs when the frame is being invalidated. */ @@ -1756,6 +1775,18 @@ select_frame (struct frame_info *fi) } } +void +select_frame_lazy (frame_id a_frame_id, int frame_level) +{ + /* get_selected_frame_info never returns level == 0, so we shouldn't + see it here either. */ + gdb_assert (frame_level != 0); + + selected_frame = nullptr; + selected_frame_id = a_frame_id; + selected_frame_level = frame_level; +} + /* Create an arbitrary (i.e. address specified by user) or innermost frame. Always returns a non-NULL value. */ diff --git a/gdb/frame.h b/gdb/frame.h index e835d49f9c..48222c69d3 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -181,8 +181,9 @@ class scoped_restore_selected_frame private: - /* The ID of the previously selected frame. */ + /* The ID and level of the previously selected frame. */ struct frame_id m_fid; + int m_level; }; /* Methods for constructing and comparing Frame IDs. */ @@ -329,13 +330,24 @@ extern void reinit_frame_cache (void); and then return that thread's previously selected frame. */ extern struct frame_info *get_selected_frame (const char *message); -/* If there is a selected frame, return it. Otherwise, return NULL. */ -extern struct frame_info *get_selected_frame_if_set (void); +/* Return frame ID and frame level of the selected frame. If there's + no selected frame or the selected frame is the current frame, + return null_frame_id/-1. This is preferred over getting the same + info out of get_selected_frame directly because this function does + not create the selected-frame's frame_info object if it hasn't been + created yet. */ +extern void get_selected_frame_info (frame_id *frame_id, int *level); -/* Select a specific frame. NULL, apparently implies re-select the - inner most frame. */ +/* Select a specific frame. NULL implies re-select the inner most + frame. */ extern void select_frame (struct frame_info *); +/* Setup frame A_FRAME_ID, with level FRAME_LEVEL as the selected + frame, but don't actually try to find it right now. The next call + to get_selected_frame will look it up (and cache the result). + null_frame_id/-1 implies re-select the inner most frame. */ +extern void select_frame_lazy (frame_id a_frame_id, int frame_level); + /* Given a FRAME, return the next (more inner, younger) or previous (more outer, older) frame. */ extern struct frame_info *get_prev_frame (struct frame_info *); diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 0166b2000f..edfdf98b3d 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -667,6 +667,10 @@ class scoped_restore_current_thread frame_id m_selected_frame_id; int m_selected_frame_level; bool m_was_stopped; + /* Save/restore the language as well, because selecting a frame + changes the current language to the frame's language if "set + language auto". */ + enum language m_lang; }; /* Returns a pointer into the thread_info corresponding to diff --git a/gdb/stack.c b/gdb/stack.c index 265e764dc2..93de451a12 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1836,9 +1836,9 @@ trailing_outermost_frame (int count) static void select_frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); + frame_info *prev_frame = get_selected_frame (NULL); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame (NULL) != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); } @@ -1857,10 +1857,9 @@ select_frame_for_mi (struct frame_info *fi) static void frame_command_core (struct frame_info *fi, bool ignored) { - struct frame_info *prev_frame = get_selected_frame_if_set (); - + frame_info *prev_frame = get_selected_frame (nullptr); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame (nullptr) != prev_frame) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); else print_selected_thread_frame (current_uiout, USER_SELECTED_FRAME); diff --git a/gdb/thread.c b/gdb/thread.c index 1ec047e35b..8a5634eae3 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1325,20 +1325,25 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) switch_to_thread (thr); } -static void +void restore_selected_frame (struct frame_id a_frame_id, int frame_level); + +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. */ + /* This either means there was no selected frame, or the selected + frame was the inner most (the current frame). */ if (frame_level == -1) { - select_frame (NULL); + select_frame (get_current_frame ()); return; } - gdb_assert (frame_level >= 0); + /* select_frame never saves 0 in selected_frame_level, so we + shouldn't see it here. */ + 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 @@ -1408,23 +1413,19 @@ 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); + { + /* Use the lazy variant because we don't want to do any work + here that might throw, since we're in a dtor. */ + select_frame_lazy (m_selected_frame_id, m_selected_frame_level); + } + + set_language (m_lang); } scoped_restore_current_thread::~scoped_restore_current_thread () { if (!m_dont_restore) - { - try - { - restore (); - } - catch (const gdb_exception &ex) - { - /* We're in a dtor, there's really nothing else we can do - but swallow the exception. */ - } - } + restore (); if (m_thread != NULL) m_thread->decref (); @@ -1436,43 +1437,15 @@ scoped_restore_current_thread::scoped_restore_current_thread () m_inf = current_inferior (); m_inf->incref (); + m_lang = current_language->la_language; + if (inferior_ptid != null_ptid) { m_thread = inferior_thread (); m_thread->incref (); - struct frame_info *frame; - m_was_stopped = m_thread->state == THREAD_STOPPED; - if (m_was_stopped - && target_has_registers - && target_has_stack - && target_has_memory) - { - /* When processing internal events, there might not be a - selected frame. If we naively call get_selected_frame - here, then we can end up reading debuginfo for the - current frame, but we don't generally need the debuginfo - at this point. */ - frame = get_selected_frame_if_set (); - } - else - frame = NULL; - - try - { - m_selected_frame_id = get_frame_id (frame); - m_selected_frame_level = frame_relative_level (frame); - } - catch (const gdb_exception_error &ex) - { - m_selected_frame_id = null_frame_id; - m_selected_frame_level = -1; - - /* Better let this propagate. */ - if (ex.error == TARGET_CLOSE_ERROR) - throw; - } + get_selected_frame_info (&m_selected_frame_id, &m_selected_frame_level); } else m_thread = NULL; -- 2.14.5