[v2] RAII-fy make_cleanup_restore_current_thread & friends

From: Pedro Alves <palves@redhat.com>

  On 04/27/2017 04:48 PM, Simon Marchi wrote:
> 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.

Replying to that here:

On 04/25/2017 08:50 PM, Sergio Durigan Junior wrote:
>> > @@ -3066,7 +3067,7 @@ update_inserted_breakpoint_locations (void)
>> >       there was an error.  */
>> >    tmp_error_stream.puts ("Warning:\n");
>> >  
>> > -  struct cleanup *cleanups = save_current_space_and_thread ();
>> > +  scoped_restore_current_pspace_and_thread restore;
> "restore" doesn't seem like a very good name; I think it's too generic.
> How about naming the variable "restore_pspace_thread" or something like
> that, like you did with other variables?

I should perhaps remark that "restore" is no more generic than "cleanups".  :-)

Seriously, I made that change, throughout.  Thanks.  It does clarify the code
when you need to reference the variable, in the cases the scoped_restore is
wrapped in a gdb::optional.  (Otherwise, I'm borderline.  All the info you
need is in the type name; the more you put in the variable name, the
more you risk getting out of sync via refactoring.)

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

I think it is, otherwise it's not clear why we need the optionals
dance and the different scoped_restores instead of just always
using scoped_restore_current_pspace_and_thread.

  I had already added this comment to scoped_restore_exited_inferior:

/* Save/restore inferior_ptid, current program space and current
   inferior.  Only use this if the current context points at an exited
   inferior (and therefore there's no current thread to save).  */

(and yes, we have too many of these inferior/thread/whatnot state 
save/restorers.  :-/ )

> 
> 
>> 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.

Honestly, I've been thinking more and more that the "empty line after
declarations" no longer makes sense nowaways, given types with
constructors, which I kind of read like a function call, and also
with the desire to exercise the freedom (from a language perspective)
to declare variables not-just-at-the-start-of-a-scope.

In this case, the resulting code looked like this:

      if (current_context->all || current_context->thread_group != -1)
	{
	  scoped_restore_current_thread restore_thread;

	  int pid = 0;
	  if (!current_context->all)
	    {
	      struct inferior *inf
		= find_inferior_id (current_context->thread_group);

	      pid = inf->pid;
	    }
	  iterate_over_threads (proceed_thread_callback, &pid);
	}

The idea was put the "pid" variable closer to where 
it's used, and the scoped_restore's ctor at the top of the scope,
kind of mirroring the fact that the dtor wil run at the end of
the scope.

Anyway, it's certainly a very minor, arguable detail, so I've
moved 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.

Good catch, thanks.

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

Yeah.

> Or by that time we'll be using shared_ptr everywhere :).

Maybe.  :-)  shared_ptr has the atomics cost, though.  I'm kind of wary of
the "just throw shared_ptr at the ownership problem" mindset, though 
it has its uses, of course.

> 
> 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?

Good point.  I've done that as a separate thread, here:
 https://sourceware.org/ml/gdb-patches/2017-05/msg00108.html

And here's what I pushed for make_cleanup_restore_current_thread:

From 5ed8105e02d0c6648b7faea9f4e941237b932717 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 4 May 2017 12:46:44 +0100
Subject: [PATCH] RAII-fy make_cleanup_restore_current_thread & friends

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.

