[PATCH 1/5] Refactor 'tsave'

[Pedro Alves]

On 03/01/2013 03:55 PM, Yao Qi wrote:
> 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.

But they are not at present.  If some future change changes this,
let's discuss it then.  target_get_raw_trace_data calls qTBuffer,
which, as documented, sends data in the format described in the
trace file format node in the manual.  That's it.  Some comments
around the target_get_raw_trace_data/write->ops->write_raw_data
calls indicating that would have made the code much clearer
for me at first glance.  Not so anymore of course, since
I've now spent a long time discussing and staring at the code.

Sorry, I don't know how to better explain it.  This argument
is going nowhere.  Let's move on, and I'll see about adding
some comments afterwards.

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

So the target sends a different buffer format to gdb,
using the same qTBuffer packet?  How will older GDB's
cope?  The packet is explicitly defined as returning
a buffer "as per the trace file format".  I'd imagine
that GDB itself will need to explicitly request the
data in the format it knows the target supports.

So again (the example code above showed this example),
if the XML file describes a format called FOO, and the
writer is CTF, how can

    if (writer->ops->write_raw_data != NULL)
     {
       gotten = target_get_raw_trace_data (buf, offset,
                           MAX_TRACE_UPLOAD);
       if (gotten == 0)
         break;
       writer->ops->write_raw_data (writer, buf, gotten);

work?  What's there in the design to convert the raw FOO to
raw CTF?

(Ignoring the old-gdb compat issue, ) I could see something like:

    if (writer->ops->write_raw_data != NULL
+        && (target_raw_trace_data_format ()
+            == writer->ops->raw_trace_data_format ()))
     {
       gotten = target_get_raw_trace_data (buf, offset,
                           MAX_TRACE_UPLOAD);
       if (gotten == 0)
         break;
       writer->ops->write_raw_data (writer, buf, gotten);
     }
   else
     {
+        if (target_raw_trace_format () == FOO)
+          {
+             /* parse in FOO format and push frame pieces to writer */
+	     gotten = target_get_raw_trace_data (buf, ...);
+             ...
+          }
        else if (target_raw_trace_format () == TFILE)
          {
            /* The target's frame buffer is in GDB's trace file
               frame format.  */

            // The existing fallback code that parses tfile-format buffer
            // goes here.
          }
     }


But we're not there yet.  Adding some comments around

  /* target_get_raw_trace_data returns a frame buffer in
     GDB's trace file frame format.  */

near this code is all I was asking, really.  If in future this
is no longer true, then let's discuss it in the future.

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

No, I'm not mixing it up.  I'm just showing a design based on
different target_ops and remote protocol entry points for each
format to be returned by the remote side.
In the snippet I posted, just remove the else, and gdb would
fall through trying all formats.  Or replace different entry
points with some enumeration identifying the format.  It ends
up being the same.

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

Sure.  It's the parser vs writer format mismatch that I was
concerned about.

>>
>>> >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?

No, I'm fine with "raw data" now, an actually prefer it over
that, as it matches the target method.  Leave it as is, I'll
see about adding some comments afterwards.

-- 
Pedro Alves