This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place
On 11/05/2011 01:03 AM, Pedro Alves wrote:
> 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)
>
OK.
>> > {
>> > - 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.)
>
Yes, this list should be taken care of when adding tracepoint while
trace is running. Since there shouldn't be many tracepoints setting at
the same address, so it is fine to me to iterate all tracepoints of the
same address.
>> > - /* 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.
>
OK, I misunderstood ctx.regspace. Fixed as you suggested.
> Okay with these changes.
Patch below is what I committed.
--
Yao (éå)
gdb/gdbserver:
2011-11-05 Yao Qi <yao@codesourcery.com>
* tracepoint.c (gdb_collect): Loop over tracepoints of same
address as TPOINT's.
gdb/testsuite:
2011-11-05 Yao Qi <yao@codesourcery.com>
* gdb.trace/trace-break.exp: Add test on setting two
fast tracepoints at the same address.
Index: gdb/gdbserver/tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/tracepoint.c,v
retrieving revision 1.29
diff -u -r1.29 tracepoint.c
--- gdb/gdbserver/tracepoint.c 2 Nov 2011 23:44:21 -0000 1.29
+++ gdb/gdbserver/tracepoint.c 5 Nov 2011 13:06:38 -0000
@@ -5480,14 +5480,9 @@
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 ());
@@ -5497,30 +5492,49 @@
return;
}
- /* Test the condition if present, and collect if true. */
- if (tpoint->cond == NULL
- || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
- tpoint))
+ for (ctx.tpoint = tpoint;
+ ctx.tpoint != NULL && ctx.tpoint->address == tpoint->address;
+ ctx.tpoint = ctx.tpoint->next)
{
- collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
- tpoint->address, tpoint);
+ if (!ctx.tpoint->enabled)
+ 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 ();
+ /* Multiple tracepoints of different types, such as fast tracepoint and
+ static tracepoint, can be set at the same address. */
+ if (ctx.tpoint->type != tpoint->type)
+ continue;
+
+ /* Test the condition if present, and collect if true. */
+ if (ctx.tpoint->cond == NULL
+ || condition_true_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
+ ctx.tpoint))
+ {
+ collect_data_at_tracepoint ((struct tracepoint_hit_ctx *) &ctx,
+ ctx.tpoint->address, 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
+ 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 ();
+ break;
+ }
+ }
+ 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 ();
+ break;
+ }
+ }
}
}
Index: gdb/testsuite/gdb.trace/trace-break.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/trace-break.exp,v
retrieving revision 1.1
diff -u -r1.1 trace-break.exp
--- gdb/testsuite/gdb.trace/trace-break.exp 31 Oct 2011 12:55:26 -0000 1.1
+++ gdb/testsuite/gdb.trace/trace-break.exp 5 Nov 2011 13:06:38 -0000
@@ -234,6 +234,7 @@
break_trace_same_addr_1 "ftrace" ${break_always_inserted}
break_trace_same_addr_2 "trace" "ftrace" ${break_always_inserted}
break_trace_same_addr_2 "ftrace" "trace" ${break_always_inserted}
+ break_trace_same_addr_2 "ftrace" "ftrace" ${break_always_inserted}
break_trace_same_addr_3 "ftrace" ${break_always_inserted}
break_trace_same_addr_4 "ftrace" ${break_always_inserted}
}