Re: [PATCH 1/5] Refactor 'tsave'

[Yao Qi <yao@codesourcery.com>]

On 03/01/2013 08:13 PM, Pedro Alves wrote:
>> I never say "raw data is binary data or raw data is format-less data".
>> >My point is raw data has its own format, and GDB is free to store them
>> >directly by write_raw_data or parse them (according to how data are stored in trace buffers) first and store them in some
>> >format (CTF and even TFILE).
> The patch clearly assumes the data stored in the trace buffers

Yes, I read some gdbserver code when writing this patch.

> is as described in the manual, in the "Trace File Format" node,
> as I pointed out above.  ("The trace frame section ...").
>

No, the "Trace File Format" node is about trace file format, not trace
buffer format.  Two formats are the same, but can be different.

>> >
>>>>> >>> >
>>>>> >>> >TFILE is a format that composed by ascii definition part and trace frames dumped from the raw data directly.  There could be another trace file format FOO that stores raw data as well.
>>> >>It's not "raw".  It's trace frames in gdb's trace file format
>>> >>as defined in the manual.
>>> >>
>> >
>> >It is just because TFILE format use the same format that data are
>> >stored in trace buffers.  One day, we can use another way to organize
>> >the trace buffers, but still output TFILE format in default.  For example, we may
>> >add checksum for each block in trace buffers in GDBserver in the future,
>> >but still generate TFILE format trace file in GDB side.  When we get raw
>> >data (with checksum in each block) from the remote target, can we call
>> >them "tfile format data"?.  Another example is GDBserver can send the format
>> >of trace buffers to GDB first via xml, and GDB can parse the data got
>> >from GDBserver according the xml description, and save them into CTF or
>> >TFILE format.  We can't call the data from the remote target "tfile format data".
> At the point there's more than one format that GDB can fetch from the
> target, then the code that fetches it will need to know what format it is
> getting.  Say, nothing would change in the patch, then ...
>
>        LONGEST gotten = 0;
>
>        if (writer->ops->write_raw_data != NULL)
> 	{
> 	  gotten = target_get_raw_trace_data (buf, offset,
> 					      MAX_TRACE_UPLOAD);
>                     ^^^^^^^^^^^^^^^^^^^^^^^^
>                say this sometimes returns XML.
> 	  if (gotten == 0)
> 	    break;
> 	  writer->ops->write_raw_data (writer, buf, gotten);
>                         ^^^^^^^^^^^^^^
>
> what format is the writer getting?  How can it tell?

When GDB connects to GDBserver, GDB can get an XML file to describe the 
format of trace buffers, we call it 'trace buffer description', similar 
to target description we are using to describe registers.  Then, GDB is 
able to generate the corresponding writer according the XML file, and 
use the write to write trace file.

target_get_raw_trace_data doesn't have to know the format, because it is 
transferring memory from the target to GDB.

>
> 	}
>
> ... this leaves gdb or the writer with no idea what "kind"
> of "raw" data it is getting.  It may not understand or
> want XML at all!
>
> Clearly, more code will need to be added here to handle
> the multiple types, and the possibility that the format
> we get from the target is not the same as the writer wants.
>
> Maybe we'll have then:
>
>        /* See if we can pass down raw trace buffer trace frames
>           directly, as per FOO's trace file format, if both the target
>           and the writer support that format.  */
>        if (writer->ops->write_raw_FOO_data != NULL)
> 	{
> 	  gotten = target_get_raw_FOO_trace_data (buf, offset,
> 					      MAX_TRACE_UPLOAD);
> 	  if (gotten == 0)
> 	    break;
> 	  writer->ops->write_raw_FOO_data (writer, buf, gotten);
>          }
>
>        /* Nope.  See if we can pass down raw trace buffer trace frames
>           directly, as per GDB's trace file format, if both the target
>           and the writer support that format.  */
>        else if (writer->ops->write_raw_data != NULL)
> 	{
> 	  gotten = target_get_raw_trace_data (buf, offset,
> 					      MAX_TRACE_UPLOAD);
>                say this returns XML.
> 	  if (gotten == 0)
> 	    break;
> 	  writer->ops->write_raw_data (writer, buf, gotten);
>          }
>
>        /* Nope.  We'll need to do trace buffer format conversion.  Try
>           format foo first, as it is more complete.  */
>         else
>           {
>
>              ... if possible, fetch data from the target in foo format,
> 	        parse it, and call writer hooks with the pieces ...
>
>              ...
>
>              ... else if possible, fetch frame data from the target in gdb
> 		trace file format, parse it, and call writer hooks
> 		with the pieces ...
>           }
>
> See, the comments should make all this much clearer.  That's really
> all I'm asking -- to make this bit of code a bit clearer.

