diff options
author | Pedro Alves <pedro@palves.net> | 2020-10-30 18:26:15 +0100 |
---|---|---|
committer | Pedro Alves <pedro@palves.net> | 2020-10-30 01:01:12 +0000 |
commit | 79952e69634bd02029e79676a38915de60052e89 (patch) | |
tree | 74f734787fcea919e5f6166aef3775147e7946a7 /gdb/frame.c | |
parent | 4dd5c35212cf4685bb14f7709508de22a36e7dc2 (diff) | |
download | gdb-79952e69634bd02029e79676a38915de60052e89.zip gdb-79952e69634bd02029e79676a38915de60052e89.tar.gz gdb-79952e69634bd02029e79676a38915de60052e89.tar.bz2 |
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) <m_level, m_lang>: 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) <m_lang>: New field.
* infrun.c (struct infcall_control_state) <selected_frame_level>:
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.
Change-Id: I73fd1cfc40d8513c28e5596383b7ecd8bcfe700f
Diffstat (limited to 'gdb/frame.c')
-rw-r--r-- | gdb/frame.c | 117 |
1 files changed, 93 insertions, 24 deletions
diff --git a/gdb/frame.c b/gdb/frame.c index 15168fb..bb835e2 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -317,17 +317,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. */ @@ -1685,10 +1683,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_selected_frame. */ + selected_frame = nullptr; +} + bool has_stack_frames () { @@ -1716,9 +1767,7 @@ has_stack_frames () return true; } -/* 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) @@ -1727,24 +1776,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(). */ @@ -1757,12 +1796,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. */ |