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.
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_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.
* 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.diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 05c26bc..706b6db 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -448,14 +448,10 @@
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 ff27b9f..2a88b7b 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -297,17 +297,15 @@
/* 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,51 @@
}
/* 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. */
+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 a_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);
+
+ 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 @@
{
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 +1741,42 @@
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 e835d49..97d50b6 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -181,8 +181,14 @@
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. */
@@ -329,13 +335,33 @@
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);
+
/* 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 0166b20..edfdf98 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -667,6 +667,10 @@
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 265e764..93de451 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1836,9 +1836,9 @@
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 @@
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 a3c2be7..e748029 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1325,20 +1325,25 @@
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). */
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,22 +1414,14 @@
&& 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 ();
if (m_thread != NULL)
m_thread->decref ();
@@ -1436,47 +1433,15 @@
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)
- {
- m_thread->decref ();
- m_inf->decref ();
- throw;
- }
- }
+ save_selected_frame (&m_selected_frame_id, &m_selected_frame_level);
}
else
m_thread = NULL;
