[PATCH v2] gdb: make thread_info::m_thread_fsm a std::unique_ptr

[PATCH v2] gdb: make thread_info::m_thread_fsm a std::unique_ptr

[Lancelot SIX via Gdb-patches]

This is V2 for
https://sourceware.org/pipermail/gdb-patches/2022-January/185300.html.

Noteworthy changes since V1:

- thread_info::m_thread_fsm is now a private member, allowing assertions
  to be placed in the setter.
- Ensure that the thread FSM is properly removed from the thread_info
  object even if thread::fsm::clean_up throws.
- There was an instance where it was possible to leak a thread_fsm if an
  exception was thrown while evaluating a function argument.  The
  pattern was:

     thread_fsm *fsm = new ...;
     some_call (std::unique_ptr<thread_fsm> (fsm),
                if_this_throws_fsm_could_leak ());

  Fix this by ensuring that the unique_ptr manages the pointer before
  the function call.

All feedback welcome.
Best,
Lancelot.

---

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

To ensure consistent use, the field is made a private member
(m_thread_fsm).  The setter method (set_thread_fsm) can then check
that it is incorrect to associate a FSM to a thread_info object if
another one is already in place.  This is ensured by an assertion.

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     |  6 ++++--
 gdb/cli/cli-interp.c |  6 +++---
 gdb/gdbthread.h      | 37 ++++++++++++++++++++++++++++-----
 gdb/infcall.c        | 49 +++++++++++++++++++++++++-------------------
 gdb/infcmd.c         |  8 ++++----
 gdb/infrun.c         | 44 +++++++++++++++++----------------------
 gdb/mi/mi-interp.c   |  6 +++---
 gdb/thread.c         |  7 +++----
 8 files changed, 96 insertions(+), 67 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 9567c737cab..3176f6b682b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10830,8 +10830,10 @@ 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));
+  tp->set_thread_fsm
+    (std::unique_ptr<thread_fsm>
+     (new until_break_fsm (command_interp (), tp->global_num,
+			   std::move (breakpoints))));
 
   if (lj_deleter)
     lj_deleter->release ();
diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 6dc3e6ae2fa..13f599dcde2 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -111,9 +111,9 @@ should_print_stop_to_console (struct interp *console_interp,
 {
   if ((bpstat_what (tp->control.stop_bpstat).main_action
        == BPSTAT_WHAT_STOP_NOISY)
-      || tp->thread_fsm == NULL
-      || tp->thread_fsm->command_interp == console_interp
-      || !tp->thread_fsm->finished_p ())
+      || tp->get_thread_fsm () == nullptr
+      || tp->get_thread_fsm ()->command_interp == console_interp
+      || !tp->get_thread_fsm ()->finished_p ())
     return 1;
   return 0;
 }
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 9921dae7a71..f6e899a3b0c 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;
@@ -443,6 +444,32 @@ class thread_info : public refcounted_object,
     m_suspend.stop_reason = reason;
   }
 
+  /* Get the FSM associated with the thread.  */
+
+  struct thread_fsm *get_thread_fsm () const
+  {
+    return m_thread_fsm.get ();
+  }
+
+  /* Get the owning reference to the FSM associated with the thread.
+
+     After a call to this method, "get_thread_fsm == null".  */
+
+  std::unique_ptr<struct thread_fsm> release_thread_fsm ()
+  {
+    return std::move (m_thread_fsm);
+  }
+
+  /* Set the FSM associated with the current thread.
+
+     It is invalid to set the FSM if another FSM is already installed.  */
+
+  void set_thread_fsm (std::unique_ptr<struct thread_fsm> fsm)
+  {
+    gdb_assert (m_thread_fsm == nullptr);
+    m_thread_fsm = std::move (fsm);
+  }
+
   int current_line = 0;
   struct symtab *current_symtab = NULL;
 
@@ -480,11 +507,6 @@ class thread_info : public refcounted_object,
      when GDB gets back SIGTRAP from step_resume_breakpoint.  */
   int step_after_step_resume_breakpoint = 0;
 
-  /* 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;
-
   /* 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
      resume of the thread, and not immediately.  */
@@ -550,6 +572,11 @@ class thread_info : public refcounted_object,
 
      Nullptr if the thread does not have a user-given name.  */
   gdb::unique_xmalloc_ptr<char> m_name;
