Add enum for result of fast_tracepoint_collecting

Message ID 1501015878-5152-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi July 25, 2017, 8:51 p.m. UTC
  I got confused by the result value of fast_tracepoint_collecting, while
it sounds like it would return true/false (whether the thread is
collecting or not), it actually returns:

  0: not collecting
  1: in the jump pad, before the relocated instruction
  2: in the jump pad, at or after the relocated instruction

To avoid confusion, I think it would be nice to make it return an enum.
If you can help find a shorter but still relavant name, it would be
awesome.  Otherwise, we'll go with that, fast_tpoint_collect_result,
which is at least consistent with the existing
fast_tpoint_collect_status.

gdb/gdbserver/ChangeLog:

	* tracepoint.h (enum class fast_tpoint_collect_result): New
	enumeration.
	(fast_tracepoint_collecting): Change return type to
	fast_tpoint_collect_result.
	* tracepoint.c (fast_tracepoint_collecting): Likewise.
	* linux-low.h: Include tracepoint.h.
	(struct lwp_info) <collecting_fast_tracepoint>: Change type to
	fast_tpoint_collect_result.
	* linux-low.c (handle_tracepoints): Adjust.
	(linux_fast_tracepoint_collecting): Change return type to
	fast_tpoint_collect_result.
	(maybe_move_out_of_jump_pad, linux_wait_for_event_filtered,
	linux_wait_1, stuck_in_jump_pad_callback,
	lwp_signal_can_be_delivered, linux_resume_one_lwp_throw,
	proceed_one_lwp): Adjust to type change.
---
 gdb/gdbserver/linux-low.c  | 61 ++++++++++++++++++++++++++++------------------
 gdb/gdbserver/linux-low.h  |  3 ++-
 gdb/gdbserver/tracepoint.c | 18 +++++++-------
 gdb/gdbserver/tracepoint.h | 22 ++++++++++++++---
 4 files changed, 67 insertions(+), 37 deletions(-)
  

Comments

Yao Qi July 26, 2017, 7:42 a.m. UTC | #1
Simon Marchi <simon.marchi@ericsson.com> writes:

>  /* Convenience wrapper.  Returns true if LWP is presently collecting a
>     fast tracepoint.  */
>  

The comments should be updated too.

