Re: [PATCH 2/2] OO tracepoint_action

Pedro Alves
2012-03-02T11:50:06Z

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