+
+  /* 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.  */
+  std::unique_ptr<struct thread_fsm> m_thread_fsm;
 };
 
 using thread_info_resumed_with_pending_wait_status_node
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 05cf18f0a7f..d1c054bd10f 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;
@@ -597,9 +597,8 @@ run_inferior_call (struct call_thread_fsm *sm,
   clear_proceed_status (0);
 
   /* 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;
+     (otherwise it'd clear this FSM).  */
+  call_thread->set_thread_fsm (std::move (sm));
 
   disable_watchpoints_before_interactive_call_start ();
 
@@ -1251,12 +1250,9 @@ 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 = call_thread->release_thread_fsm ();
+    struct call_thread_fsm *sm;
 
     /* Save this thread's ptid, we need it later but the thread
        may have exited.  */
@@ -1273,17 +1269,19 @@ call_function_by_hand_dummy (struct value *function,
 			      values_type,
 			      return_method != return_method_normal,
 			      struct_addr);
-
-    e = run_inferior_call (sm, call_thread.get (), real_pc);
+    {
+      std::unique_ptr<call_thread_fsm> sm_up (sm);
+      e = run_inferior_call (std::move (sm_up), 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->get_thread_fsm () == sm);
 
-	if (call_thread->thread_fsm->finished_p ())
+	if (call_thread->get_thread_fsm ()->finished_p ())
 	  {
 	    struct value *retval;
 
@@ -1297,11 +1295,16 @@ call_function_by_hand_dummy (struct value *function,
 	    /* Get the return value.  */
 	    retval = sm->return_value;
 
-	    /* 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;
+	    /* Restore the original FSM and clean up / destroh the call FSM.
+	       Doing it in this order ensures that if the call to clean_up
+	       throws, the original FSM is properly restored.  */
+	    {
+	      std::unique_ptr<thread_fsm> finalizing
+		= call_thread->release_thread_fsm ();
+	      call_thread->set_thread_fsm (std::move (saved_sm));
+
+	      finalizing->clean_up (call_thread.get ());
+	    }
 
 	    maybe_remove_breakpoints ();
 
@@ -1315,9 +1318,13 @@ 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;
+	{
+	  std::unique_ptr<thread_fsm> finalizing
+	    = call_thread->release_thread_fsm ();
+	  call_thread->set_thread_fsm (std::move (saved_sm));
+
+	  finalizing->clean_up (call_thread.get ());
+	}
       }
   }
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 994dd5b32a3..63f633e601f 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -848,7 +848,7 @@ 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;
+  thr->set_thread_fsm (std::unique_ptr<thread_fsm> (step_sm));
 
   step_command_fsm_prepare (step_sm, skip_subroutines,
 			    single_inst, count, thr);
@@ -865,7 +865,7 @@ step_1 (int skip_subroutines, int single_inst, const char *count_string)
 
       /* Stepped into an inline frame.  Pretend that we've
 	 stopped.  */
-      thr->thread_fsm->clean_up (thr);
+      thr->get_thread_fsm ()->clean_up (thr);
       proceeded = normal_stop ();
       if (!proceeded)
 	inferior_event_handler (INF_EXEC_COMPLETE);
@@ -1355,7 +1355,7 @@ 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;
+  tp->set_thread_fsm (std::unique_ptr<thread_fsm> (sm));
   lj_deleter.release ();
 
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
@@ -1762,7 +1762,7 @@ finish_command (const char *arg, int from_tty)
 
   sm = new finish_command_fsm (command_interp ());
 
-  tp->thread_fsm = sm;
+  tp->set_thread_fsm (std::unique_ptr<thread_fsm> (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..6648028cbe2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -698,7 +698,6 @@ follow_fork ()
   int current_line = 0;
   symtab *current_symtab = NULL;
   struct frame_id step_frame_id = { 0 };
-  struct thread_fsm *thread_fsm = NULL;
 
   if (!non_stop)
     {
@@ -741,6 +740,7 @@ follow_fork ()
     case TARGET_WAITKIND_VFORKED:
       {
 	ptid_t parent, child;
+	std::unique_ptr<struct thread_fsm> thread_fsm;
 
 	/* If the user did a next/step, etc, over a fork call,
 	   preserve the stepping state in the fork child.  */
@@ -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 = tp->release_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,7 @@ follow_fork ()
 		    tp->control.step_frame_id = step_frame_id;
 		    tp->control.exception_resume_breakpoint
 		      = exception_resume_breakpoint;
-		    tp->thread_fsm = thread_fsm;
+		    tp->set_thread_fsm (std::move (thread_fsm));
 		  }
 		else
 		  {
@@ -2651,8 +2650,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->release_thread_fsm ();
 
   tp->control.trap_expected = 0;
   tp->control.step_range_start = 0;
@@ -3935,24 +3933,24 @@ reinstall_readline_callback_handler_cleanup ()
 static void
 clean_up_just_stopped_threads_fsms (struct execution_control_state *ecs)
 {
-  if (ecs->event_thread != NULL
-      && ecs->event_thread->thread_fsm != NULL)
-    ecs->event_thread->thread_fsm->clean_up (ecs->event_thread);
+  if (ecs->event_thread != nullptr
+      && ecs->event_thread->get_thread_fsm () != nullptr)
+    ecs->event_thread->get_thread_fsm ()->clean_up (ecs->event_thread);
 
   if (!non_stop)
     {
       for (thread_info *thr : all_non_exited_threads ())
 	{
-	  if (thr->thread_fsm == NULL)
+	  if (thr->get_thread_fsm () == nullptr)
 	    continue;
 	  if (thr == ecs->event_thread)
 	    continue;
 
 	  switch_to_thread (thr);
-	  thr->thread_fsm->clean_up (thr);
+	  thr->get_thread_fsm ()->clean_up (thr);
 	}
 
-      if (ecs->event_thread != NULL)
+      if (ecs->event_thread != nullptr)
 	switch_to_thread (ecs->event_thread);
     }
 }
@@ -4103,13 +4101,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->get_thread_fsm () != nullptr)
+	  should_stop = thr->get_thread_fsm ()->should_stop (thr);
 
 	if (!should_stop)
 	  {
@@ -4122,8 +4115,9 @@ fetch_inferior_event ()
 
 	    clean_up_just_stopped_threads_fsms (ecs);
 
-	    if (thr != NULL && thr->thread_fsm != NULL)
-	      should_notify_stop = thr->thread_fsm->should_notify_stop ();
+	    if (thr != nullptr && thr->get_thread_fsm () != nullptr)
+	      should_notify_stop
+	       = thr->get_thread_fsm ()->should_notify_stop ();
 
 	    if (should_notify_stop)
 	      {
@@ -8340,13 +8334,13 @@ print_stop_event (struct ui_out *uiout, bool displays)
   }
 
   tp = inferior_thread ();
-  if (tp->thread_fsm != NULL
-      && tp->thread_fsm->finished_p ())
+  if (tp->get_thread_fsm () != nullptr
+      && tp->get_thread_fsm ()->finished_p ())
     {
       struct return_value_info *rv;
 
-      rv = tp->thread_fsm->return_value ();
-      if (rv != NULL)
+      rv = tp->get_thread_fsm ()->return_value ();
+      if (rv != nullptr)
 	print_return_value (uiout, rv);
     }
 }
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index e69ad9aff2d..3cb5e53995e 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -630,12 +630,12 @@ mi_on_normal_stop_1 (struct bpstat *bs, int print_frame)
 
       tp = inferior_thread ();
 
-      if (tp->thread_fsm != NULL
-	  && tp->thread_fsm->finished_p ())
+      if (tp->get_thread_fsm () != nullptr
+	  && tp->get_thread_fsm ()->finished_p ())
 	{
 	  enum async_reply_reason reason;
 
-	  reason = tp->thread_fsm->async_reply_reason ();
+	  reason = tp->get_thread_fsm ()->async_reply_reason ();
 	  mi_uiout->field_string ("reason", async_reason_lookup (reason));
 	}
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 611be3f4633..cd2004a555d 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -160,11 +160,10 @@ thread_has_single_step_breakpoint_here (struct thread_info *tp,
 void
 thread_cancel_execution_command (struct thread_info *thr)
 {
-  if (thr->thread_fsm != NULL)
+  if (thr->get_thread_fsm () != nullptr)
     {
-      thr->thread_fsm->clean_up (thr);
-      delete thr->thread_fsm;
-      thr->thread_fsm = NULL;
+      std::unique_ptr<thread_fsm> fsm = thr->release_thread_fsm ();
+      fsm->clean_up (thr);
     }
 }
 

base-commit: 0d8cbc5f2fcbcb9eb207f12507fdfe04f3d3ae14
-- 
2.25.1

Re: [PATCH v2] gdb: make thread_info::m_thread_fsm a std::unique_ptr

[Simon Marchi via Gdb-patches]

> @@ -443,6 +444,32 @@ class thread_info : public refcounted_object,
>      m_suspend.stop_reason = reason;
>    }
>  
> +  /* Get the FSM associated with the thread.  */
> +
> +  struct thread_fsm *get_thread_fsm () const

Style comment: I don't know if this is a consistent style, but at least
in my changes, I usually omit the "get_" prefix for the getters.

> +  {
> +    return m_thread_fsm.get ();
> +  }
> +
> +  /* Get the owning reference to the FSM associated with the thread.
> +
> +     After a call to this method, "get_thread_fsm == null".  */

null -> nullptr.

LGTM with those changed.

Simon

RE: [PATCH v2] gdb: make thread_info::m_thread_fsm a std::unique_ptr

[Six, Lancelot via Gdb-patches]

[AMD Official Use Only]

> > @@ -443,6 +444,32 @@ class thread_info : public refcounted_object,
> >      m_suspend.stop_reason = reason;
> >    }
> >
> > +  /* Get the FSM associated with the thread.  */
> > +
> > +  struct thread_fsm *get_thread_fsm () const
>
> Style comment: I don't know if this is a consistent style, but at least in my changes, I usually omit the "get_" prefix > for the getters.

> > +     After a call to this method, "get_thread_fsm == null".  */
>
> null -> nullptr.
>
> LGTM with those changed.
>
> Simon

Hi,

I have applied both of those changes locally.  I'll give it a bit more time in case anyone has more comments to it.

Thanks,
Lancelot.

RE: [PATCH v2] gdb: make thread_info::m_thread_fsm a std::unique_ptr

[Six, Lancelot via Gdb-patches]

[AMD Official Use Only]

> > LGTM with those changed.
> >
> > Simon
>
> Hi,
>
> I have applied both of those changes locally.  I'll give it a bit more time in case anyone has more comments to it.

Hi,

I have just pushed this.

Lancelot.