[PATCH] gdb: make thread_info::thread_fsm a std::unique_ptr

[PATCH] gdb: make thread_info::thread_fsm a std::unique_ptr

[Lancelot SIX]

While working on function calls, I realized that the thread_fsm member
of struct thread_info is a raw pointer to a resource it owns.  This
commit changes the type of the thread_fsm member to a std::unique_ptr in
order to signify this ownership relationship and slightly ease resource
management (no need to manually call delete).

The function run_inferior_call takes an argument as a pointer to a
call_thread_fsm and installs it in it in a thread_info instance.  Also
change this function's signature to accept a unique_ptr in order to
signify that the ownership of the call_thread_fsm is transferred during
the call.

No user visible change expected after this commit.

Tested on x86_64-linux with no regression observed.

Change-Id: Ia1224f72a4afa247801ce6650ce82f90224a9ae8
---
 gdb/breakpoint.c |  5 +++--
 gdb/gdbthread.h  |  3 ++-
 gdb/infcall.c    | 23 ++++++++++-------------
 gdb/infcmd.c     |  9 ++++++---
 gdb/infrun.c     | 20 +++++++-------------
 gdb/thread.c     |  3 +--
 6 files changed, 29 insertions(+), 34 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1812bfe42f9..b2821042303 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10838,8 +10838,9 @@ until_break_command (const char *arg, int from_tty, int anywhere)
       breakpoints.emplace_back (std::move (location_breakpoint));
     }
 
-  tp->thread_fsm = new until_break_fsm (command_interp (), tp->global_num,
-					std::move (breakpoints));
+  gdb_assert (tp->thread_fsm == nullptr);
+  tp->thread_fsm.reset (new until_break_fsm (command_interp (), tp->global_num,
+					     std::move (breakpoints)));
 
   if (lj_deleter)
     lj_deleter->release ();
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 9921dae7a71..735f37300f6 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -34,6 +34,7 @@ struct symtab;
 #include "gdbsupport/forward-scope-exit.h"
 #include "displaced-stepping.h"
 #include "gdbsupport/intrusive_list.h"
+#include "thread-fsm.h"
 
 struct inferior;
 struct process_stratum_target;
@@ -483,7 +484,7 @@ class thread_info : public refcounted_object,
   /* Pointer to the state machine manager object that handles what is
      left to do for the thread's execution command after the target
      stops.  Several execution commands use it.  */
-  struct thread_fsm *thread_fsm = NULL;
+  std::unique_ptr<struct thread_fsm> thread_fsm = nullptr;
 
   /* This is used to remember when a fork or vfork event was caught by
      a catchpoint, and thus the event is to be followed at the next
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 05cf18f0a7f..a178051391c 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -574,7 +574,7 @@ call_thread_fsm::should_notify_stop ()
    thrown errors.  The caller should rethrow if there's an error.  */
 
 static struct gdb_exception