It doesn't look right to me, because you are mixing up 'trace file 
writer' together with 'trace buffer parser'.  GDB can support multiple 
'trace buffer parser', and uses one selected parser to parse the data 
get from target_get_raw_trace_data.  GDB can also support multiple 
'trace file writer', and uses one selected writer to a specific trace 
file format.  Before these patches, GDB has one writer (tfile writer) 
and one parser, and writer and parser is hard-coded into GDB source. 
After these patches, GDB will have one parser and two writers (tfile 
writer and ctf writer).  The parser should be determined by the reply 
from the remote target, and the writer should be determined by the 
command setting.

If writer supports "write_raw_data", the parser won't parse the data at 
all, and call "write_raw_data" to write data to disk directly.  If the 
writer doesn't supports "write_raw_data", the parser has to parse the 
data, break data into pieces, and feed the small piece to writer.

>
>> >
>>>>> >>> >
>>>>>>>>>>> >>>>>> >>> >+    {
>>>>>>>>>>> >>>>>> >>> >+      uint16_t tp_num;
>>>>>>>>>>> >>>>>> >>> >+      uint32_t tf_size;
>>>>>>>>>>> >>>>>> >>> >+      unsigned int read_length;
>>>>>>>>>>> >>>>>> >>> >+      unsigned int block;
>>>>>>>>>>> >>>>>> >>> >+
>>>>>>>>>>> >>>>>> >>> >+      /* Read the first six bytes in, which is the tracepoint
>>>>>>>>>>> >>>>>> >>> >+         number and trace frame size.  */
>>>>>>>>>>> >>>>>> >>> >+      gotten = target_get_raw_trace_data (buf, offset, 6);
>>>>>>>>>>> >>>>>> >>> >+
>>>>>>>>>>> >>>>>> >>> >+      if (gotten == 0)
>>>>>>>>>>> >>>>>> >>> >+        break;
>>>>>>>>>>> >>>>>> >>> >+      tp_num = (uint16_t)
>>>>>>>>>>> >>>>>> >>> >+        extract_unsigned_integer (&buf[0], 2, byte_order);
>>>>>>>>>>> >>>>>> >>> >+
>>>>>>>>>>> >>>>>> >>> >+      tf_size = (uint32_t)
>>>>>>>>>>> >>>>>> >>> >+        extract_unsigned_integer (&buf[2], 4, byte_order);
>>> >>(**) ... as can be seen here and below with
>>> >>
>>> >>+          switch (block_type)
>>> >>+            {
>>> >>+            case 'R':
>>> >>...
>>> >>+            case 'M':
>>> >>...
>>> >>
>>> >>etc.
>>> >>
>>> >>This is parsing the "raw"'s headers, in gdb's trace file format.
>>> >>This as the else branch.  The "then" branch only works if the target
>>> >>can interpret "buf" as trace frames in gdb's trace file format.
>>> >>
>> >Again, it is not trace file format.
> Again, it is.
>
>> >It is how data are stored in trace buffers.
> It is, because the the target passes the trace buffers in
> trace file format back to gdb.  gdbserver uses the same format
> natively, for its own buffers, as naturally this avoid any
> conversion.
>
> (actually, historically, the tfile format was designed as
> reusing gdbserver's, but anyway, same difference).

The term "raw data" makes trouble here, so are you happy if I write the 
code in this way?

struct trace_file_write_ops
{
   ...
   /* Write the data of trace buffer without parsing.  The content is
      in BUF and length is LEN.  */
   void (*write_trace_buffer) (struct trace_file_writer *self,
			      gdb_byte *buf, LONGEST len);

   /* Operations to write trace frames.  The user of this field is
      responsible to parse the data of trace buffer.  Either field
      'write_trace_buffer' or field ' frame_ops' is NULL.  */
   struct trace_frame_write_ops *frame_ops;

   ...
};


in trace_save,

       /* The writer supports writing contents of trace buffer directly
	 to trace file.  Don't parse the contents of trace buffer.  */
       if (writer->ops->write_trace_buffer != NULL)
	{
	  gotten = target_get_raw_trace_data (buf, offset,
					      MAX_TRACE_UPLOAD);
	  if (gotten == 0)
	    break;
	  writer->ops->write_trace_buffer (writer, buf, gotten);
	}
       else
	{

-- 
Yao (齐尧)