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@mentor.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
>