gdb/ChangeLog:
2017-05-04  Pedro Alves  <palves@redhat.com>

	* Makefile.in (SFILES): Add progspace-and-thread.c.
	(HFILES_NO_SRCDIR): Add progspace-and-thread.h.
	(COMMON_OBS): Add progspace-and-thread.o.
	* breakpoint.c: Include "progspace-and-thread.h".
	(update_inserted_breakpoint_locations)
	(insert_breakpoint_locations, create_longjmp_master_breakpoint):
	Use scoped_restore_current_pspace_and_thread.
	(create_std_terminate_master_breakpoint): Use
	scoped_restore_current_program_space.
	(remove_breakpoint): Use scoped_restore_current_pspace_and_thread.
	(print_breakpoint_location): Use
	scoped_restore_current_program_space.
	(bp_loc_is_permanent): Use
	scoped_restore_current_pspace_and_thread.
	(resolve_sal_pc): Use scoped_restore_current_pspace_and_thread.
	(download_tracepoint_locations): Use
	scoped_restore_current_pspace_and_thread.
	(breakpoint_re_set): Use scoped_restore_current_pspace_and_thread.
	* exec.c (exec_close_1): Use scoped_restore_current_program_space.
	(enum step_over_calls_kind): Moved from inferior.h.
	(class scoped_restore_current_thread): New class.
	* gdbthread.h (make_cleanup_restore_current_thread): Delete
	declaration.
	(scoped_restore_current_thread): New class.
	* infcmd.c: Include "common/gdb_optional.h".
	(continue_1, proceed_after_attach): Use
	scoped_restore_current_thread.
	(notice_new_inferior): Use scoped_restore_current_thread.
	* inferior.c: Include "progspace-and-thread.h".
	(restore_inferior, save_current_inferior): Delete.
	(add_inferior_command, clone_inferior_command): Use
	scoped_restore_current_pspace_and_thread.
	* inferior.h (scoped_restore_current_inferior): New class.
	* infrun.c: Include "progspace-and-thread.h" and
	"common/gdb_optional.h".
	(follow_fork_inferior): Use
	scoped_restore_current_pspace_and_thread.
	(scoped_restore_exited_inferior): New class.
	(handle_vfork_child_exec_or_exit): Use
	scoped_restore_exited_inferior,
	scoped_restore_current_pspace_and_thread,
	scoped_restore_current_thread and scoped_restore.
	(fetch_inferior_event): Use scoped_restore_current_thread.
	* linespec.c (decode_line_full, decode_line_1): Use
	scoped_restore_current_program_space.
	* mi/mi-main.c: Include "progspace-and-thread.h".
	(exec_continue): Use scoped_restore_current_thread.
	(mi_cmd_exec_run): Use scoped_restore_current_pspace_and_thread.
	(mi_cmd_trace_frame_collected): Use scoped_restore_current_thread.
	* proc-service.c (ps_pglobal_lookup): Use
	scoped_restore_current_program_space.
	* progspace-and-thread.c: New file.
	* progspace-and-thread.h: New file.
	* progspace.c (release_program_space, clone_program_space): Use
	scoped_restore_current_program_space.
	(restore_program_space, save_current_program_space)
	(save_current_space_and_thread): Delete.
	(switch_to_program_space_and_thread): Moved to
	progspace-and-thread.c.
	* progspace.h (save_current_program_space)
	(save_current_space_and_thread): Delete declarations.
	(scoped_restore_current_program_space): New class.
	* remote.c (remote_btrace_maybe_reopen): Use
	scoped_restore_current_thread.
	* symtab.c: Include "progspace-and-thread.h".
	(skip_prologue_sal): Use scoped_restore_current_pspace_and_thread.
	* thread.c (print_thread_info_1): Use
	scoped_restore_current_thread.
	(struct current_thread_cleanup): Delete.
	(do_restore_current_thread_cleanup)
	(restore_current_thread_cleanup_dtor): Rename/convert both to ...
	(scoped_restore_current_thread::~scoped_restore_current_thread):
	... this new dtor.
	(make_cleanup_restore_current_thread): Rename/convert to ...
	(scoped_restore_current_thread::scoped_restore_current_thread):
	... this new ctor.
	(thread_apply_all_command): Use scoped_restore_current_thread.
	(thread_apply_command): Use scoped_restore_current_thread.
	* tracepoint.c (tdump_command): Use scoped_restore_current_thread.
	* varobj.c (value_of_root_1): Use scoped_restore_current_thread.
---
 gdb/ChangeLog              |  83 ++++++++++++++++
 gdb/Makefile.in            |   3 +
 gdb/breakpoint.c           |  97 +++++++-----------
 gdb/exec.c                 |  15 ++-
 gdb/gdbthread.h            |  22 ++++-
 gdb/infcmd.c               |  31 ++----
 gdb/inferior.c             |  29 +-----
 gdb/inferior.h             |  23 ++++-
 gdb/infrun.c               |  65 ++++++------
 gdb/linespec.c             |   6 +-
 gdb/mi/mi-main.c           |  12 +--
 gdb/proc-service.c         |  18 ++--
 gdb/progspace-and-thread.c |  43 ++++++++
 gdb/progspace-and-thread.h |  40 ++++++++
 gdb/progspace.c            |  80 +--------------
 gdb/progspace.h            |  32 +++---
 gdb/remote.c               |   5 +-
 gdb/symtab.c               |  12 +--
 gdb/thread.c               | 241 ++++++++++++++++++++-------------------------
 gdb/tracepoint.c           |   6 +-
 gdb/varobj.c               |   5 +-
 21 files changed, 453 insertions(+), 415 deletions(-)
 create mode 100644 gdb/progspace-and-thread.c
 create mode 100644 gdb/progspace-and-thread.h

[v2] RAII-fy make_cleanup_restore_current_thread & friends

Commit Message

Comments

Patch