-run_inferior_call (struct call_thread_fsm *sm,
+run_inferior_call (std::unique_ptr<call_thread_fsm> sm,
 		   struct thread_info *call_thread, CORE_ADDR real_pc)
 {
   struct gdb_exception caught_error;
@@ -599,7 +599,7 @@ run_inferior_call (struct call_thread_fsm *sm,
   /* Associate the FSM with the thread after clear_proceed_status
      (otherwise it'd clear this FSM), and before anything throws, so
      we don't leak it (and any resources it manages).  */
-  call_thread->thread_fsm = sm;
+  call_thread->thread_fsm = std::move (sm);
 
   disable_watchpoints_before_interactive_call_start ();
 
@@ -1251,12 +1251,10 @@ call_function_by_hand_dummy (struct value *function,
      just below is the place to chop this function in two..  */
 
   {
-    struct thread_fsm *saved_sm;
-    struct call_thread_fsm *sm;
-
     /* Save the current FSM.  We'll override it.  */
-    saved_sm = call_thread->thread_fsm;
-    call_thread->thread_fsm = NULL;
+    std::unique_ptr<thread_fsm> saved_sm
+      = std::move (call_thread->thread_fsm);
+    struct call_thread_fsm *sm;
 
     /* Save this thread's ptid, we need it later but the thread
        may have exited.  */
@@ -1274,14 +1272,15 @@ call_function_by_hand_dummy (struct value *function,
 			      return_method != return_method_normal,
 			      struct_addr);
 
-    e = run_inferior_call (sm, call_thread.get (), real_pc);
+    e = run_inferior_call (std::unique_ptr<call_thread_fsm> (sm),
+			   call_thread.get (), real_pc);
 
     gdb::observers::inferior_call_post.notify (call_thread_ptid, funaddr);
 
     if (call_thread->state != THREAD_EXITED)
       {
 	/* The FSM should still be the same.  */
-	gdb_assert (call_thread->thread_fsm == sm);
+	gdb_assert (call_thread->thread_fsm.get () == sm);
 
 	if (call_thread->thread_fsm->finished_p ())
 	  {
@@ -1300,8 +1299,7 @@ call_function_by_hand_dummy (struct value *function,
 	    /* Clean up / destroy the call FSM, and restore the
 	       original one.  */
 	    call_thread->thread_fsm->clean_up (call_thread.get ());
-	    delete call_thread->thread_fsm;
-	    call_thread->thread_fsm = saved_sm;
+	    call_thread->thread_fsm = std::move (saved_sm);
 
 	    maybe_remove_breakpoints ();
 
@@ -1316,8 +1314,7 @@ call_function_by_hand_dummy (struct value *function,
 	/* Didn't complete.  Clean up / destroy the call FSM, and restore the
 	   previous state machine, and handle the error.  */
 	call_thread->thread_fsm->clean_up (call_thread.get ());
-	delete call_thread->thread_fsm;
-	call_thread->thread_fsm = saved_sm;
+	call_thread->thread_fsm = std::move (saved_sm);
       }
   }
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9f4ed8bff13..af8b6c3eb46 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -848,7 +848,8 @@ step_1 (int skip_subroutines, int single_inst, const char *count_string)
      steps.  */
   thr = inferior_thread ();
   step_sm = new step_command_fsm (command_interp ());
-  thr->thread_fsm = step_sm;
+  gdb_assert (thr->thread_fsm == nullptr);
+  thr->thread_fsm.reset (step_sm);
 
   step_command_fsm_prepare (step_sm, skip_subroutines,
 			    single_inst, count, thr);
@@ -1355,7 +1356,8 @@ until_next_command (int from_tty)
   delete_longjmp_breakpoint_cleanup lj_deleter (thread);
 
   sm = new until_next_fsm (command_interp (), tp->global_num);
-  tp->thread_fsm = sm;
+  gdb_assert (tp->thread_fsm == nullptr);
+  tp->thread_fsm.reset (sm);
   lj_deleter.release ();
 
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
@@ -1762,7 +1764,8 @@ finish_command (const char *arg, int from_tty)
 
   sm = new finish_command_fsm (command_interp ());
 
-  tp->thread_fsm = sm;
+  gdb_assert (tp->thread_fsm == nullptr);
+  tp->thread_fsm.reset (sm);
 
   /* Finishing from an inline frame is completely different.  We don't
      try to show the "return value" - no way to locate it.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 2e7ed15723f..e3bc0706c68 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -698,7 +698,7 @@ follow_fork ()
   int current_line = 0;
   symtab *current_symtab = NULL;
   struct frame_id step_frame_id = { 0 };
-  struct thread_fsm *thread_fsm = NULL;
+  std::unique_ptr<struct thread_fsm> thread_fsm = nullptr;
 
   if (!non_stop)
     {
@@ -755,7 +755,7 @@ follow_fork ()
 	    step_frame_id = tp->control.step_frame_id;
 	    exception_resume_breakpoint
 	      = clone_momentary_breakpoint (tp->control.exception_resume_breakpoint);
-	    thread_fsm = tp->thread_fsm;
+	    thread_fsm = std::move (tp->thread_fsm);
 
 	    /* For now, delete the parent's sr breakpoint, otherwise,
 	       parent/child sr breakpoints are considered duplicates,
@@ -767,7 +767,6 @@ follow_fork ()
 	    tp->control.step_range_end = 0;
 	    tp->control.step_frame_id = null_frame_id;
 	    delete_exception_resume_breakpoint (tp);
-	    tp->thread_fsm = NULL;
 	  }
 
 	parent = inferior_ptid;
@@ -809,7 +808,8 @@ follow_fork ()
 		    tp->control.step_frame_id = step_frame_id;
 		    tp->control.exception_resume_breakpoint
 		      = exception_resume_breakpoint;
-		    tp->thread_fsm = thread_fsm;
+		    gdb_assert (tp->thread_fsm == nullptr);
+		    tp->thread_fsm = std::move (thread_fsm);
 		  }
 		else
 		  {
@@ -2651,8 +2651,7 @@ clear_proceed_status_thread (struct thread_info *tp)
   if (!signal_pass_state (tp->stop_signal ()))
     tp->set_stop_signal (GDB_SIGNAL_0);
 
-  delete tp->thread_fsm;
-  tp->thread_fsm = NULL;
+  tp->thread_fsm.reset ();
 
   tp->control.trap_expected = 0;
   tp->control.step_range_start = 0;
@@ -4103,13 +4102,8 @@ fetch_inferior_event ()
 
 	delete_just_stopped_threads_infrun_breakpoints ();
 
-	if (thr != NULL)
-	  {
-	    struct thread_fsm *thread_fsm = thr->thread_fsm;
-
-	    if (thread_fsm != NULL)
-	      should_stop = thread_fsm->should_stop (thr);
-	  }
+	if (thr != nullptr && thr->thread_fsm != nullptr)
+	  should_stop = thr->thread_fsm->should_stop (thr);
 
 	if (!should_stop)
 	  {
diff --git a/gdb/thread.c b/gdb/thread.c
index 611be3f4633..b13a066e7f5 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -163,8 +163,7 @@ thread_cancel_execution_command (struct thread_info *thr)
   if (thr->thread_fsm != NULL)
     {
       thr->thread_fsm->clean_up (thr);
-      delete thr->thread_fsm;
-      thr->thread_fsm = NULL;
+      thr->thread_fsm.reset ();
     }
 }
 

base-commit: 8ffb6df2aa60af2fae5378e2520fbf6bdd0b0962
-- 
2.25.1

Re: [PATCH] gdb: make thread_info::thread_fsm a std::unique_ptr

[Simon Marchi]

> @@ -483,7 +484,7 @@ class thread_info : public refcounted_object,
>    /* Pointer to the state machine manager object that handles what is
>       left to do for the thread's execution command after the target
>       stops.  Several execution commands use it.  */
> -  struct thread_fsm *thread_fsm = NULL;
> +  std::unique_ptr<struct thread_fsm> thread_fsm = nullptr;

No need to initialize to nullptr.  You could argue that this is clearer
this way, but at the moment we don't really do this anywhere else, as
far as I know.  So I would just follow the convention.

> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 9f4ed8bff13..af8b6c3eb46 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -848,7 +848,8 @@ step_1 (int skip_subroutines, int single_inst, const char *count_string)
>       steps.  */
>    thr = inferior_thread ();
>    step_sm = new step_command_fsm (command_interp ());
> -  thr->thread_fsm = step_sm;
> +  gdb_assert (thr->thread_fsm == nullptr);
> +  thr->thread_fsm.reset (step_sm);

Since you always assert that thread_fsm is nullptr before assigning it
(which is a good thing), it might be good to make the field private and
have a setter that does the assert at a single place.  What I would try
is:

 - Rename the thread_fsm field to m_thread_fsm, make it private
 - Add a thread_fsm method that returns a pointer to (constant?)
   thread_fsm
 - Add a set_thread_fsm method that accepts a unique_ptr and does the
   assert
 - Add a release_thread_fsm method, for those cases you need to steal
   the thread_fsm from the thread.
 - Add a clear_thread_fsm.  That is a bit redundant with
   release_thread_fsm, since instead of calling clear_thread_fsm, you
   could just call release_thread_fsm and do nothing with it.  But I
   think that clear_thread_fsm would show better the caller's intention.

> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 2e7ed15723f..e3bc0706c68 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -698,7 +698,7 @@ follow_fork ()
>    int current_line = 0;
>    symtab *current_symtab = NULL;
>    struct frame_id step_frame_id = { 0 };
> -  struct thread_fsm *thread_fsm = NULL;
> +  std::unique_ptr<struct thread_fsm> thread_fsm = nullptr;

Same here.  Also, I would move it to the narrowest scope possible.

> diff --git a/gdb/thread.c b/gdb/thread.c
> index 611be3f4633..b13a066e7f5 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -163,8 +163,7 @@ thread_cancel_execution_command (struct thread_info *thr)
>    if (thr->thread_fsm != NULL)
>      {
>        thr->thread_fsm->clean_up (thr);
> -      delete thr->thread_fsm;
> -      thr->thread_fsm = NULL;
> +      thr->thread_fsm.reset ();

This has me wondering: can clean_up throw?

If clean_up can't throw, then why don't we do this work in the
thread_fsm destructor instead?  Maybe it's just remmants of before
thread_fsm was C++-ified?  If so, that could be done as future work, no
need to worry about it.

But if clean_up can throw, then this means the thread_fsm will
wrongfully stay installed in the thread_info when we exit
thread_cancel_execution_command with an exception.  The way I would see
around it would be to do:

  std::unique_ptr<thread_fsm> fsm = std::move (thr->thread_fsm);
  fsm->clean_up ();
  thr->thread_fsm.reset ();  // not sure if needed or if we can assume
                             // the move left thr->thread_fsm empty...


Simon

RE: [PATCH] gdb: make thread_info::thread_fsm a std::unique_ptr

[Six, Lancelot]

[AMD Official Use Only]

>> +  std::unique_ptr<struct thread_fsm> thread_fsm = nullptr;
>
>No need to initialize to nullptr.  You could argue that this is clearer this way, but at the moment we don't really do this anywhere else, as far as I know.  So I would just follow the convention.

Will do.

>> +  gdb_assert (thr->thread_fsm == nullptr);  thr->thread_fsm.reset 
>> + (step_sm);
>
>Since you always assert that thread_fsm is nullptr before assigning it (which is a good thing), it might be good to make the field private and have a setter that does the assert at a single place.  What I would try
>is:
>
> - Rename the thread_fsm field to m_thread_fsm, make it private
> - Add a thread_fsm method that returns a pointer to (constant?)
>   thread_fsm
> - Add a set_thread_fsm method that accepts a unique_ptr and does the
>   assert
> - Add a release_thread_fsm method, for those cases you need to steal
>   the thread_fsm from the thread.
> - Add a clear_thread_fsm.  That is a bit redundant with
>   release_thread_fsm, since instead of calling clear_thread_fsm, you
>   could just call release_thread_fsm and do nothing with it.  But I
>   think that clear_thread_fsm would show better the caller's intention.
>

I'll change that as well.  I did use the assertions to convince myself that I did not miss something, but It make sense to have this enforced in an accessor.

>> +  std::unique_ptr<struct thread_fsm> thread_fsm = nullptr;
>
>Same here.  Also, I would move it to the narrowest scope possible.

Ok.

>>    if (thr->thread_fsm != NULL)
>>      {
>>        thr->thread_fsm->clean_up (thr);
>> -      delete thr->thread_fsm;
>> -      thr->thread_fsm = NULL;
>> +      thr->thread_fsm.reset ();
>
>This has me wondering: can clean_up throw?
>
>If clean_up can't throw, then why don't we do this work in the thread_fsm destructor instead?  Maybe it's just remmants of before thread_fsm was C++-ified?  If so, that could be done as future work, no need to worry about it.
>
>But if clean_up can throw, then this means the thread_fsm will wrongfully stay installed in the thread_info when we exit >thread_cancel_execution_command with an exception.  The way I would see around it would be to do:
>
 > std::unique_ptr<thread_fsm> fsm = std::move (thr->thread_fsm);
 > fsm->clean_up ();
>  thr->thread_fsm.reset ();  // not sure if needed or if we can assume
>                             // the move left thr->thread_fsm empty...

I do not really know if clean_up can currently throws, but since it can be overridden by inheritance, I am not sure we can rely on its current behavior. It is safer to just assume it can throw and just deal with it.  Thanks for spotting that.

I would have probably done something with SCOPE_EXIT to ensure that thread_fsm.reset is called, but your approach works as well and might be easier to read (with SCOPE_EXIT, you have to place the cleanup code before the section that might throw).  And I am pretty sure that a "up = std::move (other_up)" guarantees that other_up == nullptr after the operation (I'll double check before relying on this).

Thanks for the review,
Lancelot.