Replace finish_thread_state_cleanup with a RAII class

Message ID 20180328144428.15032-1-palves@redhat.com
State New
Headers show
Series
  • Replace finish_thread_state_cleanup with a RAII class
Related show

Commit Message

Pedro Alves March 28, 2018, 2:44 p.m.
A small patch I was sitting on in the multi-target branch.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* gdbthread.h (finish_thread_state_cleanup): Delete declaration.
	(scoped_finish_thread_state): New class.
	* infcmd.c (run_command_1): Use it instead of finish_thread_state
	cleanup.
	* infrun.c (proceed, prepare_for_detach, wait_for_inferior)
	(fetch_inferior_event, normal_stop): Likewise.
	* thread.c (finish_thread_state_cleanup): Delete.
---
 gdb/gdbthread.h | 31 +++++++++++++++++++++++++++----
 gdb/infcmd.c    | 13 +++++--------
 gdb/infrun.c    | 52 +++++++++++++++++++++++++---------------------------
 gdb/thread.c    | 10 ----------
 4 files changed, 57 insertions(+), 49 deletions(-)

-- 
2.14.3

Comments

Simon Marchi March 30, 2018, 5:58 a.m. | #1
On 2018-03-28 10:44, Pedro Alves wrote:
> A small patch I was sitting on in the multi-target branch.

> 

> gdb/ChangeLog:

> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

> 

> 	* gdbthread.h (finish_thread_state_cleanup): Delete declaration.

> 	(scoped_finish_thread_state): New class.

> 	* infcmd.c (run_command_1): Use it instead of finish_thread_state

> 	cleanup.

> 	* infrun.c (proceed, prepare_for_detach, wait_for_inferior)

> 	(fetch_inferior_event, normal_stop): Likewise.

> 	* thread.c (finish_thread_state_cleanup): Delete.


LGTM, two tiny nits:

> @@ -655,11 +653,10 @@ run_command_1 (const char *args, int from_tty,

> enum run_how run_how)

>       events --- the frontend shouldn't see them as stopped.  In

>       all-stop, always finish the state of all threads, as we may be

>       resuming more than just the new process.  */

> -  if (non_stop)

> -    ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));

> -  else

> -    ptid = minus_one_ptid;

> -  old_chain = make_cleanup (finish_thread_state_cleanup, &ptid);

> +  ptid_t finish_ptid = (non_stop

> +			? pid_to_ptid (current_inferior ()->pid)


Nit: you could use the ptid_t constructor (I saw you used it somewhere 
else in the patch).


> @@ -8164,7 +8159,6 @@ normal_stop (void)

>  {

>    struct target_waitstatus last;

>    ptid_t last_ptid;

> -  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);

>    ptid_t pid_ptid;


pid_ptid is now unused.

Simon
Pedro Alves April 10, 2018, 1:54 p.m. | #2
On 03/30/2018 06:58 AM, Simon Marchi wrote:
> 

> LGTM, two tiny nits:


> Nit: you could use the ptid_t constructor (I saw you used it somewhere else in the patch).


> pid_ptid is now unused.


Thanks, I've pushed it in with those fixed.

Pedro Alves

Patch

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 9b468dbda7..09ea5baf23 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -567,10 +567,33 @@  extern int threads_are_executing (void);
    Notifications are only emitted if the thread state did change.  */
 extern void finish_thread_state (ptid_t ptid);
 
-/* Same as FINISH_THREAD_STATE, but with an interface suitable to be
-   registered as a cleanup.  PTID_P points to the ptid_t that is
-   passed to FINISH_THREAD_STATE.  */
-extern void finish_thread_state_cleanup (void *ptid_p);
+/* Calls finish_thread_state on scope exit, unless release() is called
+   to disengage.  */
+class scoped_finish_thread_state
+{
+public:
+  explicit scoped_finish_thread_state (ptid_t ptid)
+    : m_ptid (ptid)
+  {}
+
+  ~scoped_finish_thread_state ()
+  {
+    if (!m_released)
+      finish_thread_state (m_ptid);
+  }
+
+  /* Disengage.  */
+  void release ()
+  {
+    m_released = true;
+  }
+
+  DISABLE_COPY_AND_ASSIGN (scoped_finish_thread_state);
+
+private:
+  bool m_released = false;
+  ptid_t m_ptid;
+};
 
 /* Commands with a prefix of `thread'.  */
 extern struct cmd_list_element *thread_cmd_list;
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9c236b8e70..bacc10144b 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -565,8 +565,6 @@  static void
 run_command_1 (const char *args, int from_tty, enum run_how run_how)
 {
   const char *exec_file;
-  struct cleanup *old_chain;
-  ptid_t ptid;
   struct ui_out *uiout = current_uiout;
   struct target_ops *run_target;
   int async_exec;
@@ -655,11 +653,10 @@  run_command_1 (const char *args, int from_tty, enum run_how run_how)
      events --- the frontend shouldn't see them as stopped.  In
      all-stop, always finish the state of all threads, as we may be
      resuming more than just the new process.  */
-  if (non_stop)
-    ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
-  else
-    ptid = minus_one_ptid;
-  old_chain = make_cleanup (finish_thread_state_cleanup, &ptid);
+  ptid_t finish_ptid = (non_stop
+			? pid_to_ptid (current_inferior ()->pid)
+			: minus_one_ptid);
+  scoped_finish_thread_state finish_state (finish_ptid);
 
   /* Pass zero for FROM_TTY, because at this point the "run" command
      has done its thing; now we are setting up the running program.  */
@@ -680,7 +677,7 @@  run_command_1 (const char *args, int from_tty, enum run_how run_how)
 
   /* Since there was no error, there's no need to finish the thread
      states here.  */
-  discard_cleanups (old_chain);
+  finish_state.release ();
 }
 
 static void
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6648698df6..fc0aac0f82 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2979,7 +2979,6 @@  proceed (CORE_ADDR addr, enum gdb_signal siggnal)
   ptid_t resume_ptid;
   struct execution_control_state ecss;
   struct execution_control_state *ecs = &ecss;
-  struct cleanup *old_chain;
   int started;
 
   /* If we're stopped at a fork/vfork, follow the branch set by the
@@ -3043,7 +3042,7 @@  proceed (CORE_ADDR addr, enum gdb_signal siggnal)
   /* If an exception is thrown from this point on, make sure to
      propagate GDB's knowledge of the executing state to the
      frontend/user running state.  */
-  old_chain = make_cleanup (finish_thread_state_cleanup, &resume_ptid);
+  scoped_finish_thread_state finish_state (resume_ptid);
 
   /* Even if RESUME_PTID is a wildcard, and we end up resuming fewer
      threads (e.g., we might need to set threads stepping over
@@ -3198,7 +3197,7 @@  proceed (CORE_ADDR addr, enum gdb_signal siggnal)
 
   target_commit_resume ();
 
-  discard_cleanups (old_chain);
+  finish_state.release ();
 
   /* Tell the event loop to wait for it to stop.  If the target
      supports asynchronous execution, it'll do this from within
@@ -3641,7 +3640,6 @@  prepare_for_detach (void)
 
   while (!ptid_equal (displaced->step_ptid, null_ptid))
     {
-      struct cleanup *old_chain_2;
       struct execution_control_state ecss;
       struct execution_control_state *ecs;
 
@@ -3663,14 +3661,13 @@  prepare_for_detach (void)
       /* If an error happens while handling the event, propagate GDB's
 	 knowledge of the executing state to the frontend/user running
 	 state.  */
-      old_chain_2 = make_cleanup (finish_thread_state_cleanup,
-				  &minus_one_ptid);
+      scoped_finish_thread_state finish_state (minus_one_ptid);
 
       /* Now figure out what to do with the result of the result.  */
       handle_inferior_event (ecs);
 
       /* No error, don't finish the state yet.  */
-      discard_cleanups (old_chain_2);
+      finish_state.release ();
 
       /* Breakpoints and watchpoints are not installed on the target
 	 at this point, and signals are passed directly to the
@@ -3696,7 +3693,6 @@  void
 wait_for_inferior (void)
 {
   struct cleanup *old_cleanups;
-  struct cleanup *thread_state_chain;
 
   if (debug_infrun)
     fprintf_unfiltered
@@ -3709,7 +3705,7 @@  wait_for_inferior (void)
   /* If an error happens while handling the event, propagate GDB's
      knowledge of the executing state to the frontend/user running
      state.  */
-  thread_state_chain = make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
+  scoped_finish_thread_state finish_state (minus_one_ptid);
 
   while (1)
     {
@@ -3740,7 +3736,7 @@  wait_for_inferior (void)
     }
 
   /* No error, don't finish the state yet.  */
-  discard_cleanups (thread_state_chain);
+  finish_state.release ();
 
   do_cleanups (old_cleanups);
 }
@@ -3859,7 +3855,6 @@  fetch_inferior_event (void *client_data)
   struct execution_control_state ecss;
   struct execution_control_state *ecs = &ecss;
   struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
