Re: [PATCH] store trace default-collect to target [1/6] target, trace and remote

[Hui Zhu <teawater@gmail.com>]

Ping http://sourceware.org/ml/gdb-patches/2013-04/msg00452.html

Thanks,
Hui

On Tue, Apr 16, 2013 at 5:28 PM, Hui Zhu <teawater@gmail.com> wrote:
> Hi Yao and Abid,
>
> Thanks for your review.
>
> On Thu, Apr 11, 2013 at 6:48 PM, Yao Qi <yao@codesourcery.com> wrote:
>> On 04/11/2013 02:16 PM, Hui Zhu wrote:
>>>
>>> --- a/remote.c
>>> +++ b/remote.c
>>> @@ -215,6 +215,8 @@ static int remote_get_trace_status (stru
>>>   static int remote_upload_tracepoints (struct uploaded_tp **utpp);
>>>
>>>   static int remote_upload_trace_state_variables (struct uploaded_tsv
>>> **utsvp);
>>> +
>>> +static void remote_upload_trace_default_collest (char **collectp);
>>
>>                                            ^^^^^^^ typo "collect"
>
> Oops, all fixed.
>
>>
>>>
>>>   static void remote_query_supported (void);
>>>
>>> @@ -3567,6 +3569,7 @@ remote_start_remote (int from_tty, struc
>>>       {
>>>         struct uploaded_tp *uploaded_tps = NULL;
>>>         struct uploaded_tsv *uploaded_tsvs = NULL;
>>> +      char *uploaded_default_collest = NULL;
>>
>>                                 ^^^^^^^ typo.
>>
>>>
>>>         if (current_trace_status ()->running)
>>>         printf_filtered (_("Trace is already running on the target.\n"));
>>> @@ -3581,6 +3584,10 @@ remote_start_remote (int from_tty, struc
>>>         remote_upload_tracepoints (&uploaded_tps);
>>>
>>>         merge_uploaded_tracepoints (&uploaded_tps);
>>> +
>>> +      remote_upload_trace_default_collest (&uploaded_default_collest);
>>> +
>>> +      trace_set_default_collect (uploaded_default_collest);
>>
>>
>> Likewise.
>>
>>>       }
>>>
>>>     /* The thread and inferior lists are now synchronized with the
>>> @@ -10567,6 +10574,23 @@ remote_download_tracepoint (struct bp_lo
>>>     do_cleanups (old_chain);
>>>   }
>>>
>>> +static void
>>> +remote_download_tracepoint_default_collect (char *collect)
>>
>>
>> Add a comment to this function.
>
> Fixed.
>
> /* Download COLLECT that is default collect string to target.  */
>
>>
>>>
>>> +static void
>>> +remote_upload_trace_default_collest (char **collectp)
>>
>>
>> Comments to this function are needed as well.
>
> Fixed.
>
>
> /* Download COLLECT that is default collect string to target.  */
>
> static void
> remote_download_tracepoint_default_collect (char *collect)
>
>>
>>> +/* Not overwrite default collect If COLLECT is NULL or its size is 0.  */
>>> +
>>> +void
>>> +trace_set_default_collect (char *collect)
>>> +{
>>> +  if (collect != NULL && strlen (collect) != 0)
>>> +    {
>>> +      xfree (default_collect);
>>> +      default_collect = collect;
>>> +    }
>>> +}
>>
>>
>> It looks incorrect to me.  When we change target to a tfile, in which the
>> default-collect is "", the variable default_collect won't be updated.
>>
>> Supposing we have two tfile actions.tf and actions1.tf, the default-collect
>> is "$regs" and "" respectively,
>>
>> (gdb) target tfile testsuite/gdb.trace/actions.tf
>> (gdb) show default-collect
>> The list of expressions to collect by default is "$regs".
>> (gdb) target tfile testsuite/gdb.trace/actions1.tf
>> (gdb) show default-collect
>> The list of expressions to collect by default is "$regs".
>>
>> I don't have other comments.
>
> Agree with you.  Updated patch for it.
>
>>
>> --
>> Yao (齐尧)
>
> On Thu, Apr 11, 2013 at 10:08 PM, Abid, Hafiz <hafiz_abid@mentor.com> wrote:
>> Hi Hui,
>>>
>>> +  putpkt (buf);
>>> +  remote_get_noisy_reply (&target_buf, &target_buf_size);
>>> +  if (strcmp (target_buf, "OK"))
>>> +    warning (_("\
>>
>>
>> Just a small comment on style. I think it is better to be explicit in such
>> case and write != 0. Also I was advised by Pedro recently that new packets
>> should be using packet_ok which will eliminate the need for strcmp here
>> anyway.
>>
>
> Update there part use packet_ok.
>
>>> +  putpkt ("qTDC");
>>> +  getpkt (&rs->buf, &rs->buf_size, 0);
>>> +  if (strncmp (rs->buf, "DC", 2))
>>> +    {
>>
>> Why not remote_get_noisy_reply here.
>
> Changed.  And I also add "PACKET_qTDC" for this part.  But I am not
> sure set "remote_protocol_packets[PACKET_qTDC].support" inside the
> function is a good choice or not.
>
>>
>>
>> Regards,
>> Abid
>
>
> Post a new version for that.  Please help me review it.
>
> Best,
> Hui
>
>
> 2013-04-16  Hui Zhu  <hui@codesourcery.com>
>
> * remote.c (remote_upload_trace_default_collect): New static.
> (PACKET_QTDDCsrc, PACKET_qTDC): New.
> (remote_start_remote): Call remote_upload_trace_default_collect
> and trace_set_default_collect.
> (remote_download_tracepoint_default_collect): New.
> (remote_can_download_tracepoint): Set
> to_download_tracepoint_default_collect.
> (remote_upload_trace_default_collect): New.
> (_initialize_remote): Add QTDDCsrc and qTDC.
> * target.c (update_current_target): Add
> to_download_tracepoint_default_collect.
> * target.h (target_ops): to_download_tracepoint_default_collect.
> (target_download_tracepoint_default_collect): New.
> * tracepoint.c (start_tracing): Call
> target_download_tracepoint_default_collect.
> (trace_set_default_collect): New.
> * tracepoint.h (trace_set_default_collect): New extern.