From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by sourceware.org (Postfix) with ESMTPS id 3D14E3865491 for ; Thu, 9 Jul 2020 22:22:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3D14E3865491 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-wm1-f68.google.com with SMTP id l2so3555337wmf.0 for ; Thu, 09 Jul 2020 15:22:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=X0tAIx3A49crbqq0R5xyL+gV9IPW9uoUi8A1bvJXLOM=; b=CVXL4FIj0/vkVTY1UNJgRAehnbUdIfxWbFo7WJeMxy+iTQNsY7REsR3KRBW9CCvATK H3TlZj6TWPganNRPuGBUmfJ5RK//MoYLoIjiLxs9rNQwChSyy44nvZUVZsBmC5qiJSbC dxsJzbB6Sx8AJtFdj6frtwDw0CXCHUqRqMAZY3uaihsGazS2irczz3pYVd3XajsDhVwJ 7lS1fmozFiudFa7ZYbFQP1P0YieBGfur4Icpg+IgXHV6V92iT9L5Cv7OspxUeKz9CiNm IKxde5Il+nhVhp0DS2Vqraylsaqh3Deu3qS392sYBKEFwgW1soJ7hI5Tuh2tX7oZiP2B 5ymw== X-Gm-Message-State: AOAM533DDL/0coqt177P2jNiAjo4D6Rgxo5tKjkqrH62sAwi1BymtL+a tPeSvgww+QSI4VlUxJKYMMmzFw2PEW6nTw== X-Google-Smtp-Source: ABdhPJz1ySutT+HbV8ofFGjn86zLKk2++1ZJwDH5c15ks1LsjEiLxss6asWPsIUCUVJL1OHs0egotA== X-Received: by 2002:a1c:7416:: with SMTP id p22mr2057781wmc.32.1594333341287; Thu, 09 Jul 2020 15:22:21 -0700 (PDT) Received: from ?IPv6:2001:8a0:f91a:c400:56ee:75ff:fe8d:232b? ([2001:8a0:f91a:c400:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id 5sm6132627wmk.9.2020.07.09.15.22.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Jul 2020 15:22:20 -0700 (PDT) Subject: Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) To: Simon Marchi , gdb-patches@sourceware.org References: <20200708233125.1030-1-pedro@palves.net> <20200708233125.1030-4-pedro@palves.net> <58a4020f-fbbb-611f-eb23-c1b3fa25d4f2@simark.ca> <39ad8351-3f99-1162-4d2f-b5dbee5756f8@simark.ca> From: Pedro Alves Message-ID: <47b34393-3833-bb85-84dc-9a8bde3e1a77@palves.net> Date: Thu, 9 Jul 2020 23:22:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <39ad8351-3f99-1162-4d2f-b5dbee5756f8@simark.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LOTSOFHASH, KAM_SHORT, 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: Thu, 09 Jul 2020 22:22:27 -0000 On 7/9/20 4:40 PM, Simon Marchi wrote: >> I've also polished the patch some more. I now renamed >> the current restore_selected_frame to lookup_selected_frame, >> to give space to the new save_selected_frame/restore_selected_frame >> pair. select_frame_lazy is now restore_selected_frame. >> save_selected_frame/restore_selected_frame are now noexcept, and >> their intro comments explain why. > > I noticed there is also a `restore_selected_frame` in infrun.c... I don't > know if it's replicating features also implemented in frame.c? Yeah, I saw that too, but didn't look too deeply, given I was more looking at validating the whole idea. I think that can be replaced, yes. Done now. BTW, I don't know why I missed it before, but we _already_ don't warn the user if we fail to restore frame #0: /* Warn the user. */ if (frame_level > 0 && !current_uiout->is_mi_like_p ()) ^^^^^^^^^^^^^^^ { And that came in with: commit 6c95b8df7fef5273da71c34775918c554aae0ea8 Author: Pedro Alves AuthorDate: Mon Oct 19 09:51:43 2009 +0000 2009-10-19 Pedro Alves Stan Shebs Add base multi-executable/process support to GDB. I'm pretty sure I wrote that bit of the patch... > >> @@ -1641,10 +1639,51 @@ 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. */ >> + >> +/* The "single source of truth" for the selected frame is the >> + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. Frame IDs can be >> + saved/restored across reinitializing the frame cache, while >> + frame_info pointers can't (frame_info objects are invalidated). If >> + we know the corresponding frame_info object, it is cached in >> + SELECTED_FRAME. If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are >> + null_ptid / -1, and the target has stack and is stopped, the >> + selected frame is the current (innermost) frame, otherwise there's >> + no selected frame. */ > > Pedantically, the `otherwise` could let the reader think that if > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are not null_ptid / -1, then there > is no selected frame. I'd suggest moving this out to its own sentence to be > clear. > > I'd also make it more explicit here that when the innermost frame is selected, > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL. Currently, it says that if > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1, it means the > innermost frame is selected, but it doesn't imply that if the innermost frame > is selected, then SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_ptid / -1 > (difference between if and "if and only if"). > > I think it would help to separate that in paragraphes to ease reading. > > Also, I just noticed that you comment uses `null_ptid` instead of `null_frame_id`, > which just shows how much `null_ptid` is engraved in your brain :). I also > re-worked the comment for 15 minutes before noticing :). > > So: > > /* The "single source of truth" for the selected frame is the > SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. > > Frame IDs can be saved/restored across reinitializing the frame cache, while > frame_info pointers can't (frame_info objects are invalidated). If we know > the corresponding frame_info object, it is cached in SELECTED_FRAME. > > If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, and the > target has stack and is stopped, the selected frame is the current > (innermost) frame. This means that SELECTED_FRAME_LEVEL is never 0 and > SELECTED_FRAME_ID is never the ID of the innermost frame. > > If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, and the > target has no stack or is executing, the selected, then there's no selected > frame. */ I like it. > >> +static frame_id selected_frame_id = null_frame_id; >> +static int selected_frame_level = -1; >> + >> +/* The cached frame_info object pointing to the selected frame. >> + Looked up on demand by get_selected_frame. */ >> static struct frame_info *selected_frame; > > Ah ok, in my previous reply I almost said: "so this essentially becomes > a cache for selected_frame_id/selected_frame_level", but changed it because > I *thought* I had understood that it wasn't the case when the frame_level > was > 0. But if we can word it this way, that makes it simpler to understand. > >> >> +/* See frame.h. */ >> + >> +void >> +save_selected_frame (frame_id *frame_id, int *frame_level) >> + noexcept >> +{ >> + *frame_id = selected_frame_id; >> + *frame_level = selected_frame_level; >> +} >> + >> +/* See frame.h. */ >> + >> +void >> +restore_selected_frame (frame_id a_frame_id, int frame_level) >> + noexcept >> +{ >> + /* get_selected_frame_info never returns level == 0, so we shouldn't >> + see it here either. */ >> + gdb_assert (frame_level != 0); > > We could also assert that a_frame_id can be null_frame_id IFF frame_level is -1. Done. > >> + >> + selected_frame_id = a_frame_id; >> + selected_frame_level = frame_level; >> + >> + /* Will be looked up latter by get_seleted_frame. */ >> + selected_frame = nullptr; >> +} >> + >> int >> has_stack_frames (void) >> { >> @@ -1682,24 +1721,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 ()); >> + >> + lookup_selected_frame (selected_frame_id, selected_frame_level); > > Could you fix (in this patch or another one) the comment of `get_selected_frame` > to be `/* See frame.h. */` and make sure that the version in frame.h is the > most up to date one? Done. > >> @@ -1712,12 +1741,42 @@ 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 lookup_selected_frame). E.g.: >> + >> + // The current frame is selected, the target had just stopped. >> + { >> + scoped_restore_selected_frame restore_frame; >> + some_operation_that_changes_the_stack (); >> + } >> + // scoped_restore_selected_frame's dtor runs, but the >> + // original frame_id can't be found. No matter whether it >> + // is found or not, we still end up with the now-current >> + // frame selected. Warning in lookup_selected_frame in this >> + // case seems pointless. >> + >> + Also get_frame_id may access the target's registers/memory, >> + and thus skipping get_frame_id optimizes the common case. >> + >> + Saving the selected frame this way makes get_selected_frame >> + and restore_current_frame return/re-select whatever frame is > > restore_selected_frame Fixed. > >> @@ -329,13 +335,33 @@ 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); >> - >> -/* 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 *); >> >> +/* Save the frame ID and frame level of the selected frame in FRAME_ID >> + and FRAME_LEVEL, to be restored later with restore_selected_frame. >> + 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, and thus doesn't throw. */ >> +extern void save_selected_frame (frame_id *frame_id, int *frame_level) >> + noexcept; >> + >> +/* Restore selected frame as saved with save_selected_frame. Does not >> + try to find the corresponding frame_info object. Instead the next >> + call to get_selected_frame will look it up and cache the result. >> + This function does not throw, it is designed to be safe to called >> + from the destructors of RAII types. */ >> +extern void restore_selected_frame (frame_id frame_id, int frame_level) >> + noexcept; >> + >> +/* Lookup the frame_info object for the selected frame FRAME_ID / >> + FRAME_LEVEL and cache the result. If FRAME_LEVEL > 0 and the >> + originally selected frame isn't found, warn and select the >> + innermost (current) frame. */ >> +extern void lookup_selected_frame (frame_id frame_id, int frame_level); > > As I mentioned above, I think the comments would be easier to read with > a bit more newlines. In general, I like to have one short sentence by > itself first, that says what the function does at a very high level. A > bit like in man pages you have a short description next to the name: > > rm - remove files or directories > sudo, sudoedit — execute a command as another user > ssh — OpenSSH remote login client > > Then, you can go in details about the behavior of the function in following > paragraphs, making sure to split different ideas in different paragraphs. > > At first, I thought it would just be nitpicking to ask people to space > out their comments and code a bit more, but now I think it really does > make a difference in readability (at least, it helps me). > Done. >> 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) > > I'm telling people that we try to use `nullptr` for new code. I don't > know what's your opinion on this. I would just like to have a simple > and clear so we don't have to wonder which of `NULL` and `nullptr` to > use. I just tend to use nullptr for new code, unless the surrounding code is already using NULL. In this case I just copy/pasted from elsewhere and didn't even realize it. NULL and nullptr are both cyan and thus look the same on my emacs. :-) I do tend to stick with writing "NULL" in comments. > >> 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) > > ... especially that you've used `nullptr` here :). > > Although maybe that in this case, get_selected_frame could have a default > parameter value of `nullptr`. > Done. >> diff --git a/gdb/thread.c b/gdb/thread.c >> index a3c2be7dd0..e0b49abf0c 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 >> -restore_selected_frame (struct frame_id a_frame_id, int frame_level) >> +/* See frame.h. */ >> + >> +void >> +lookup_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 innermost (the current frame). */ > > It's a bit heavy to always precise that `innermost` means the `current` > frame. Let's choose one name and stick to it, it will be clear enough > on its own. I'd use `current`, since that's how the functions are named > (e.g. get_current_frame). I understand there might be a bit confusion > between `selected` and `current` for newcomers, but once you know the > distinction, it's pretty clear. Sure. > > I'd also add to that comment, what is the action we take as a result. > > So, maybe: > > /* This either means there was no selected frame, or the selected frame was > the current one. In either case, try to select the current frame. */ OK. I'll remove the "try" though. > > >> @@ -1409,22 +1414,14 @@ scoped_restore_current_thread::restore () >> && target_has_stack >> && target_has_memory) >> restore_selected_frame (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 (); > > I don't know if it would be worth it, but I'd like if we could assert (abort > GDB) if an exception does try to exit the destructor. The `restore` method > is non-trivial and calls into other non-trivial functions, so it would be > possible for a change far far away to cause that to happen. It will already abort. Destructors are noexcept by default, so if an exception escapes a destructor, std::terminate() is called, and that calls abort by default. > > What do you think of keeping the try/catch, but using `gdb_assert_not_reached` > in it? Not sure. If we do that, we do get a nicer error message. However if the user says "n" to "Quit this debugging session" we still abort. /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) n This is a bug, please report it. For instructions, see: . /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello A problem internal to GDB has been detected, further debugging may prove unreliable. Create a core file of GDB? (y or n) n terminate called after throwing an instance of 'gdb_exception_quit' Aborted (core dumped) Maybe it would be interesting to add a variant of internal_error that did not throw a quit, so the user could swallow the exception... Maybe consider wrapping that as a generic facility to add to all non-trivial RAII destructors we have? Like a function that takes a function_view as parameter, so we would write: foo::~foo () { safe_dtor (__FILE__, __LINE__, [&] () { restore (); }); } Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT macro uses to be able to write: foo::~foo () { SAFE_DTOR { restore (); }; } Here's the current version of the patch. >From 63310d189bb2516e0fec2b0922041388a5a96374 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 9 Jul 2020 18:26:15 +0100 Subject: [PATCH] Make scoped_restore_current_thread's cdtors exception free (RFC) 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. Note that this patch renames restore_selected_frame to lookup_selected_frame, and adds a new restore_selected_frame function that doesn't throw, to be paired with the also-new save_selected_frame function. There's a restore_selected_frame function in infrun.c that I think can be replaced by the new one in frame.c. Also done in this patch is make the get_selected_frame's parameter be optional, so that we don't have to pass down nullptr explicitly all over the place. lookup_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 did make it extern and declared it in frame.h already, preparing for the move. I will do the move as a follow up 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/ChangeLog: * blockframe.c (block_innermost_frame): Use get_selected_frame. * frame.c (scoped_restore_selected_frame::scoped_restore_selected_frame): Use save_selected_frame. Save language as well. (scoped_restore_selected_frame::~scoped_restore_selected_frame): Use restore_selected_frame, and restore language as well. (selected_frame_id, selected_frame_level): New. (selected_frame): Update comments. (save_selected_frame, restore_selected_frame): New. (get_selected_frame): Use lookup_selected_frame. (get_selected_frame_if_set): Delete. (select_frame): Record selected_frame_level and selected_frame_id. * frame.h (scoped_restore_selected_frame) : New fields. (get_selected_frame): Make 'message' parameter optional. (get_selected_frame_if_set): Delete declaration. (select_frame): Update comments. (save_selected_frame, restore_selected_frame) (lookup_selected_frame): Declare. * gdbthread.h (scoped_restore_current_thread) : New field. * infrun.c (struct infcall_control_state) : New field. (save_infcall_control_state): Use save_selected_frame. (restore_selected_frame): Delete. (restore_infcall_control_state): Use restore_selected_frame. * stack.c (select_frame_command_core, frame_command_core): Use get_selected_frame. * thread.c (restore_selected_frame): Rename to ... (lookup_selected_frame): ... this and make extern. Select the current frame if the frame level is -1. (scoped_restore_current_thread::restore): Also restore the language. (scoped_restore_current_thread::~scoped_restore_current_thread): Don't try/catch. (scoped_restore_current_thread::scoped_restore_current_thread): Save the language as well. Use save_selected_frame. --- gdb/blockframe.c | 6 +-- gdb/frame.c | 117 +++++++++++++++++++++++++++++++++++++++++++------------ gdb/frame.h | 53 +++++++++++++++++++------ gdb/gdbthread.h | 4 ++ gdb/infrun.c | 40 ++++--------------- gdb/stack.c | 9 ++--- gdb/thread.c | 64 ++++++++---------------------- 7 files changed, 168 insertions(+), 125 deletions(-) diff --git a/gdb/blockframe.c b/gdb/blockframe.c index 05c26bc2c2..0b868d8341 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 (); 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..86cf9e4b69 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)); + m_lang = current_language->la_language; + save_selected_frame (&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); + restore_selected_frame (m_fid, m_level); + set_language (m_lang); } /* Flag to control debugging. */ @@ -1641,10 +1639,63 @@ 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. + + The "single source of truth" for the selected frame is the + SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. + + Frame IDs can be saved/restored across reinitializing the frame + cache, while frame_info pointers can't (frame_info objects are + invalidated). If we know the corresponding frame_info object, it + is cached in SELECTED_FRAME. + + If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, + and the target has stack and is stopped, the selected frame is the + current (innermost) frame. This means that SELECTED_FRAME_LEVEL is + never 0 and SELECTED_FRAME_ID is never the ID of the innermost + frame. + + If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, + and the target has no stack or is executing, then there's no + selected frame. */ +static frame_id selected_frame_id = null_frame_id; +static int selected_frame_level = -1; + +/* The cached frame_info object pointing to the selected frame. + Looked up on demand by get_selected_frame. */ static struct frame_info *selected_frame; +/* See frame.h. */ + +void +save_selected_frame (frame_id *frame_id, int *frame_level) + noexcept +{ + *frame_id = selected_frame_id; + *frame_level = selected_frame_level; +} + +/* See frame.h. */ + +void +restore_selected_frame (frame_id frame_id, int frame_level) + noexcept +{ + /* save_selected_frame never returns level == 0, so we shouldn't see + it here either. */ + gdb_assert (frame_level != 0); + + /* FRAME_ID can be null_frame_id only IFF frame_level is -1. */ + gdb_assert ((frame_level == -1 && !frame_id_p (frame_id)) + || (frame_level != -1 && frame_id_p (frame_id))); + + selected_frame_id = frame_id; + selected_frame_level = frame_level; + + /* Will be looked up later by get_seleted_frame. */ + selected_frame = nullptr; +} + int has_stack_frames (void) { @@ -1671,9 +1722,7 @@ has_stack_frames (void) return 1; } -/* 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. */ +/* See frame.h. */ struct frame_info * get_selected_frame (const char *message) @@ -1682,24 +1731,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 ()); + + lookup_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 +1751,42 @@ 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 lookup_selected_frame). E.g.: + + // The current frame is selected, the target had just stopped. + { + scoped_restore_selected_frame restore_frame; + some_operation_that_changes_the_stack (); + } + // scoped_restore_selected_frame's dtor runs, but the + // original frame_id can't be found. No matter whether it + // is found or not, we still end up with the now-current + // frame selected. Warning in lookup_selected_frame in this + // case seems pointless. + + Also get_frame_id may access the target's registers/memory, + and thus skipping get_frame_id optimizes the common case. + + Saving the selected frame this way makes get_selected_frame + and restore_current_frame return/re-select whatever frame is + the innermost (current) then. */ + 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. */ diff --git a/gdb/frame.h b/gdb/frame.h index e835d49f9c..4901de1bf5 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -181,8 +181,14 @@ 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; + + /* 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; }; /* Methods for constructing and comparing Frame IDs. */ @@ -318,24 +324,49 @@ extern int has_stack_frames (void); modifies the target invalidating the frame cache). */ extern void reinit_frame_cache (void); -/* 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. */ +/* Return the selected frame. Always returns non-NULL. If there + isn't an inferior sufficient for creating a frame, an error is + thrown. When MESSAGE is non-NULL, use it for the error message, + otherwise 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); - -/* If there is a selected frame, return it. Otherwise, return NULL. */ -extern struct frame_info *get_selected_frame_if_set (void); +extern struct frame_info *get_selected_frame (const char *message = nullptr); -/* 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 *); +/* Save the frame ID and frame level of the selected frame in FRAME_ID + and FRAME_LEVEL, to be restored later with restore_selected_frame. + + 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, and thus is more efficient and doesn't throw. */ +extern void save_selected_frame (frame_id *frame_id, int *frame_level) + noexcept; + +/* Restore selected frame as saved with save_selected_frame. + + Does not try to find the corresponding frame_info object. Instead + the next call to get_selected_frame will look it up and cache the + result. + + This function does not throw. It is designed to be safe to called + from the destructors of RAII types. */ +extern void restore_selected_frame (frame_id frame_id, int frame_level) + noexcept; + +/* Lookup the frame_info object for the selected frame FRAME_ID / + FRAME_LEVEL and cache the result. + + If FRAME_LEVEL > 0 and the originally selected frame isn't found, + warn and select the innermost (current) frame. */ +extern void lookup_selected_frame (frame_id 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 ab5771fdb4..da20dd4b4e 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -673,6 +673,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/infrun.c b/gdb/infrun.c index 31266109a6..ad16385029 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -9266,8 +9266,10 @@ 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. */ + /* ID and level of the selected frame when the inferior function + call was made. */ struct frame_id selected_frame_id {}; + int selected_frame_level = -1; }; /* Save all of the information associated with the inferior<==>gdb @@ -9296,27 +9298,12 @@ 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)); + save_selected_frame (&inf_status->selected_frame_id, + &inf_status->selected_frame_level); 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 @@ -9344,21 +9331,8 @@ restore_infcall_control_state (struct infcall_control_state *inf_status) if (target_has_stack) { - /* The point of the try/catch is that if the stack is clobbered, - walking the stack might encounter a garbage pointer and - error() trying to dereference it. */ - try - { - restore_selected_frame (inf_status->selected_frame_id); - } - catch (const gdb_exception_error &ex) - { - exception_fprintf (gdb_stderr, ex, - "Unable to restore previously selected frame:\n"); - /* Error in restoring the selected frame. Select the - innermost frame. */ - select_frame (get_current_frame ()); - } + restore_selected_frame (inf_status->selected_frame_id, + inf_status->selected_frame_level); } delete inf_status; diff --git a/gdb/stack.c b/gdb/stack.c index 265e764dc2..dbc02f5ff0 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 (); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame () != 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 (); select_frame (fi); - if (get_selected_frame_if_set () != prev_frame) + if (get_selected_frame () != 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 4dce1ef82a..32f2ccdbd6 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1325,20 +1325,26 @@ 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) +/* See frame.h. */ + +void +lookup_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 current frame. In either case, select 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 @@ -1409,64 +1415,28 @@ scoped_restore_current_thread::restore () && target_has_stack && target_has_memory) restore_selected_frame (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 (); } scoped_restore_current_thread::scoped_restore_current_thread () { m_inf = inferior_ref::new_reference (current_inferior ()); + m_lang = current_language->la_language; + if (inferior_ptid != null_ptid) { m_thread = thread_info_ref::new_reference (inferior_thread ()); - 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; - } + save_selected_frame (&m_selected_frame_id, &m_selected_frame_level); } } base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0 prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6 prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52 prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1 prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22 prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f prerequisite-patch-id: b06932368faf983af6404ae711dfed17e5e32c6f -- 2.14.5