-  struct cleanup *ts_old_chain;
   int cmd_done = 0;
   ptid_t waiton_ptid = minus_one_ptid;
 
@@ -3911,14 +3906,12 @@  fetch_inferior_event (void *client_data)
   /* If an error happens while handling the event, propagate GDB's
      knowledge of the executing state to the frontend/user running
      state.  */
-  if (!target_is_non_stop_p ())
-    ts_old_chain = make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
-  else
-    ts_old_chain = make_cleanup (finish_thread_state_cleanup, &ecs->ptid);
+  ptid_t finish_ptid = !target_is_non_stop_p () ? minus_one_ptid : ecs->ptid;
+  scoped_finish_thread_state finish_state (finish_ptid);
 
   /* Get executed before make_cleanup_restore_current_thread above to apply
      still for the thread which has thrown the exception.  */
-  make_bpstat_clear_actions_cleanup ();
+  struct cleanup *ts_old_chain = make_bpstat_clear_actions_cleanup ();
 
   make_cleanup (delete_just_stopped_threads_infrun_breakpoints_cleanup, NULL);
 
@@ -3973,9 +3966,11 @@  fetch_inferior_event (void *client_data)
 	}
     }
 
-  /* No error, don't finish the thread states yet.  */
   discard_cleanups (ts_old_chain);
 
+  /* No error, don't finish the thread states yet.  */
+  finish_state.release ();
+
   /* Revert thread and frame.  */
   do_cleanups (old_chain);
 
@@ -8164,7 +8159,6 @@  normal_stop (void)
 {
   struct target_waitstatus last;
   ptid_t last_ptid;
-  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   ptid_t pid_ptid;
 
   get_last_target_status (&last_ptid, &last);
@@ -8175,8 +8169,11 @@  normal_stop (void)
      propagate GDB's knowledge of the executing state to the
      frontend/user running state.  A QUIT is an easy exception to see
      here, so do this before any filtered output.  */
+
+  ptid_t finish_ptid = null_ptid;
+
   if (!non_stop)
-    make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
+    finish_ptid = minus_one_ptid;
   else if (last.kind == TARGET_WAITKIND_SIGNALLED
 	   || last.kind == TARGET_WAITKIND_EXITED)
     {
@@ -8185,14 +8182,15 @@  normal_stop (void)
 	 "checkpoint", when the current checkpoint/fork exits,
 	 linux-fork.c automatically switches to another fork from
 	 within target_mourn_inferior.  */
-      if (!ptid_equal (inferior_ptid, null_ptid))
-	{
-	  pid_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
-	  make_cleanup (finish_thread_state_cleanup, &pid_ptid);
-	}
+      if (inferior_ptid != null_ptid)
+	finish_ptid = ptid_t (inferior_ptid.pid ());
     }
   else if (last.kind != TARGET_WAITKIND_NO_RESUMED)
-    make_cleanup (finish_thread_state_cleanup, &inferior_ptid);
+    finish_ptid = inferior_ptid;
+
+  gdb::optional<scoped_finish_thread_state> maybe_finish_thread_state;
+  if (finish_ptid != null_ptid)
+    maybe_finish_thread_state.emplace (finish_ptid);
 
   /* As we're presenting a stop, and potentially removing breakpoints,
      update the thread list so we can tell whether there are threads
@@ -8224,7 +8222,7 @@  normal_stop (void)
      after this event is handled, so we're not really switching, only
      informing of a stop.  */
   if (!non_stop
-      && !ptid_equal (previous_inferior_ptid, inferior_ptid)
+      && previous_inferior_ptid != inferior_ptid
       && target_has_execution
       && last.kind != TARGET_WAITKIND_SIGNALLED
       && last.kind != TARGET_WAITKIND_EXITED
@@ -8265,7 +8263,7 @@  normal_stop (void)
     }
 
   /* Let the user/frontend see the threads as stopped.  */
-  do_cleanups (old_chain);
+  maybe_finish_thread_state.reset ();
 
   /* Select innermost stack frame - i.e., current frame is frame 0,
      and current location is based on that.  Handle the case where the
diff --git a/gdb/thread.c b/gdb/thread.c
index 99f3f5be63..c1a8174e96 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1040,16 +1040,6 @@  finish_thread_state (ptid_t ptid)
     gdb::observers::target_resumed.notify (ptid);
 }
 
-void
-finish_thread_state_cleanup (void *arg)
-{
-  ptid_t *ptid_p = (ptid_t *) arg;
-
-  gdb_assert (arg);
-
-  finish_thread_state (*ptid_p);
-}
-
 /* See gdbthread.h.  */
 
 void