[PATCH 2/2] OO tracepoint_action

Yao Qi
2012-03-02T01:00:55Z

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