[patch 0/2] OO tracepoint_action

[patch 0/2] OO tracepoint_action

[Yao Qi]

Hi,
When modifying GDBserver to send various types of tracepoint_actions to
agent, I noticed there has been three places using "switch/case" blocks for
different types of tracepoint_action, and I don't like to add one more.  It
should be cleaner to rewrite them in an OO way.  So these patches come up.

[PATCH 2/2] OO tracepoint_action

[Yao Qi]

This is the major part of this patch series, which replace several
switch/case blocks in code with function vectors.  It is easier to add
new operation to tracepoint_action in the future.

gdb/gdbserver:

	* tracepoint.c (struct tracepoint_action_ops): New.
	(struct tracepoint_action) <ops>: New field.
	(record_tracepoint_error): Removed.
	(record_expr_eval_error): New.
	(add_tracepoint_action): Move code to ...
	(m_tracepoint_action_init, r_tracepoint_action_init):
	(x_tracepoint_action_init, l_tracepoint_action_init: ... here.
	New.
	(download_tracepoint_1): Move code to ...
	(m_tracepoint_action_download, r_tracepoint_action_download):
	(x_tracepoint_action_download, l_tracepoint_action_download):
	.. .here.  New.
	(do_action_at_tracepoint): Move code to ...
	(m_tracepoint_action_execute, r_tracepoint_action_execute):
	(x_tracepoint_action_execute, l_tracepoint_action_execute):
	... here.  New.
	(condition_true_at_tracepoint): Call record_expr_eval_error
	instead.
---
 gdb/gdbserver/tracepoint.c |  560 ++++++++++++++++++++++++++++----------------
 1 files changed, 358 insertions(+), 202 deletions(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 5dcc7a4..bc23825 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -469,12 +469,38 @@ write_inferior_uinteger (CORE_ADDR symaddr, unsigned int val)
   return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));
 }
 
+static CORE_ADDR target_malloc (ULONGEST size);
+static int write_inferior_data_ptr (CORE_ADDR where, CORE_ADDR ptr);
 #endif
 
+/* Operations on various types of tracepoint actions.  */
+
+struct tracepoint_action;
+struct tracepoint_hit_ctx;
+
+struct tracepoint_action_ops
+{
+#ifndef IN_PROCESS_AGENT
+  /* Parse rsp packet PACKET and initialize ACTION.  Return the updated
+     pointer to rsp packet..  */
+  char* (*init) (char *packet,  struct tracepoint_action* action);
+
+  /* Download tracepoint action ACTION to IPA.  Return the address of action
+     in IPA/inferior.  */
+  CORE_ADDR (*download) (struct tracepoint_action *action);
+#endif
+
+  /* Do tracepoint action ACTION.  Return 0 if success otherwise return
+     non-zero.   */
+  int (*execute) (struct tracepoint_action *action,
+		  struct tracepoint_hit_ctx *ctx, struct traceframe *tframe);
+};
+
 /* Base action.  Concrete actions inherit this.  */
 
 struct tracepoint_action
 {
+  struct tracepoint_action_ops *ops;
   char type;
 };
 
@@ -1247,12 +1273,8 @@ static LONGEST get_timestamp (void);
 /* Record that an error occurred during expression evaluation.  */
 
 static void
