Re: [PATCH] Tracepoint source strings

[Stan Shebs <stan@codesourcery.com>]

Eli Zaretskii wrote:
>>   struct breakpoint *
>>   create_tracepoint_from_upload (struct uploaded_tp *utp)
>>   {
>> !   char *addr_str, small_buf[100];
>>   [...]
>> !       sprintf (small_buf, "*%s", hex_string (utp->addr));
>>     
>
> Tz-tz-tz... Using a constant-size buffer in sprintf without any check
> for overflow?  Are you sure that calling the buffer ``small'' will
> magically keep you from trouble? ;-)
>   

Presumably even a hypothetical future 128-bit address won't need more 
than 65 chars to print. :-)

>> !   if (utp->cond && !utp->cond_string)
>> !     warning ("Uploaded tracepoint %d condition has no source form, ignoring it",
>>     
>
> What about _() ?
>
>   

Oops, yeah.

>
>> + void
>> + encode_source_string (int tpnum, ULONGEST addr,
>> + 		      char *srctype, char *src, char *buf)
>> + {
>> +   sprintf (buf, "%x:%s:%s:%x:%x:",
>> + 	   tpnum, phex_nz (addr, sizeof (addr)), srctype, 0, (int) strlen (src));
>>     
>
> Again, sprintf on a buffer whose size is not even known.
>   

Hmmm, I'll see what I can do.

>>     written = fwrite ("\x7fTRACE0\n", 8, 1, fp);
>> !   if (written < 8)
>>       perror_with_name (pathname);
>>   
>>     /* Write descriptive info.  */
>> --- 2468,2474 ----
>>        binary file, plus a hint as what this file is, and a version
>>        number in case of future needs.  */
>>     written = fwrite ("\x7fTRACE0\n", 8, 1, fp);
>> !   if (written < 1)
>>       perror_with_name (pathname);
>>     
>
> Why did you change this to accept partial writes?
>   

I was hoping to fix a major brain cramp of mine without anybody noticing 
- oh well. :-)  The two numeric arguments to fwrite are semi-redundant a 
la calloc, and the return result is based on the *second* argument, 
which is the number of "items".  The other way to fix is to swap the two 
arguments; about the only advantage of this way is that we always test 
against 1, instead of repeating the fwrite argument.

>> + Specify a source string of tracepoint @var{n} at address @var{var}.
>>     
>                                                              ^^^^^^^^^
> You meant @var{addr}, I presume.
>
> Anyway, can we have several tracepoints with the same number?  If not,
> why do we need to give the address as well?
>   

Yes and yes.

>>                                       such as @samp{cond} for the
>> + conditional expression
>>     
>
> Conditional expression for what or from where?  I'm guessing this is
> somehow related to the original definition of the tracepoint, but
> please connect the dots more explicitly.
>   

Yes, it's the tracepoint's condition, I can make an xref.  I tend to 
think we can be a little more abbreviated when describing the protocol, 
because presumably anybody writing a target agent is going to know GDB 
concepts pretty well.

>> + @value{GDBN} issues a separate packet, in order, for each command in a
>> + list.
>>     
>
> What list?
>   

Command list.

>>          The target does not need to do anything with source strings
>> + except report them back as part of the @samp{qTfP}/@samp{qTsP}
>> + queries.
>>     
>
> "As part of queries" or as part or _responses_?
>   

As part of replies.  I've glossed over details of the qT[fs]P reply to 
date, so as to not to have to rewrite so much as the rest of the 
tracepoint patches go in.  The grand plan is that there is a notion of 
"tracepoint description" with a syntax that is common between remote 
download, remote upload, and trace file, and it should go into its own 
section that is referenced from both protocol and file format descriptions.

Stan