[PATCH v2] RAII-fy make_cleanup_restore_current_thread & friends
Simon Marchi
simon.marchi@polymtl.ca
Thu Apr 27 15:48:00 GMT 2017
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
More information about the Gdb-patches
mailing list