-record_tracepoint_error (struct tracepoint *tpoint, const char *which,
-			 enum eval_result_type rtype)
+record_expr_eval_error (enum eval_result_type rtype)
 {
-  trace_debug ("Tracepoint %d at %s %s eval reports error %d",
-	       tpoint->number, paddress (tpoint->address), which, rtype);
-
 #ifdef IN_PROCESS_AGENT
   /* Only record the first error we get.  */
   if (cmpxchg (&expr_eval_result,
@@ -1263,8 +1285,6 @@ record_tracepoint_error (struct tracepoint *tpoint, const char *which,
   if (expr_eval_result != expr_eval_no_error)
     return;
 #endif
-
-  error_tracepoint = tpoint;
 }
 
 /* Trace buffer management.  */
@@ -1603,6 +1623,298 @@ trace_buffer_alloc (size_t amt)
 }
 
 #ifndef IN_PROCESS_AGENT
+static char*
+m_tracepoint_action_init (char *packet, struct tracepoint_action* action)
+{
+  ULONGEST basereg;
+  int is_neg;
+  struct collect_memory_action *maction
+    = (struct collect_memory_action *) action;
+
+  maction->base.type = *packet;
+  ++packet;
+
+  is_neg = (*packet == '-');
+  if (*packet == '-')
+    ++packet;
+  packet = unpack_varlen_hex (packet, &basereg);
+  ++packet;
+  packet = unpack_varlen_hex (packet, &maction->addr);
+  ++packet;
+  packet = unpack_varlen_hex (packet, &maction->len);
+  maction->basereg = (is_neg ? - (int) basereg : (int) basereg);
+  trace_debug ("Want to collect %s bytes at 0x%s (basereg %d)",
+	       pulongest (maction->len),
+	       paddress (maction->addr), maction->basereg);
+
+  return packet;
+}
+
+static CORE_ADDR
+m_tracepoint_action_download (struct tracepoint_action *action)
+{
+  CORE_ADDR ipa_action = target_malloc (sizeof (struct collect_memory_action));
+
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct collect_memory_action));
+  /* Write NULL to field `ops'.  */
+  write_inferior_data_ptr(ipa_action + offsetof (struct tracepoint_action, ops),
+			  0);
+
+  return ipa_action;
+}
+#endif /* !IN_PROCESS_AGENT */
+
+static int
+m_tracepoint_action_execute (struct tracepoint_action *action,
+			     struct tracepoint_hit_ctx *ctx,
+			     struct traceframe *tframe)
+{
+  struct collect_memory_action *maction;
+
+  maction = (struct collect_memory_action *) action;
+
+  trace_debug ("Want to collect %s bytes at 0x%s (basereg %d)",
+	       pulongest (maction->len), paddress (maction->addr),
+	       maction->basereg);
+  /* (should use basereg) */
+  agent_mem_read (tframe, NULL,
+		  (CORE_ADDR) maction->addr, maction->len);
+
+  return 0;
+}
+
+static struct tracepoint_action_ops m_tracepoint_action_ops =
+{
+#ifndef IN_PROCESS_AGENT
+  m_tracepoint_action_init,
+  m_tracepoint_action_download,
+#endif
+  m_tracepoint_action_execute,
+};
+
+#ifndef IN_PROCESS_AGENT
+static char*
+r_tracepoint_action_init (char * packet, struct tracepoint_action* action)
+{
+  action->type = *packet;
+  ++packet;
+
+  trace_debug ("Want to collect registers");
+
+  /* skip past hex digits of mask for now */
+  while (isxdigit(*packet))
+    ++packet;
+
+  return packet;
+}
+
+static CORE_ADDR
+r_tracepoint_action_download (struct tracepoint_action *action)
+{
+  CORE_ADDR ipa_action
+    = target_malloc (sizeof (struct collect_registers_action));
+
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct collect_registers_action));
+  /* Write NULL to field `ops'.  */
+  write_inferior_data_ptr(ipa_action + offsetof (struct tracepoint_action, ops),
+			  0);
+
+  return ipa_action;
+}
+#endif /* !IN_PROCESS_AGENT */
+
+static unsigned char* add_traceframe_block (struct traceframe *tframe, int amt);
+static struct regcache* get_context_regcache (struct tracepoint_hit_ctx *ctx);
+
+static int
+r_tracepoint_action_execute (struct tracepoint_action *action,
+			     struct tracepoint_hit_ctx *ctx,
+			     struct traceframe *tframe)
+{
+  unsigned char *regspace;
+  struct regcache tregcache;
+  struct regcache *context_regcache;
+
+  trace_debug ("Want to collect registers");
+
+  /* Collect all registers for now.  */
+  regspace = add_traceframe_block (tframe, 1 + register_cache_size ());
+  if (regspace == NULL)
+    {
+      trace_debug ("Trace buffer block allocation failed, skipping");
+      return 1;
+    }
+  /* Identify a register block.  */
+  *regspace = 'R';
+
+  context_regcache = get_context_regcache (ctx);
+
+  /* Wrap the regblock in a register cache (in the stack, we don't want
+     to malloc here).  */
+  init_register_cache (&tregcache, regspace + 1);
+
+  /* Copy the register data to the regblock.  */
+  regcache_cpy (&tregcache, context_regcache);
+
+#ifndef IN_PROCESS_AGENT
+  /* On some platforms, trap-based tracepoints will have the PC pointing
+     to the next instruction after the trap, but we don't want the user
+     or GDB trying to guess whether the saved PC needs adjusting; so
+     always record the adjusted stop_pc.  Note that we can't use
+     tpoint->address instead, since it will be wrong for while-stepping
+     actions.  This adjustment is a nop for fast tracepoints collected from
+     the in-process lib (but not if GDBserver is collecting one preemptively),
+     since the PC had already been adjusted to contain the tracepoint's
+     address by the jump pad.  */
+  trace_debug ("Storing stop pc (0x%s) in regblock",
+	       paddress (ctx->stop_pc));
+
+  /* This changes the regblock, not the thread's regcache.  */
+  regcache_write_pc (&tregcache, ctx->stop_pc);
+#endif
+
+  return 0;
+}
+
+static struct tracepoint_action_ops r_tracepoint_action_ops =
+{
+#ifndef IN_PROCESS_AGENT
+  r_tracepoint_action_init,
+  r_tracepoint_action_download,
+#endif
+  r_tracepoint_action_execute,
+};
+
+#ifndef IN_PROCESS_AGENT
+static char*
+x_tracepoint_action_init (char *packet, struct tracepoint_action* action)
+{
+  action->type = *packet;
+
+  trace_debug ("Want to evaluate expression");
+  ((struct eval_expr_action *) action)->expr = gdb_parse_agent_expr (&packet);
+
+  return packet;
+}
+
+static CORE_ADDR download_agent_expr (struct agent_expr *expr);
+
+static CORE_ADDR
+x_tracepoint_action_download (struct tracepoint_action *action)
+{
+  CORE_ADDR ipa_action = target_malloc (sizeof (struct eval_expr_action));
+  CORE_ADDR expr;
+
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct eval_expr_action));
+  expr = download_agent_expr (((struct eval_expr_action *)action)->expr);
+  write_inferior_data_ptr(ipa_action + offsetof (struct eval_expr_action, expr),
+			  expr);
+  /* Write NULL to field `ops'.  */
+  write_inferior_data_ptr(ipa_action + offsetof (struct tracepoint_action, ops),
+			  0);
+
+  return ipa_action;
+}
+#endif /* !IN_PROCESS_AGENT */
+
+static void record_expr_eval_error (enum eval_result_type rtype);
+
+static int
+x_tracepoint_action_execute (struct tracepoint_action *action,
+			     struct tracepoint_hit_ctx *ctx,
+			     struct traceframe *tframe)
+{
+  struct eval_expr_action *eaction = (struct eval_expr_action *) action;
+  enum eval_result_type err;
+
+  trace_debug ("Want to evaluate expression");
+
+  err = eval_tracepoint_agent_expr (ctx, tframe, eaction->expr, NULL);
+
+  if (err != expr_eval_no_error)
+    {
+      trace_debug ("Tracepoint at %s action expression eval reports error %d",
+		   paddress (ctx->stop_pc), err);
+
+      record_expr_eval_error (err);
+
+      return 1;
+    }
+
+  return 0;
+}
+
+static struct tracepoint_action_ops x_tracepoint_action_ops =
+{
+#ifndef IN_PROCESS_AGENT
+  x_tracepoint_action_init,
+  x_tracepoint_action_download,
+#endif
+  x_tracepoint_action_execute,
+};
+
+#ifndef IN_PROCESS_AGENT
+static char*
+l_tracepoint_action_init (char *packet, struct tracepoint_action* action)
+{
+  action->type = *packet;
+  packet++;
+
+  trace_debug ("Want to collect static trace data");
+
+  return packet;
+}
+
+static CORE_ADDR
+l_tracepoint_action_download (struct tracepoint_action *action)
+{
+  CORE_ADDR ipa_action
+    = target_malloc (sizeof (struct  collect_static_trace_data_action));
+
+  write_inferior_memory (ipa_action, (unsigned char *) action,
+			 sizeof (struct  collect_static_trace_data_action));
+  /* Write NULL to field `ops'.  */
+  write_inferior_data_ptr(ipa_action + offsetof (struct tracepoint_action, ops),
+			  0);
+
+  return ipa_action;
+}
+#endif /* !IN_PROCESS_AGENT */
+
+#if defined IN_PROCESS_AGENT && defined HAVE_UST
+static void collect_ust_data_at_tracepoint (struct tracepoint_hit_ctx *ctx,
+					    struct traceframe *tframe);
+#endif
+
+static int
+l_tracepoint_action_execute (struct tracepoint_action *action,
+			     struct tracepoint_hit_ctx *ctx,
+			     struct traceframe *tframe)
+{
+#if defined IN_PROCESS_AGENT && defined HAVE_UST
+  trace_debug ("Want to collect static trace data");
+  collect_ust_data_at_tracepoint (ctx, tframe);
+  return 0;
+#else
+  trace_debug ("warning: collecting static trace data, "
+	       "but static tracepoints are not supported");
+  return 1;
+#endif
+}
+
+static struct tracepoint_action_ops l_tracepoint_action_ops =
+{
+#ifndef IN_PROCESS_AGENT
+  l_tracepoint_action_init,
+  l_tracepoint_action_download,
+#endif
+  l_tracepoint_action_execute,
+};
+
+#ifndef IN_PROCESS_AGENT
 
 /* Return the total free space.  This is not necessarily the largest
    block we can allocate, because of the two-part case.  */
