Re: [PATCH] Add enum for result of fast_tracepoint_collecting

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2017-07-26 09:42 AM, Yao Qi wrote:
> 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.

Yeah of course.

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

Indeed.

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

Thanks, here is what I pushed:


>From 229d26fc9ebca61b8d899cf8fe4342a6cc9795ff Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Wed, 26 Jul 2017 10:55:35 +0200
Subject: [PATCH] Add enum for result of fast_tracepoint_collecting

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/ChangeLog    | 18 +++++++++++++
 gdb/gdbserver/linux-low.c  | 65 +++++++++++++++++++++++++++-------------------
 gdb/gdbserver/linux-low.h  | 12 ++++-----
 gdb/gdbserver/tracepoint.c | 18 ++++++-------
 gdb/gdbserver/tracepoint.h | 22 +++++++++++++---
 5 files changed, 91 insertions(+), 44 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index c505ccf..fdc07f2 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,21 @@
+2017-07-26  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* 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.
+
 2017-07-10  Yao Qi  <yao.qi@linaro.org>

 	* linux-x86-low.c (x86_linux_read_description): Re-indent the code.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 3d7cfe3..fd46d85 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)
     {
@@ -2094,10 +2096,10 @@ handle_tracepoints (struct lwp_info *lwp)
   return 0;
 }

-/* Convenience wrapper.  Returns true if LWP is presently collecting a
-   fast tracepoint.  */
+/* Convenience wrapper.  Returns information about LWP's fast tracepoint
+   collection status.  */

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

@@ -353,12 +354,11 @@ struct lwp_info
      and then processed and cleared in linux_resume_one_lwp.  */
   struct thread_resume *resume;

-  /* True if it is known that this lwp is presently collecting a fast
-     tracepoint (it is in the jump pad or in some code that will
-     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;
+  /* Information bout this lwp's fast tracepoint collection status (is it
+     currently stopped in the jump pad, and if so, before or at/after the
+     relocated instruction).  Normally, we won't care about this, but we will
+     if a signal arrives to this lwp while it is collecting.  */
+  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);
-- 
2.7.4