> -static int
> +static fast_tpoint_collect_result
>  linux_fast_tracepoint_collecting (struct lwp_info *lwp,
>  				  struct fast_tpoint_collect_status *status)
>  {


> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
> index 6328da0..18ca6a0 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -26,6 +26,7 @@
>  /* Included for ptrace type definitions.  */
>  #include "nat/linux-ptrace.h"
>  #include "target/waitstatus.h" /* For enum target_stop_reason.  */
> +#include "tracepoint.h"
>  
>  #define PTRACE_XFER_TYPE long
>  
> @@ -358,7 +359,7 @@ struct lwp_info
>       return to the jump pad.  Normally, we won't care about this, but
>       we will if a signal arrives to this lwp while it is
>       collecting.  */
> -  int collecting_fast_tracepoint;
> +  fast_tpoint_collect_result collecting_fast_tracepoint;

The comments should be updated too.

>  
>    /* If this is non-zero, it points to a chain of signals which need
>       to be reported to GDB.  These were deferred because the thread

Otherwise, the patch is good to me.
  

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 3d7cfe3..3da60ea 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -2082,7 +2082,9 @@  handle_tracepoints (struct lwp_info *lwp)
   lwp_suspended_decr (lwp);
 
   gdb_assert (lwp->suspended == 0);
-  gdb_assert (!stabilizing_threads || lwp->collecting_fast_tracepoint);
+  gdb_assert (!stabilizing_threads
+	      || (lwp->collecting_fast_tracepoint
+		  != fast_tpoint_collect_result::not_collecting));
 
   if (tpoint_related_event)
     {
@@ -2097,7 +2099,7 @@  handle_tracepoints (struct lwp_info *lwp)
 /* Convenience wrapper.  Returns true if LWP is presently collecting a
    fast tracepoint.  */
 
-static int
+static fast_tpoint_collect_result
 linux_fast_tracepoint_collecting (struct lwp_info *lwp,
 				  struct fast_tpoint_collect_status *status)
 {
@@ -2105,14 +2107,14 @@  linux_fast_tracepoint_collecting (struct lwp_info *lwp,
   struct thread_info *thread = get_lwp_thread (lwp);
 
   if (the_low_target.get_thread_area == NULL)
-    return 0;
+    return fast_tpoint_collect_result::not_collecting;
 
   /* Get the thread area address.  This is used to recognize which
      thread is which when tracing with the in-process agent library.
      We don't read anything from the address, and treat it as opaque;
      it's the address itself that we assume is unique per-thread.  */
   if ((*the_low_target.get_thread_area) (lwpid_of (thread), &thread_area) == -1)
-    return 0;
+    return fast_tpoint_collect_result::not_collecting;
 
   return fast_tracepoint_collecting (thread_area, lwp->stop_pc, status);
 }
@@ -2136,14 +2138,14 @@  maybe_move_out_of_jump_pad (struct lwp_info *lwp, int *wstat)
       && agent_loaded_p ())
     {
       struct fast_tpoint_collect_status status;
-      int r;
 
       if (debug_threads)
 	debug_printf ("Checking whether LWP %ld needs to move out of the "
 		      "jump pad.\n",
 		      lwpid_of (current_thread));
 
-      r = linux_fast_tracepoint_collecting (lwp, &status);
+      fast_tpoint_collect_result r
+	= linux_fast_tracepoint_collecting (lwp, &status);
 
       if (wstat == NULL
 	  || (WSTOPSIG (*wstat) != SIGILL
@@ -2153,9 +2155,10 @@  maybe_move_out_of_jump_pad (struct lwp_info *lwp, int *wstat)
 	{
 	  lwp->collecting_fast_tracepoint = r;
 
-	  if (r != 0)
+	  if (r != fast_tpoint_collect_result::not_collecting)
 	    {
-	      if (r == 1 && lwp->exit_jump_pad_bkpt == NULL)
+	      if (r == fast_tpoint_collect_result::before_insn
+		  && lwp->exit_jump_pad_bkpt == NULL)
 		{
 		  /* Haven't executed the original instruction yet.
 		     Set breakpoint there, and wait till it's hit,
@@ -2181,9 +2184,10 @@  maybe_move_out_of_jump_pad (struct lwp_info *lwp, int *wstat)
 	     reporting to GDB.  Otherwise, it's an IPA lib bug: just
 	     report the signal to GDB, and pray for the best.  */
 
-	  lwp->collecting_fast_tracepoint = 0;
+	  lwp->collecting_fast_tracepoint
+	    = fast_tpoint_collect_result::not_collecting;
 
-	  if (r != 0
+	  if (r != fast_tpoint_collect_result::not_collecting
 	      && (status.adjusted_insn_addr <= lwp->stop_pc
 		  && lwp->stop_pc < status.adjusted_insn_addr_end))
 	    {
@@ -2715,7 +2719,8 @@  linux_wait_for_event_filtered (ptid_t wait_ptid, ptid_t filter_ptid,
 
       if (stopping_threads == NOT_STOPPING_THREADS
 	  && requested_child->status_pending_p
-	  && requested_child->collecting_fast_tracepoint)
+	  && (requested_child->collecting_fast_tracepoint
+	      != fast_tpoint_collect_result::not_collecting))
 	{
 	  enqueue_one_deferred_signal (requested_child,
 				       &requested_child->status_pending);
@@ -3467,20 +3472,22 @@  linux_wait_1 (ptid_t ptid,
 	}
     }
 
-  if (event_child->collecting_fast_tracepoint)
+  if (event_child->collecting_fast_tracepoint
+      != fast_tpoint_collect_result::not_collecting)
     {
       if (debug_threads)
 	debug_printf ("LWP %ld was trying to move out of the jump pad (%d). "
 		      "Check if we're already there.\n",
 		      lwpid_of (current_thread),
-		      event_child->collecting_fast_tracepoint);
+		      (int) event_child->collecting_fast_tracepoint);
 
       trace_event = 1;
 
       event_child->collecting_fast_tracepoint
 	= linux_fast_tracepoint_collecting (event_child, NULL);
 
-      if (event_child->collecting_fast_tracepoint != 1)
+      if (event_child->collecting_fast_tracepoint
+	  != fast_tpoint_collect_result::before_insn)
 	{
 	  /* No longer need this breakpoint.  */
 	  if (event_child->exit_jump_pad_bkpt != NULL)
@@ -3507,7 +3514,8 @@  linux_wait_1 (ptid_t ptid,
 	    }
 	}
 
-      if (event_child->collecting_fast_tracepoint == 0)
+      if (event_child->collecting_fast_tracepoint
+	  == fast_tpoint_collect_result::not_collecting)
 	{
 	  if (debug_threads)
 	    debug_printf ("fast tracepoint finished "
@@ -4192,7 +4200,8 @@  stuck_in_jump_pad_callback (struct inferior_list_entry *entry, void *data)
 	  && (gdb_breakpoint_here (lwp->stop_pc)
 	      || lwp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT
 	      || thread->last_resume_kind == resume_step)
-	  && linux_fast_tracepoint_collecting (lwp, NULL));
+	  && (linux_fast_tracepoint_collecting (lwp, NULL)
+	      != fast_tpoint_collect_result::not_collecting));
 }
 
 static void
@@ -4369,7 +4378,8 @@  single_step (struct lwp_info* lwp)
 static int
 lwp_signal_can_be_delivered (struct lwp_info *lwp)
 {
-  return !lwp->collecting_fast_tracepoint;
+  return (lwp->collecting_fast_tracepoint
+	  == fast_tpoint_collect_result::not_collecting);
 }
 
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
@@ -4381,7 +4391,6 @@  linux_resume_one_lwp_throw (struct lwp_info *lwp,
 {
   struct thread_info *thread = get_lwp_thread (lwp);
   struct thread_info *saved_thread;
-  int fast_tp_collecting;
   int ptrace_request;
   struct process_info *proc = get_thread_process (thread);
 
@@ -4397,9 +4406,12 @@  linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
   gdb_assert (lwp->waitstatus.kind == TARGET_WAITKIND_IGNORE);
 
-  fast_tp_collecting = lwp->collecting_fast_tracepoint;
+  fast_tpoint_collect_result fast_tp_collecting
+    = lwp->collecting_fast_tracepoint;
 
-  gdb_assert (!stabilizing_threads || fast_tp_collecting);
+  gdb_assert (!stabilizing_threads
+	      || (fast_tp_collecting
+		  != fast_tpoint_collect_result::not_collecting));
 
   /* Cancel actions that rely on GDB not changing the PC (e.g., the
      user used the "jump" command, or "set $pc = foo").  */
@@ -4455,7 +4467,7 @@  linux_resume_one_lwp_throw (struct lwp_info *lwp,
 
       if (can_hardware_single_step ())
 	{
-	  if (fast_tp_collecting == 0)
+	  if (fast_tp_collecting == fast_tpoint_collect_result::not_collecting)
 	    {
 	      if (step == 0)
 		warning ("BAD - reinserting but not stepping.");
@@ -4468,14 +4480,14 @@  linux_resume_one_lwp_throw (struct lwp_info *lwp,
       step = maybe_hw_step (thread);
     }
 
-  if (fast_tp_collecting == 1)
+  if (fast_tp_collecting == fast_tpoint_collect_result::before_insn)
     {
       if (debug_threads)
 	debug_printf ("lwp %ld wants to get out of fast tracepoint jump pad"
 		      " (exit-jump-pad-bkpt)\n",
 		      lwpid_of (thread));
     }
-  else if (fast_tp_collecting == 2)
+  else if (fast_tp_collecting == fast_tpoint_collect_result::at_insn)
     {
       if (debug_threads)
 	debug_printf ("lwp %ld wants to get out of fast tracepoint jump pad"
@@ -5294,7 +5306,8 @@  proceed_one_lwp (struct inferior_list_entry *entry, void *except)
 
   if (thread->last_resume_kind == resume_stop
       && lwp->pending_signals_to_report == NULL
-      && lwp->collecting_fast_tracepoint == 0)
+      && (lwp->collecting_fast_tracepoint
+	  == fast_tpoint_collect_result::not_collecting))
     {
       /* We haven't reported this LWP as stopped yet (otherwise, the
 	 last_status.kind check above would catch it, and we wouldn't
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 6328da0..18ca6a0 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -26,6 +26,7 @@ 
 /* Included for ptrace type definitions.  */
 #include "nat/linux-ptrace.h"
 #include "target/waitstatus.h" /* For enum target_stop_reason.  */
+#include "tracepoint.h"
 
 #define PTRACE_XFER_TYPE long
 
@@ -358,7 +359,7 @@  struct lwp_info
      return to the jump pad.  Normally, we won't care about this, but
      we will if a signal arrives to this lwp while it is
      collecting.  */
-  int collecting_fast_tracepoint;
+  fast_tpoint_collect_result collecting_fast_tracepoint;
 
   /* If this is non-zero, it points to a chain of signals which need
      to be reported to GDB.  These were deferred because the thread
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index d4fe76a..68ce10f 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -5581,7 +5581,7 @@  force_unlock_trace_buffer (void)
    case, if we want to move the thread out of the jump pad, we need to
    single-step it until this function returns 0.  */
 
-int
+fast_tpoint_collect_result
 fast_tracepoint_collecting (CORE_ADDR thread_area,
 			    CORE_ADDR stop_pc,
 			    struct fast_tpoint_collect_status *status)
@@ -5656,7 +5656,7 @@  fast_tracepoint_collecting (CORE_ADDR thread_area,
       if (tpoint == NULL)
 	{
 	  warning ("in jump pad, but no matching tpoint?");
-	  return 0;
+	  return fast_tpoint_collect_result::not_collecting;
 	}
       else
 	{
@@ -5684,7 +5684,7 @@  fast_tracepoint_collecting (CORE_ADDR thread_area,
       if (tpoint == NULL)
 	{
 	  warning ("in trampoline, but no matching tpoint?");
-	  return 0;
+	  return fast_tpoint_collect_result::not_collecting;
 	}
       else
 	{
@@ -5712,14 +5712,14 @@  fast_tracepoint_collecting (CORE_ADDR thread_area,
 	{
 	  trace_debug ("fast_tracepoint_collecting:"
 		       " failed reading 'collecting' in the inferior");
-	  return 0;
+	  return fast_tpoint_collect_result::not_collecting;
 	}
 
       if (!ipa_collecting)
 	{
 	  trace_debug ("fast_tracepoint_collecting: not collecting"
 		       " (and nobody is).");
-	  return 0;
+	  return fast_tpoint_collect_result::not_collecting;
 	}
 
       /* Some thread is collecting.  Check which.  */
@@ -5732,7 +5732,7 @@  fast_tracepoint_collecting (CORE_ADDR thread_area,
 	{
 	  trace_debug ("fast_tracepoint_collecting: not collecting "
 		       "(another thread is)");
-	  return 0;
+	  return fast_tpoint_collect_result::not_collecting;
 	}
 
       tpoint
@@ -5742,7 +5742,7 @@  fast_tracepoint_collecting (CORE_ADDR thread_area,
 	  warning ("fast_tracepoint_collecting: collecting, "
 		   "but tpoint %s not found?",
 		   paddress ((CORE_ADDR) ipa_collecting_obj.tpoint));
-	  return 0;
+	  return fast_tpoint_collect_result::not_collecting;
 	}
 
       /* The thread is within `gdb_collect', skip over the rest of
@@ -5769,7 +5769,7 @@  fast_tracepoint_collecting (CORE_ADDR thread_area,
 fast_tracepoint_collecting, returning continue-until-break at %s",
 		   paddress (tpoint->adjusted_insn_addr));
 
-      return 1; /* continue */
+      return fast_tpoint_collect_result::before_insn; /* continue */
     }
   else
     {
@@ -5780,7 +5780,7 @@  fast_tracepoint_collecting, returning continue-until-break at %s",
 		   paddress (tpoint->adjusted_insn_addr),
 		   paddress (tpoint->adjusted_insn_addr_end));
 
-      return 2; /* single-step */
+      return fast_tpoint_collect_result::at_insn; /* single-step */
     }
 }
 
diff --git a/gdb/gdbserver/tracepoint.h b/gdb/gdbserver/tracepoint.h
index 799a16c..31103a3 100644
--- a/gdb/gdbserver/tracepoint.h
+++ b/gdb/gdbserver/tracepoint.h
@@ -115,9 +115,25 @@  struct fast_tpoint_collect_status
   CORE_ADDR adjusted_insn_addr_end;
 };
 
-int fast_tracepoint_collecting (CORE_ADDR thread_area,
-				CORE_ADDR stop_pc,
-				struct fast_tpoint_collect_status *status);
+/* The possible states a thread can be in, related to the collection of fast
+   tracepoint.  */
+
+enum class fast_tpoint_collect_result
+{
+  /* Not collecting a fast tracepoint.  */
+  not_collecting,
+
+  /* In the jump pad, but before the relocated instruction.  */
+  before_insn,
+
+  /* In the jump pad, but at (or after) the relocated instruction.  */
+  at_insn,
+};
+
+fast_tpoint_collect_result fast_tracepoint_collecting
+  (CORE_ADDR thread_area, CORE_ADDR stop_pc,
+   struct fast_tpoint_collect_status *status);
+
 void force_unlock_trace_buffer (void);
 
 int handle_tracepoint_bkpts (struct thread_info *tinfo, CORE_ADDR stop_pc);