@@ -1755,56 +2067,23 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	{
 	case 'M':
 	  {
-	    struct collect_memory_action *maction;
-	    ULONGEST basereg;
-	    int is_neg;
-
-	    maction = xmalloc (sizeof *maction);
-	    maction->base.type = *act;
-	    action = &maction->base;
-
-	    ++act;
-	    is_neg = (*act == '-');
-	    if (*act == '-')
-	      ++act;
-	    act = unpack_varlen_hex (act, &basereg);
-	    ++act;
-	    act = unpack_varlen_hex (act, &maction->addr);
-	    ++act;
-	    act = unpack_varlen_hex (act, &maction->len);
-	    maction->basereg = (is_neg
-				? - (int) basereg
-				: (int) basereg);
-	    trace_debug ("Want to collect %s bytes at 0x%s (basereg %d)",
-			 pulongest (maction->len),
-			 paddress (maction->addr), maction->basereg);
+	    action = xmalloc (sizeof (struct collect_memory_action));
+	    action->ops = &m_tracepoint_action_ops;
+
 	    break;
 	  }
 	case 'R':
 	  {
-	    struct collect_registers_action *raction;
-
-	    raction = xmalloc (sizeof *raction);
-	    raction->base.type = *act;
-	    action = &raction->base;
+	    action = xmalloc (sizeof (struct collect_registers_action));
+	    action->ops = &r_tracepoint_action_ops;
 
-	    trace_debug ("Want to collect registers");
-	    ++act;
-	    /* skip past hex digits of mask for now */
-	    while (isxdigit(*act))
-	      ++act;
 	    break;
 	  }
 	case 'L':
 	  {
-	    struct collect_static_trace_data_action *raction;
+	    action = xmalloc (sizeof (struct collect_static_trace_data_action));
+	    action->ops = &l_tracepoint_action_ops;
 
-	    raction = xmalloc (sizeof *raction);
-	    raction->base.type = *act;
-	    action = &raction->base;
-
-	    trace_debug ("Want to collect static trace data");
-	    ++act;
 	    break;
 	  }
 	case 'S':
@@ -1813,14 +2092,8 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
 	  break;
 	case 'X':
 	  {
-	    struct eval_expr_action *xaction;
-
-	    xaction = xmalloc (sizeof (*xaction));
-	    xaction->base.type = *act;
-	    action = &xaction->base;
-
-	    trace_debug ("Want to evaluate expression");
-	    xaction->expr = gdb_parse_agent_expr (&act);
+	    action = xmalloc (sizeof (struct eval_expr_action));
+	    action->ops = &x_tracepoint_action_ops;
 	    break;
 	  }
 	default:
@@ -1833,6 +2106,8 @@ add_tracepoint_action (struct tracepoint *tpoint, char *packet)
       if (action == NULL)
 	break;
 
+      act = action->ops->init (act, action);
+
       if (seen_step_action_flag)
 	{
 	  tpoint->num_step_actions++;
@@ -4263,8 +4538,6 @@ tracepoint_was_hit (struct thread_info *tinfo, CORE_ADDR stop_pc)
 
 #if defined IN_PROCESS_AGENT && defined HAVE_UST
 struct ust_marker_data;
-static void collect_ust_data_at_tracepoint (struct tracepoint_hit_ctx *ctx,
-					    struct traceframe *tframe);
 #endif
 
 /* Create a trace frame for the hit of the given tracepoint in the
@@ -4412,105 +4685,31 @@ do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 			 struct traceframe *tframe,
 			 struct tracepoint_action *taction)
 {
-  enum eval_result_type err;
-
-  switch (taction->type)
+#ifdef IN_PROCESS_AGENT
+  if (taction->ops == NULL)
     {
-    case 'M':
-      {
-	struct collect_memory_action *maction;
-
-	maction = (struct collect_memory_action *) taction;
-
-	trace_debug ("Want to collect %s bytes at 0x%s (basereg %d)",
-		     pulongest (maction->len),
-		     paddress (maction->addr), maction->basereg);
-	/* (should use basereg) */
-	agent_mem_read (tframe, NULL,
-			(CORE_ADDR) maction->addr, maction->len);
-	break;
-      }
-    case 'R':
-      {
-	unsigned char *regspace;
-	struct regcache tregcache;
-	struct regcache *context_regcache;
-
-
-	trace_debug ("Want to collect registers");
-
-	/* Collect all registers for now.  */
-	regspace = add_traceframe_block (tframe,
-					 1 + register_cache_size ());
-	if (regspace == NULL)
-	  {
-	    trace_debug ("Trace buffer block allocation failed, skipping");
-	    break;
-	  }
-	/* Identify a register block.  */
-	*regspace = 'R';
-
-	context_regcache = get_context_regcache (ctx);
-
-	/* Wrap the regblock in a register cache (in the stack, we
-	   don't want to malloc here).  */
-	init_register_cache (&tregcache, regspace + 1);
-
-	/* Copy the register data to the regblock.  */
-	regcache_cpy (&tregcache, context_regcache);
-
-#ifndef IN_PROCESS_AGENT
-	/* On some platforms, trap-based tracepoints will have the PC
-	   pointing to the next instruction after the trap, but we
-	   don't want the user or GDB trying to guess whether the
-	   saved PC needs adjusting; so always record the adjusted
-	   stop_pc.  Note that we can't use tpoint->address instead,
-	   since it will be wrong for while-stepping actions.  This
-	   adjustment is a nop for fast tracepoints collected from the
-	   in-process lib (but not if GDBserver is collecting one
-	   preemptively), since the PC had already been adjusted to
-	   contain the tracepoint's address by the jump pad.  */
-	trace_debug ("Storing stop pc (0x%s) in regblock",
-		     paddress (ctx->stop_pc));
-
-	/* This changes the regblock, not the thread's
-	   regcache.  */
-	regcache_write_pc (&tregcache, ctx->stop_pc);
+      switch (taction->type)
+	{
+	case 'M':
+	  taction->ops = &m_tracepoint_action_ops;
+	  break;
+	case 'X':
+	  taction->ops = &x_tracepoint_action_ops;
+	  break;
+	case 'R':
+	  taction->ops = &r_tracepoint_action_ops;
+	  break;
+	case 'L':
+	  taction->ops = &l_tracepoint_action_ops;
+	  break;
+	default:
+	  return;
+	};
+    }
 #endif
-      }
-      break;
-    case 'X':
-      {
-	struct eval_expr_action *eaction;
-
-	eaction = (struct eval_expr_action *) taction;
 
-	trace_debug ("Want to evaluate expression");
-
-	err = eval_tracepoint_agent_expr (ctx, tframe, eaction->expr, NULL);
-
-	if (err != expr_eval_no_error)
-	  {
-	    record_tracepoint_error (tpoint, "action expression", err);
-	    return;
-	  }
-      }
-      break;
-    case 'L':
-      {
-#if defined IN_PROCESS_AGENT && defined HAVE_UST
-	trace_debug ("Want to collect static trace data");
-	collect_ust_data_at_tracepoint (ctx, tframe);
-#else
-	trace_debug ("warning: collecting static trace data, "
-		     "but static tracepoints are not supported");
-#endif
-      }
-      break;
-    default:
-      trace_debug ("unknown trace action '%c', ignoring", taction->type);
-      break;
-    }
+  if (taction->ops->execute (taction, ctx, tframe))
+    error_tracepoint = tpoint;
 }
 
 static int
