Re: [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place

[Pedro Alves <pedro@codesourcery.com>]

On Friday 04 November 2011 16:18:28, Yao Qi wrote:

> The problem is that when setting two fast tracepoints at the same place,
> only one works.  When setting fast tracepoint at an address, a jump pad
> is created to connect a jmp insn written at tracepoint place and
> collection routine.  Even multiple fast tracepoints are set at the same
> address, there is only jump pad associated with this address.  Inferior
> will jump to gdb_collect from jump pad with the first tracepoint
> information, rather than all tracepoints information at that address.
> This is cause of this problem.

Yeah, a known problem that we never got to fix.  We talked about it
in the context of the original private stub where later the gdbserver
port was based on, and we worried about the extra instructions
slowing down fast collection.  Every instruction counts in this path,
and speed was more important.  But in the gdbserver case, I think
we're fine.

> @@ -5417,47 +5417,62 @@ gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
>    if (!tracing)
>      return;
>  
> -  if (!tpoint->enabled)
> -    return;
> -
>    ctx.base.type = fast_tracepoint;
>    ctx.regs = regs;
>    ctx.regcache_initted = 0;
> -  ctx.tpoint = tpoint;
>  
> -  /* Wrap the regblock in a register cache (in the stack, we don't
> -     want to malloc here).  */
> -  ctx.regspace = alloca (register_cache_size ());
> -  if (ctx.regspace == NULL)
> +  ctx.tpoint = tpoint;
> +  for (; ctx.tpoint && ctx.tpoint->address == tpoint->address;
> +       ctx.tpoint = ctx.tpoint->next)

Put the initialization where it belongs.  Use explicit NULL as the
surrounding code:

+  for (ctx.tpoint = tpoint;
+       ctx.tpoint != NULL && ctx.tpoint->address == tpoint->address;
+       ctx.tpoint = ctx.tpoint->next)

>      {
> -      trace_debug ("Trace buffer block allocation failed, skipping");
> -      return;
> -    }
> +      if (!ctx.tpoint->enabled)
> +       continue;
>  
> -  /* Test the condition if present, and collect if true.  */
> -  if (tpoint->cond == NULL
> -      || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
> -                                      tpoint))
> -    {
> -      collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
> -                                 tpoint->address, tpoint);
> +      /* Multiple tracepoints of different types, such as fast tracepoint and
> +        static tracepoint, can be set at the same address.  */

I was going to suggest sorting by type as well as address, but that
won't play well with adding new tracepoints while the trace run is
ongoing.  ;-)  (Splitting the single list into separate sorted lists
one per type would work though, but that's a bigger change.)

> +      if (ctx.tpoint->type != tpoint->type)
> +       continue;
>  
> -      /* Note that this will cause original insns to be written back
> -        to where we jumped from, but that's OK because we're jumping
> -        back to the next whole instruction.  This will go badly if
> -        instruction restoration is not atomic though.  */
> -      if (stopping_tracepoint
> -         || trace_buffer_is_full
> -         || expr_eval_result != expr_eval_no_error)
> -       stop_tracing ();
> -    }
> -  else
> -    {
> -      /* If there was a condition and it evaluated to false, the only
> -        way we would stop tracing is if there was an error during
> -        condition expression evaluation.  */
> -      if (expr_eval_result != expr_eval_no_error)
> -       stop_tracing ();
> +      /* Wrap the regblock in a register cache (in the stack, we don't
> +        want to malloc here).  */
> +      ctx.regspace = alloca (register_cache_size ());
> +      if (ctx.regspace == NULL)
> +       {
> +         trace_debug ("Trace buffer block allocation failed, skipping");
> +         continue;
> +       }

This grows the stack at every iteration.  We should do this only
once.  Please move it out of the loop.

Okay with these changes.

-- 
Pedro Alves