This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] RAII-fy make_cleanup_restore_current_thread & friends
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 27 Apr 2017 11:48:11 -0400
- Subject: Re: [PATCH v2] RAII-fy make_cleanup_restore_current_thread & friends
- Authentication-results: sourceware.org; auth=none
- References: <1492618687-11704-1-git-send-email-palves@redhat.com>
On 2017-04-19 12:18, Pedro Alves wrote:
New in v2:
- v1 had missed deleting save_current_program_space /
restore_program_space.
After all the make_cleanup_restore_current_thread fixing, I thought
I'd convert that and its relatives (which are all cleanups) to RAII
classes.
scoped_restore_current_pspace_and_thread was put in a separate file to
avoid a circular dependency.
Tested on x86-64 Fedora 23, native and gdbserver.
I agree with Sergio's comment. A few other comments below, but in
general it LGTM.
@@ -922,16 +936,17 @@ handle_vfork_child_exec_or_exit (int exec)
inf->vfork_parent->pending_detach = 0;
+ gdb::optional<scoped_restore_exited_inferior>
+ maybe_restore_inferior;
+ gdb::optional<scoped_restore_current_pspace_and_thread>
+ maybe_restore_thread;
+
+ /* If we're handling a child exit, then inferior_ptid points
+ at the inferior's pid, not to a thread. */
I wonder if this is still the right place for this comment. The other
logical place would be in scoped_restore_exited_inferior.
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index c3e7bf7..8145eb5 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -59,6 +59,7 @@
#include <ctype.h>
#include "run-time-clock.h"
#include <chrono>
+#include "progspace-and-thread.h"
enum
{
@@ -274,9 +275,9 @@ exec_continue (char **argv, int argc)
See comment on infcmd.c:proceed_thread_callback for rationale. */
if (current_context->all || current_context->thread_group != -1)
{
- int pid = 0;
- struct cleanup *back_to = make_cleanup_restore_current_thread ();
+ scoped_restore_current_thread restore_thread;
+ int pid = 0;
I'd keep "int pid" above the empty line.
@@ -2794,7 +2793,7 @@ mi_cmd_trace_frame_collected (const char
*command, char **argv, int argc)
/* This command only makes sense for the current frame, not the
selected frame. */
- old_chain = make_cleanup_restore_current_thread ();
The declaration of old_chain can be removed.
@@ -1571,77 +1575,53 @@ restore_selected_frame (struct frame_id
a_frame_id, int frame_level)
}
}
-/* Data used by the cleanup installed by
- 'make_cleanup_restore_current_thread'. */
-
-struct current_thread_cleanup
-{
- thread_info *thread;
- struct frame_id selected_frame_id;
- int selected_frame_level;
- int was_stopped;
- inferior *inf;
-};
-
-static void
-do_restore_current_thread_cleanup (void *arg)
+scoped_restore_current_thread::~scoped_restore_current_thread ()
{
- struct current_thread_cleanup *old = (struct current_thread_cleanup
*) arg;
-
/* If an entry of thread_info was previously selected, it won't be
deleted because we've increased its refcount. The thread
represented
by this thread_info entry may have already exited (due to normal
exit,
detach, etc), so the thread_info.state is THREAD_EXITED. */
- if (old->thread != NULL
+ if (m_thread != NULL
/* If the previously selected thread belonged to a process that
has
in the mean time exited (or killed, detached, etc.), then don't
revert
back to it, but instead simply drop back to no thread selected. */
- && old->inf->pid != 0)
- switch_to_thread (old->thread);
+ && m_inf->pid != 0)
+ switch_to_thread (m_thread);
else
{
switch_to_no_thread ();
- set_current_inferior (old->inf);
+ set_current_inferior (m_inf);
}
/* The running state of the originally selected thread may have
changed, so we have to recheck it here. */
if (inferior_ptid != null_ptid
- && old->was_stopped
+ && m_was_stopped
&& is_stopped (inferior_ptid)
&& target_has_registers
&& target_has_stack
&& target_has_memory)
- restore_selected_frame (old->selected_frame_id,
- old->selected_frame_level);
-}
-
-static void
-restore_current_thread_cleanup_dtor (void *arg)
-{
- struct current_thread_cleanup *old = (struct current_thread_cleanup
*) arg;
+ restore_selected_frame (m_selected_frame_id,
m_selected_frame_level);
- if (old->thread != NULL)
- old->thread->decref ();
-
- old->inf->decref ();
- xfree (old);
+ if (m_thread != NULL)
+ m_thread->decref ();
+ m_inf->decref ();
}
The cleanup did the decref in the dtor, I assume because we still want
to decref when the cleanup is discarded. If we ever add a release
method to this restore, we need to remember to do that. Or by that time
we'll be using shared_ptr everywhere :).
-struct cleanup *
-make_cleanup_restore_current_thread (void)
+scoped_restore_current_thread::scoped_restore_current_thread ()
{
- struct current_thread_cleanup *old = XNEW (struct
current_thread_cleanup);
-
- old->thread = NULL;
- old->inf = current_inferior ();
+ m_thread = NULL;
+ m_inf = current_inferior ();
if (inferior_ptid != null_ptid)
{
+ thread_info *tp = find_thread_ptid (inferior_ptid);
struct frame_info *frame;
- old->was_stopped = is_stopped (inferior_ptid);
- if (old->was_stopped
+ gdb_assert (tp != NULL);
+
+ m_was_stopped = tp->state == THREAD_STOPPED;
+ if (m_was_stopped
&& target_has_registers
&& target_has_stack
&& target_has_memory)
This does a bit more than 1:1 translating (calling find_thread_ptid
earlier, the assert). I think it's good, but can you put it in a
separate patch before this one, so it's not lost in the sea of
mechanical changes of the current patch?
Thanks,
Simon