* [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place
@ 2011-11-04 16:18 Yao Qi
2011-11-04 17:03 ` Pedro Alves
0 siblings, 1 reply; 3+ messages in thread
From: Yao Qi @ 2011-11-04 16:18 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]
Some days ago, I sent a test case (trace-break.exp) and a fix about
setting breakpoint and fast tracepoint at the same the same address,
[patch, gdbserver] Uninsert bpkt when regular and fast tracepoint are
set at the same address
http://sourceware.org/ml/gdb-patches/2011-10/msg00714.html
I read trace-break.exp again, and find we didn't test setting two fast
tracepoints at the same address. This test can be done easily by adding
one line in trace-brea.exp (see below in my patch), however, it reveals
one bug about tracepoint.
FAIL: gdb.trace/trace-break.exp: 2 ftrace ftrace on: tfind frame 1
FAIL: gdb.trace/trace-break.exp: 2 ftrace ftrace off: tfind frame 1
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.
Fortunately, tracepoints list are sorted, and jump pad is associated
with the "first" tracepoint of them. So tracepoint passed to
gdb_collect is the "first" tracepoint of them which have the same
address, then we can loop over tracepoints started from "first"
tracepoint, and call collect_data_at_tracepoint in the loop. This is
how this written. Tested on x86_64-linux native-gdbserver, OK to apply?
--
Yao (é½å°§)
[-- Attachment #2: 0001-two-ftrace-point-at-the-same-addr.patch --]
[-- Type: text/x-patch, Size: 4545 bytes --]
gdb/gdbserver:
2011-11-03 Yao Qi <yao@codesourcery.com>
* tracepoint.c (gdb_collect): Loop over TPOINT.
gdb/testsuite:
2011-11-03 Yao Qi <yao@codesourcery.com>
* gdb.trace/trace-break.exp: Add test on setting two
fast tracepoints at the same address.
---
gdb/gdbserver/tracepoint.c | 83 ++++++++++++++++++-------------
gdb/testsuite/gdb.trace/trace-break.exp | 1 +
2 files changed, 50 insertions(+), 34 deletions(-)
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 0a64104..f09a7d8 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -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)
{
- 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. */
+ 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;
+ }
+
+ /* 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;
+ }
+ }
}
}
diff --git a/gdb/testsuite/gdb.trace/trace-break.exp b/gdb/testsuite/gdb.trace/trace-break.exp
index 4a14d32..c2d7b2c 100644
--- a/gdb/testsuite/gdb.trace/trace-break.exp
+++ b/gdb/testsuite/gdb.trace/trace-break.exp
@@ -234,6 +234,7 @@ if { [gdb_test "info sharedlibrary" ".*libinproctrace\.so.*" "IPA loaded"] != 0
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}
}
--
1.7.0.4
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place
2011-11-04 16:18 [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place Yao Qi
@ 2011-11-04 17:03 ` Pedro Alves
2011-11-05 13:14 ` Yao Qi
0 siblings, 1 reply; 3+ messages in thread
From: Pedro Alves @ 2011-11-04 17:03 UTC (permalink / raw)
To: gdb-patches; +Cc: Yao Qi
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place
2011-11-04 17:03 ` Pedro Alves
@ 2011-11-05 13:14 ` Yao Qi
0 siblings, 0 replies; 3+ messages in thread
From: Yao Qi @ 2011-11-05 13:14 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
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}
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-11-05 13:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-04 16:18 [patch/gdbserver] Fix a bug when setting two fast tracepoints at the same place Yao Qi
2011-11-04 17:03 ` Pedro Alves
2011-11-05 13:14 ` Yao Qi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox