Re: [PATCH] Fix tracepoint tstart again get gdb_assert

Yao Qi
2011-12-07T04:09:19Z

On 12/06/2011 11:56 PM, Hui Zhu wrote:
> Hi,
>
> I use a gdb tstart again in a section an got:
>
> (gdb) tstart
> (gdb) tstop
> (gdb) tstart
> ../../src/gdb/tracepoint.c:1770: internal-error: start_tracing:
> Assertion `!loc->inserted' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
>

Thanks for this patch! It is related to my previous commit

[patch 4/8] Download tracepoint locations and track its status
http://sourceware.org/ml/gdb-patches/2011-11/msg00337.html

> The reason is:
> start_tracing:
> for (loc = b->loc; loc; loc = loc->next)
> {
> /* Since tracepoint locations are never duplicated, `inserted'
> flag should be zero. */
> gdb_assert (!loc->inserted);
>
> target_download_tracepoint (loc);
>
> loc->inserted = 1;
> }
>
> But in stop_tracing and trace_status_command don't have code to set
> inserted back to 0.
>

Right, I agree that loc->inserted should be cleared on stop_tracing.
However, I don't know why we have to clear loc->inserted in
trace_status_command. A few comments below.

> So I make a patch for it.
>
> Thanks,
> Hui
>
> 2011-12-06 Hui Zhu <teawater@gmail.com>
>
> * tracepoint.c (stop_tracing): Set loc->inserted if need.
> (trace_status_command): Ditto.
> ---
> tracepoint.c | 41 ++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> --- a/tracepoint.c
> +++ b/tracepoint.c
> @@ -1847,6 +1847,9 @@ void
> stop_tracing (char *note)
> {
> int ret;
> + VEC(breakpoint_p) *tp_vec = NULL;
> + int ix;
> + struct breakpoint *b;
>
> target_trace_stop ();
>
> @@ -1859,6 +1862,22 @@ stop_tracing (char *note)
>
> /* Should change in response to reply? */
> current_trace_status ()->running = 0;
> +
> + tp_vec = all_tracepoints ();
> + for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> + {
> + struct tracepoint *t = (struct tracepoint *) b;
> + struct bp_location *loc;
> +
> + if ((b->type == bp_fast_tracepoint
> + ? !may_insert_fast_tracepoints
> + : !may_insert_tracepoints))
> + continue;
> +

I don't think this check above is needed here. In start_tracing, we
need may_insert_fast_tracepoints and may_insert_tracepoints to download
desired type of tracepoint. However, these two variables may be changed
in a trace experiment, so we don't have to check them and clear
loc->inserted unconditionally.

> + for (loc = b->loc; loc; loc = loc->next)
> + loc->inserted = 0;
> + }
> + VEC_free (breakpoint_p, tp_vec);
> }
>

> /* tstatus command */
> @@ -1868,7 +1887,7 @@ trace_status_command (char *args, int fr
> struct trace_status *ts = current_trace_status ();
> int status, ix;
> VEC(breakpoint_p) *tp_vec = NULL;
> - struct breakpoint *t;
> + struct breakpoint *b;
>
> status = target_get_trace_status (ts);
>
> @@ -2013,8 +2032,24 @@ trace_status_command (char *args, int fr
> /* Now report any per-tracepoint status available. */
> tp_vec = all_tracepoints ();
>
> - for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
> - target_get_tracepoint_status (t, NULL);
> + for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
> + {
> + struct tracepoint *t = (struct tracepoint *) b;
> + struct bp_location *loc;
> +
> + target_get_tracepoint_status (b, NULL);
> +
> + if (!ts->running)
> + {
> + if ((b->type == bp_fast_tracepoint
> + ? !may_insert_fast_tracepoints
> + : !may_insert_tracepoints))
> + continue;
> +
> + for (loc = b->loc; loc; loc = loc->next)
> + loc->inserted = 0;
> + }
> + }
>
> VEC_free (breakpoint_p, tp_vec);
> }

As I said at the beginning, I don't understand why we have to clear
loc->inserted in trace_status_command when ts->running is 0.

--
Yao (é½å°§)