@@ -4543,7 +4742,11 @@ condition_true_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 
   if (err != expr_eval_no_error)
     {
-      record_tracepoint_error (tpoint, "condition", err);
+      trace_debug ("Tracepoint %d at %s condition eval reports error %d",
+		   tpoint->number, paddress (tpoint->address), err);
+
+      record_expr_eval_error (err);
+      error_tracepoint = tpoint;
       /* The error case must return false.  */
       return 0;
     }
@@ -5666,55 +5869,8 @@ download_tracepoint_1 (struct tracepoint *tpoint)
       /* Now for each pointer, download the action.  */
       for (i = 0; i < tpoint->numactions; i++)
 	{
-	  CORE_ADDR ipa_action = 0;
 	  struct tracepoint_action *action = tpoint->actions[i];
-
-	  switch (action->type)
-	    {
-	    case 'M':
-	      ipa_action
-		= target_malloc (sizeof (struct collect_memory_action));
-	      write_inferior_memory (ipa_action,
-				     (unsigned char *) action,
-				     sizeof (struct collect_memory_action));
-	      break;
-	    case 'R':
-	      ipa_action
-		= target_malloc (sizeof (struct collect_registers_action));
-	      write_inferior_memory (ipa_action,
-				     (unsigned char *) action,
-				     sizeof (struct collect_registers_action));
-	      break;
-	    case 'X':
-	      {
-		CORE_ADDR expr;
-		struct eval_expr_action *eaction
-		  = (struct eval_expr_action *) action;
-
-		ipa_action = target_malloc (sizeof (*eaction));
-		write_inferior_memory (ipa_action,
-				       (unsigned char *) eaction,
-				       sizeof (*eaction));
-
-		expr = download_agent_expr (eaction->expr);
-		write_inferior_data_ptr
-		  (ipa_action + offsetof (struct eval_expr_action, expr),
-		   expr);
-		break;
-	      }
-	    case 'L':
-	      ipa_action = target_malloc
-		(sizeof (struct collect_static_trace_data_action));
-	      write_inferior_memory
-		(ipa_action,
-		 (unsigned char *) action,
-		 sizeof (struct collect_static_trace_data_action));
-	      break;
-	    default:
-	      trace_debug ("unknown trace action '%c', ignoring",
-			   action->type);
-	      break;
-	    }
+	  CORE_ADDR ipa_action = action->ops->download (action);
 
 	  if (ipa_action != 0)
 	    write_inferior_data_ptr
-- 
1.7.0.4

Re: [PATCH 2/2] OO tracepoint_action

[Yao Qi]

On 03/02/2012 09:00 AM, Yao Qi wrote:
> This is the major part of this patch series, which replace several
> switch/case blocks in code with function vectors.  It is easier to add
> new operation to tracepoint_action in the future.

This patch is a mechanical refactor change, except in
do_action_at_tracepoint.  In GDBserver, we can install
tracepoint_action_ops when creating tracepoint_action objects, however,
in IPA, all tracepoint_actions objects are written from GDBserver.  So
when writing tracepoint_actions to IPA, we firstly write field `ops' to
NULL, and check it in do_action_at_tracepoint.  If `ops' field is NULL,
install corresponding tracepoint_action_ops instances.  The other
approach is to expose the symbols of tracepoint_action_ops instances to
GDBserver, so GDBserver can write it to `ops' field.  This approach
exposed too much details of IPA (symbols of tracepoint_action_ops
variables), so I don't use it.

-- 
Yao (齐尧)

Re: [PATCH 2/2] OO tracepoint_action

[Pedro Alves]

On 03/02/2012 01:00 AM, Yao Qi wrote:

> +/* Operations on various types of tracepoint actions.  */
> +
> +struct tracepoint_action;
> +struct tracepoint_hit_ctx;
> +
> +struct tracepoint_action_ops
> +{
> +#ifndef IN_PROCESS_AGENT
> +  /* Parse rsp packet PACKET and initialize ACTION.  Return the updated
> +     pointer to rsp packet..  */
> +  char* (*init) (char *packet,  struct tracepoint_action* action);

I think this method is unnecessary.  Think like C++, where
there's no vtable slot for contructors.  You can just call the init
method directly, since the caller must always know the right type
(in order to malloc the object), and have it set ->ops.

static void
foo_ctor (struct foo *self, char *packet)
{
  self->ops = &foo_ops;

  ... construct ...
}

static struct foo *
new_foo (char *packet)
{
  struct foo *self;

  self = xmalloc (sizeof (struct foo));
  foo_ctor (self, packet);
  return self;
}

or simpler if there's no hierarchy to worry about:

static struct foo *self
new_foo (char *packet)
{
  struct foo * self = xmalloc (sizeof (struct foo));
  self->ops = &foo_ops;

  ... construct ...

  return self;
}

> @@ -1247,12 +1273,8 @@ static LONGEST get_timestamp (void);
>  /* Record that an error occurred during expression evaluation.  */
>  
>  static void
> -record_tracepoint_error (struct tracepoint *tpoint, const char *which,
> -			 enum eval_result_type rtype)
> +record_expr_eval_error (enum eval_result_type rtype)
>  {
> -  trace_debug ("Tracepoint %d at %s %s eval reports error %d",
> -	       tpoint->number, paddress (tpoint->address), which, rtype);
> -
>  #ifdef IN_PROCESS_AGENT
>    /* Only record the first error we get.  */
>    if (cmpxchg (&expr_eval_result,
> @@ -1263,8 +1285,6 @@ record_tracepoint_error (struct tracepoint *tpoint, const char *which,
>    if (expr_eval_result != expr_eval_no_error)
>      return;
>  #endif
> -
> -  error_tracepoint = tpoint;

By moving this error_tracepoint set elsewhere, you're bypassing the cmpxchg
synchronization just above, introducing the race this method was supposed to
prevent.  What was the motivation for this change?

>  }
>  
>  /* Trace buffer management.  */
> @@ -1603,6 +1623,298 @@ trace_buffer_alloc (size_t amt)


> +static CORE_ADDR
> +m_tracepoint_action_download (struct tracepoint_action *action)
> +{
> +  CORE_ADDR ipa_action = target_malloc (sizeof (struct collect_memory_action));
> +
> +  write_inferior_memory (ipa_action, (unsigned char *) action,
> +			 sizeof (struct collect_memory_action));
> +  /* Write NULL to field `ops'.  */
> +  write_inferior_data_ptr(ipa_action + offsetof (struct tracepoint_action, ops),
> +			  0);

Missing space before parens.  Several instances.

> +
> +static CORE_ADDR
> +l_tracepoint_action_download (struct tracepoint_action *action)
> +{
> +  CORE_ADDR ipa_action
> +    = target_malloc (sizeof (struct  collect_static_trace_data_action));

spurious double-space.

> @@ -4412,105 +4685,31 @@ do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
>  			 struct traceframe *tframe,
>  			 struct tracepoint_action *taction)
>  {
> -  enum eval_result_type err;
> -
> -  switch (taction->type)
> +#ifdef IN_PROCESS_AGENT
> +  if (taction->ops == NULL)
>      {

So ops is lazily initialized in the IPA?  I'm a little warry of the potential
for slowing down the collect path (adding several indirections, and
extra calls that are hard for the compiler to optimize out work against code
density, cache locality, etc.).  We want to squeeze out performance in the
nano-second range, though what matters the most is the case of tracepoint hit
and then the condition evaluating false.

> -    case 'M':
> -      {
> -	struct collect_memory_action *maction;
> -
> -	maction = (struct collect_memory_action *) taction;
> -
> -	trace_debug ("Want to collect %s bytes at 0x%s (basereg %d)",
> -		     pulongest (maction->len),
> -		     paddress (maction->addr), maction->basereg);
> -	/* (should use basereg) */
> -	agent_mem_read (tframe, NULL,
> -			(CORE_ADDR) maction->addr, maction->len);
> -	break;
> -      }
> -    case 'R':
> -      {
> -	unsigned char *regspace;
> -	struct regcache tregcache;
> -	struct regcache *context_regcache;
> -
> -
> -	trace_debug ("Want to collect registers");
> -
> -	/* Collect all registers for now.  */
> -	regspace = add_traceframe_block (tframe,
> -					 1 + register_cache_size ());
> -	if (regspace == NULL)
> -	  {
> -	    trace_debug ("Trace buffer block allocation failed, skipping");
> -	    break;
> -	  }
> -	/* Identify a register block.  */
> -	*regspace = 'R';
> -
> -	context_regcache = get_context_regcache (ctx);
> -
> -	/* Wrap the regblock in a register cache (in the stack, we
> -	   don't want to malloc here).  */
> -	init_register_cache (&tregcache, regspace + 1);
> -
> -	/* Copy the register data to the regblock.  */
> -	regcache_cpy (&tregcache, context_regcache);
> -
> -#ifndef IN_PROCESS_AGENT
> -	/* On some platforms, trap-based tracepoints will have the PC
> -	   pointing to the next instruction after the trap, but we
> -	   don't want the user or GDB trying to guess whether the
> -	   saved PC needs adjusting; so always record the adjusted
> -	   stop_pc.  Note that we can't use tpoint->address instead,
> -	   since it will be wrong for while-stepping actions.  This
> -	   adjustment is a nop for fast tracepoints collected from the
> -	   in-process lib (but not if GDBserver is collecting one
> -	   preemptively), since the PC had already been adjusted to
> -	   contain the tracepoint's address by the jump pad.  */
> -	trace_debug ("Storing stop pc (0x%s) in regblock",
> -		     paddress (ctx->stop_pc));
> -
> -	/* This changes the regblock, not the thread's
> -	   regcache.  */
> -	regcache_write_pc (&tregcache, ctx->stop_pc);
> +      switch (taction->type)
> +	{
> +	case 'M':
> +	  taction->ops = &m_tracepoint_action_ops;
> +	  break;
> +	case 'X':
> +	  taction->ops = &x_tracepoint_action_ops;
> +	  break;
> +	case 'R':
> +	  taction->ops = &r_tracepoint_action_ops;
> +	  break;
> +	case 'L':
> +	  taction->ops = &l_tracepoint_action_ops;
> +	  break;
> +	default:
> +	  return;

I understand why you did this, though this makes me question the whole exercise.
It seems the point of not having switch statements ends up mostly moot...  If
you still have to write/update switch, you may as well just call the function
directly, instead of going through the extra indirection.

-- 
Pedro Alves

Re: [PATCH 2/2] OO tracepoint_action

[Yao Qi]

On 03/02/2012 07:49 PM, Pedro Alves wrote:
>> >  #ifdef IN_PROCESS_AGENT
>> >    /* Only record the first error we get.  */
>> >    if (cmpxchg (&expr_eval_result,
>> > @@ -1263,8 +1285,6 @@ record_tracepoint_error (struct tracepoint *tpoint, const char *which,
>> >    if (expr_eval_result != expr_eval_no_error)
>> >      return;
>> >  #endif
>> > -
>> > -  error_tracepoint = tpoint;
> By moving this error_tracepoint set elsewhere, you're bypassing the cmpxchg
> synchronization just above, introducing the race this method was supposed to
> prevent.  What was the motivation for this change?
> 

The motivation here is to not pass tracepoint `tpoint' to
tracepoint_action_ops->execute, because `tpoint' is not used in actions
except X action.

The update to error_tracepoint is guarded by action->ops->execute

  if (taction->ops->execute (taction, ctx, tframe))
    error_tracepoint = tpoint;

and ops->execute calls cmpxchg (for X action).  `execute' of other types
of action always return 0.  So I think this is equivalent to original.

>> > +
>> > +static CORE_ADDR
>> > +l_tracepoint_action_download (struct tracepoint_action *action)
>> > +{
>> > +  CORE_ADDR ipa_action
>> > +    = target_malloc (sizeof (struct  collect_static_trace_data_action));
> spurious double-space.
> 
>> > @@ -4412,105 +4685,31 @@ do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
>> >  			 struct traceframe *tframe,
>> >  			 struct tracepoint_action *taction)
>> >  {
>> > -  enum eval_result_type err;
>> > -
>> > -  switch (taction->type)
>> > +#ifdef IN_PROCESS_AGENT
>> > +  if (taction->ops == NULL)
>> >      {
> So ops is lazily initialized in the IPA?  I'm a little warry of the potential
> for slowing down the collect path (adding several indirections, and
> extra calls that are hard for the compiler to optimize out work against code
> density, cache locality, etc.).  We want to squeeze out performance in the
> nano-second range, though what matters the most is the case of tracepoint hit
> and then the condition evaluating false.
> 

Hmm, if we concern performance here, it is fine with me.  Then, I
suggest that we drop the changes in do_action_at_tracepoint, and get rid
of fields `init' and `execute' from struct tracepoint_action_ops, like this,

struct tracepoint_action_ops
{
  CORE_ADDR (*download) (struct tracepoint_action *action);
};

struct tracepoint_action
{
#ifndef IN_PROCESS_AGENT
  struct tracepoint_action_ops *ops;
#endif
  char type;
};

What do you think?  Note that I plan to add a new field `send' in
tracepoint_action_ops to send different types of actions to agent
through "command buffer".

-- 
Yao (齐尧)

Re: [PATCH 2/2] OO tracepoint_action

[Pedro Alves]

On 03/02/2012 01:35 PM, Yao Qi wrote:

> The update to error_tracepoint is guarded by action->ops->execute
> 
>   if (taction->ops->execute (taction, ctx, tframe))
>     error_tracepoint = tpoint;
> 
> and ops->execute calls cmpxchg (for X action).  `execute' of other types
> of action always return 0.  So I think this is equivalent to original.

  /* Only record the first error we get.  */
  if (cmpxchg (&expr_eval_result,

So if two threads (in the IPA) happen to trigger an eval error at the same
time, only the first error is stored.  And then error_tracepoint
is set to the tracepoint whose action was being evaluated.  With your change,
the _last_ (random) thread to return from ->execute would set error_tracepoint,
even though that thread hadn't set expr_eval_result.  It doesn't look equivalent.

> struct tracepoint_action_ops
> {
>   CORE_ADDR (*download) (struct tracepoint_action *action);
> };
> 
> struct tracepoint_action
> {
> #ifndef IN_PROCESS_AGENT
>   struct tracepoint_action_ops *ops;
> #endif
>   char type;
> };
> 
> What do you think?  

Let's try that.

> Note that I plan to add a new field `send' in
> tracepoint_action_ops to send different types of actions to agent
> through "command buffer".

-- 
Pedro Alves

[PATCH 1/2] New field stop_pc in tracepoint_hit_ctx

[Yao Qi]

This patch is to add a new field `stop_pc' in tracepoint_hit_ctx, so
parameter `stop_pc' used here and there can be removed.  This change
allows us to give a clean interface in next patch.

Note the name `tracepoint_hit_ctx' is not very accurate, because it
has been used in collect_data_at_step for "while-stepping" action also.
It may be renamed to `tracepoint_action_ctx', which is about the
context of doing tracepoint actions.  I don't rename `tracepoint_hit_ctx'
to keep this patch as readable as possible.  I can send a follow-up
patch to rename it if this change is reasonable.

As `tracepoint_hit_ctx' is about the context of doing tracepoint
actions, it is natural to add field `stop_pc' to show the pc value when
to do tracepoint actions.

gdb/gdbserver:
	* tracepoint (struct tracepoint_hit_ctx) <stop_pc>: New field.
	(collect_data_at_tracepoint): Remove parameter `stop_pc'.
	Update callers.
	(collect_data_at_step): Remove parameter `stop_pc'.  Update
	callers.
	(do_action_at_tracepoint): Remove parameter `stop_pc'.
---
 gdb/gdbserver/tracepoint.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 7167876..5dcc7a4 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -1125,6 +1125,7 @@ char *tracing_stop_note;
 struct tracepoint_hit_ctx
 {
   enum tracepoint_type type;
+  CORE_ADDR stop_pc;
 };
 
 #ifdef IN_PROCESS_AGENT
@@ -1205,17 +1206,14 @@ static void clear_installed_tracepoints (void);
 #endif
 
 static void collect_data_at_tracepoint (struct tracepoint_hit_ctx *ctx,
-					CORE_ADDR stop_pc,
 					struct tracepoint *tpoint);
 #ifndef IN_PROCESS_AGENT
 static void collect_data_at_step (struct tracepoint_hit_ctx *ctx,
-				  CORE_ADDR stop_pc,
 				  struct tracepoint *tpoint, int current_step);
 static void compile_tracepoint_condition (struct tracepoint *tpoint,
 					  CORE_ADDR *jump_entry);
 #endif
 static void do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
-				     CORE_ADDR stop_pc,
 				     struct tracepoint *tpoint,
 				     struct traceframe *tframe,
 				     struct tracepoint_action *taction);
@@ -4051,6 +4049,7 @@ tracepoint_finished_step (struct thread_info *tinfo, CORE_ADDR stop_pc)
 	       wstep->tp_number, paddress (wstep->tp_address));
 
   ctx.base.type = trap_tracepoint;
+  ctx.base.stop_pc = stop_pc;
   ctx.regcache = get_thread_regcache (tinfo, 1);
 
   while (wstep != NULL)
@@ -4074,7 +4073,7 @@ tracepoint_finished_step (struct thread_info *tinfo, CORE_ADDR stop_pc)
 
       /* Collect data.  */
       collect_data_at_step ((struct tracepoint_hit_ctx *) &ctx,
-			    stop_pc, tpoint, wstep->current_step);
+			    tpoint, wstep->current_step);
 
       if (wstep->current_step >= tpoint->step_count)
 	{
@@ -4213,6 +4212,7 @@ tracepoint_was_hit (struct thread_info *tinfo, CORE_ADDR stop_pc)
     return 0;
 
   ctx.base.type = trap_tracepoint;
+  ctx.base.stop_pc = stop_pc;
   ctx.regcache = get_thread_regcache (tinfo, 1);
 
   for (tpoint = tracepoints; tpoint; tpoint = tpoint->next)
@@ -4235,7 +4235,7 @@ tracepoint_was_hit (struct thread_info *tinfo, CORE_ADDR stop_pc)
 	      || (condition_true_at_tracepoint
 		  ((struct tracepoint_hit_ctx *) &ctx, tpoint)))
 	    collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-					stop_pc, tpoint);
+					tpoint);
 
 	  if (stopping_tracepoint
 	      || trace_buffer_is_full
@@ -4271,7 +4271,7 @@ static void collect_ust_data_at_tracepoint (struct tracepoint_hit_ctx *ctx,
    given thread.  */
 
 static void
-collect_data_at_tracepoint (struct tracepoint_hit_ctx *ctx, CORE_ADDR stop_pc,
+collect_data_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 			    struct tracepoint *tpoint)
 {
   struct traceframe *tframe;
@@ -4305,7 +4305,7 @@ collect_data_at_tracepoint (struct tracepoint_hit_ctx *ctx, CORE_ADDR stop_pc,
 		       tpoint->actions_str[acti]);
 #endif
 
-	  do_action_at_tracepoint (ctx, stop_pc, tpoint, tframe,
+	  do_action_at_tracepoint (ctx, tpoint, tframe,
 				   tpoint->actions[acti]);
 	}
 
@@ -4320,7 +4320,6 @@ collect_data_at_tracepoint (struct tracepoint_hit_ctx *ctx, CORE_ADDR stop_pc,
 
 static void
 collect_data_at_step (struct tracepoint_hit_ctx *ctx,
-		      CORE_ADDR stop_pc,
 		      struct tracepoint *tpoint, int current_step)
 {
   struct traceframe *tframe;
@@ -4342,7 +4341,7 @@ collect_data_at_step (struct tracepoint_hit_ctx *ctx,
 		       tpoint->number, paddress (tpoint->address),
 		       tpoint->step_actions_str[acti]);
 
-	  do_action_at_tracepoint (ctx, stop_pc, tpoint, tframe,
+	  do_action_at_tracepoint (ctx, tpoint, tframe,
 				   tpoint->step_actions[acti]);
 	}
 
@@ -4409,7 +4408,6 @@ get_context_regcache (struct tracepoint_hit_ctx *ctx)
 
 static void
 do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
-			 CORE_ADDR stop_pc,
 			 struct tracepoint *tpoint,
 			 struct traceframe *tframe,
 			 struct tracepoint_action *taction)
@@ -4473,11 +4471,11 @@ do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
 	   preemptively), since the PC had already been adjusted to
 	   contain the tracepoint's address by the jump pad.  */
 	trace_debug ("Storing stop pc (0x%s) in regblock",
-		     paddress (stop_pc));
+		     paddress (ctx->stop_pc));
 
 	/* This changes the regblock, not the thread's
 	   regcache.  */
-	regcache_write_pc (&tregcache, stop_pc);
+	regcache_write_pc (&tregcache, ctx->stop_pc);
 #endif
       }
       break;
@@ -5411,6 +5409,7 @@ gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
     return;
 
   ctx.base.type = fast_tracepoint;
+  ctx.base.stop_pc = tpoint->address;
   ctx.regs = regs;
   ctx.regcache_initted = 0;
   /* Wrap the regblock in a register cache (in the stack, we don't
@@ -5440,7 +5439,7 @@ gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
 					   ctx.tpoint))
 	{
 	  collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-				      ctx.tpoint->address, ctx.tpoint);
+				      ctx.tpoint);
 
 	  /* Note that this will cause original insns to be written back
 	     to where we jumped from, but that's OK because we're jumping
@@ -6280,6 +6279,8 @@ gdb_probe (const struct marker *mdata, void *probe_private,
     }
 
   tpoint = ust_marker_to_static_tracepoint (mdata);
+  ctx.base.stop_pc = tpoint->address;
+
   if (tpoint == NULL)
     {
       trace_debug ("gdb_probe: marker not known: "
@@ -6308,7 +6309,7 @@ gdb_probe (const struct marker *mdata, void *probe_private,
 				       tpoint))
     {
       collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
-				  tpoint->address, tpoint);
+				  tpoint);
 
       if (stopping_tracepoint
 	  || trace_buffer_is_full
-- 
1.7.0.4

Re: [PATCH 1/2] New field stop_pc in tracepoint_hit_ctx

[Pedro Alves]

On 03/02/2012 01:00 AM, Yao Qi wrote:
> This patch is to add a new field `stop_pc' in tracepoint_hit_ctx, so
> parameter `stop_pc' used here and there can be removed.  This change
> allows us to give a clean interface in next patch.
> 
> Note the name `tracepoint_hit_ctx' is not very accurate, because it
> has been used in collect_data_at_step for "while-stepping" action also.
> It may be renamed to `tracepoint_action_ctx', which is about the
> context of doing tracepoint actions.  I don't rename `tracepoint_hit_ctx'
> to keep this patch as readable as possible.  I can send a follow-up
> patch to rename it if this change is reasonable.
> 
> As `tracepoint_hit_ctx' is about the context of doing tracepoint
> actions, it is natural to add field `stop_pc' to show the pc value when
> to do tracepoint actions.

Fine with me, though honestly I find that the similarity between
tracepoint_action vs tracepoint_action_ctx will increase confusion,
not reduce.

>    tpoint = ust_marker_to_static_tracepoint (mdata);
> +  ctx.base.stop_pc = tpoint->address;

tpoint may be NULL, see just below.

> +
>    if (tpoint == NULL)
>      {

-- 
Pedro Alves