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

[Hui Zhu <teawater at gmail dot com>]

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.

Attachment: defc-remote.txt
Description: Text document