Re: [PATCH v2] RAII-fy make_cleanup_restore_current_thread & friends

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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