gdb/gdb-rhbz1909902-frame_id_p-assert-1.patch
DistroBaker e0c5ec26e5 Merged update from upstream sources
This is an automated DistroBaker update from upstream sources.
If you do not know what this is about or would like to opt out,
contact the OSCI team.

Source: https://src.fedoraproject.org/rpms/gdb.git#37f07ef81844ffce99bad7676b21c3c534df688a
2021-03-16 15:24:22 +00:00

594 lines
21 KiB
Diff

From FEDORA_PATCHES Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 30 Oct 2020 18:26:15 +0100
Subject: gdb-rhbz1909902-frame_id_p-assert-1.patch
;; Backport fix for frame_id_p assertion failure (RH BZ 1909902).
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
diff --git a/gdb/blockframe.c b/gdb/blockframe.c
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -464,14 +464,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
--- 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 ()
{
@@ -1715,9 +1766,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)
@@ -1726,24 +1775,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(). */
@@ -1756,12 +1795,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
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -186,8 +186,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. */
@@ -316,24 +322,49 @@ extern bool has_stack_frames ();
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,
+/* 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
--- 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
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9017,8 +9017,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
@@ -9047,27 +9049,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
@@ -9095,21 +9082,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
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1842,9 +1842,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);
}
@@ -1863,10 +1863,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
--- 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);
}
}