Re: [PATCH] Fix error when gdb connect to a stub that tracepoint is running[1/2] reset current_trace_status in the begin of remote_start_remote

[Hui Zhu <hui_zhu at mentor dot com>]

Hi Yao,

Thanks for you review.
I have 2 ideas with your mail:
1. If we want move merge_uploaded_tracepoints (&uploaded_tps); to the 
begin, I think all the code about tracepoint should move to the begin of 
this function.

  /* Possibly the target has been engaged in a trace run started
     previously; find out where things are at.  */
  if (remote_get_trace_status (current_trace_status ()) != -1)
    {
      struct uploaded_tp *uploaded_tps = NULL;
      struct uploaded_tsv *uploaded_tsvs = NULL;

      if (current_trace_status ()->running)
	printf_filtered (_("Trace is already running on the target.\n"));

      /* Get trace state variables first, they may be checked when
	 parsing uploaded commands.  */

      remote_upload_trace_state_variables (&uploaded_tsvs);

      merge_uploaded_trace_state_variables (&uploaded_tsvs);

      remote_upload_tracepoints (&uploaded_tps);

      merge_uploaded_tracepoints (&uploaded_tps);
    }

2.  But I think move the code before start_remote is not very well.
Because the code before tracepoint code do a lot of init work, maybe 
move it will bring more issues to current code.

Best,
Hui

On 01/31/12 16:22, Yao Qi wrote:
> On 01/31/2012 10:04 AM, Hui Zhu wrote:
> > #9  0x00000000006cc8e5 in post_create_inferior (target=0x1d1e860,
> > from_tty=1) at ../../src/gdb/infcmd.c:431
> > #10 0x00000000006d479d in start_remote (from_tty=1) at
> > ../../src/gdb/infrun.c:2309
> > #11 0x00000000005d80c9 in remote_start_remote (from_tty=1,
> > target=0x1cefce0, extended_p=0)
> >      at ../../src/gdb/remote.c:3367
> >
> > About this patch, what I do is:
> > 1. If we try to connect an new stub, reset current_trace_status to unknown.
> > 2. Change remote_can_download_tracepoint to if the current_trace_status
> > is unknown, return false.
> > After this patch, the tracepoint insered after:
> >    /* Possibly the target has been engaged in a trace run started
> >       previously; find out where things are at.  */
> >    if (remote_get_trace_status (current_trace_status ()) != -1)
> >      {
> > I think that will make us handle tracepoint issue more easy.
>
> It is caused by setting `inserted' flag improperly, but I have some
> different understanding on the cause of improper-set-of-inserted-flag.
>
> As you posted, the call stack is,
>
> remote_start_remote
>          |
>          +-->  start_remote
>          |          |
>          |          `-->  .... [1]
>          `-->  merge_uploaded_tracepoints
>
> Error occurs in the callees of [1].  In fact, the `inserted' flag will
> be set in merge_uploaded_tracepoints, if it has been in remote target
> (See merge_uploaded_tracepoints).  The intention of these bits is to
> mark tracepoint bp_locations as `inserted' to avoid inserting them
> again.  The root cause is that we use the `inserted' flag (in calleees
> of [1]) prior to setting it properly (in merge_uploaded_tracepoints), so
> the fix to this problem is moving merge_uploaded_tracepoints prior to
> start_remote.  I drafted a patch in this way, and fixed the problem you
> posted.
>
> Patch attached is used to illustrate my thought to fix this problem.  I
> am not confident on it because I don't know it is correct to change the
> order of function calls in remote_start_remote.  The "non stop" path in
> remote_start_remote is not affected by this patch.  In the "stop" path,
> the order of some functions call is changed, but don't know they are
> equivalent.
>
>     Original                            Patched
>
>     start_remote                  merge_uploaded_tracepoints
>     remote_check_symbols          start_remote
>     merge_uploaded_tracepoints    remote_check_symbols