Re: [PATCH] Fix tracepoint tstart again get gdb_assert

[Yao Qi <yao at codesourcery dot com>]

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 (éå)