MatCat.OpenSource/8sa1-binutils-gdb
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Make scoped_restore_current_thread's cdtors exception free (RFC)

Browse Source 
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
This commit is contained in:
Pedro Alves 2020-10-30 18:26:15 +01:00
parent 4dd5c35212
commit 79952e6963
8 changed files with 205 additions and 123 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
gdb/ChangeLog
View File

@ -1,3 +1,42 @@
2020-10-30 Pedro Alves <pedro@palves.net>
* 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.
2020-10-29 Simon Marchi <simon.marchi@polymtl.ca>
* gdbarch.sh (displaced_step_hw_singlestep): Adjust

6
gdb/blockframe.c
View File

@ -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);

115
gdb/frame.c
View File

@ -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. */

51
gdb/frame.h
View File

@ -186,8 +186,14 @@ public:
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);
extern struct frame_info *get_selected_frame (const char *message = nullptr);
/* 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 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 *);

4
gdb/gdbthread.h
View File

@ -673,6 +673,10 @@ private:
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

40
gdb/infrun.c
View File

@ -9003,8 +9003,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
@ -9033,27 +9035,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
@ -9081,21 +9068,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;

9
gdb/stack.c
View File

@ -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);

64
gdb/thread.c
View File

@ -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);
}
}