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

[Pedro Alves <palves@redhat.com>]

On 03/01/2013 08:57 AM, Yao Qi wrote:
> Hi Pedro,
> thanks for the quick review.  The updated patch address most of your comments except some I'd like to discuss with you below,
> 
> On 03/01/2013 12:33 AM, Pedro Alves wrote:
>> Ok, I think I got it.
>>
>> I think an important missing clue here is that "raw" in
>> "write_raw_data" and target_get_raw_trace_data actually means
>> frames in "tfile" format, right?  "ops->write_raw_data" being
>> NULL means that the writer can't handle parsing the "tfile"
>> frames itself.
>>
>> "raw" here is almost meaningless to the reader -- surely "raw" in
>> the ctf context could mean raw ctf "something".
>> Maybe "raw" should really be s/raw/tfile/ or s/raw/raw_tfile/ ?
>>
> 
> "raw" here means the data in the trace buffers, and "raw data" has few to 
> do with the file format.

And what is the format of that "raw" data?  Exactly the format
described in the manual, in the "Trace File Format" node:

"The trace frame section consists of a number of consecutive frames.
Each frame begins with a two-byte tracepoint number, followed by a
four-byte size giving the amount of data in the frame.  The data in
the frame consists of a number of blocks, each introduced by a
character indicating its type (at least register, memory, and trace
state variable).  The data in this section is raw binary, not a
hexadecimal or other encoding; its endianness matches the target's
endianness."

So it's not really "raw" binary data -- there's a format, with
headers and all.  And it can't be any other format, otherwise
the patch's design won't work... (**)

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

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

>>> >+
>>> >+      writer->ops->frame_ops->start (writer, tp_num);
>>> >+      gotten = 6;
>>> >+
>>> >+      if (tf_size <= MAX_TRACE_UPLOAD)
>>> >+        read_length = tf_size;
>>> >+      else
>>> >+        {
>>> >+          read_length = MAX_TRACE_UPLOAD;
>>> >+          gdb_assert (0);
>> Leftover?  I guess this means you didn't try with a big trace
>> frame?
>>
> 
> I change this part that GDB will skip the frame if it is too big, and print a warning, like this:
> 
>   warning: Skip this trace frame because its size (708) is greater than the size of GDB internal buffer (200)
> 
> I set MAX_TRACE_UPLOAD to 200, and run the testsuite.  The CTF file is still correct, but some frames are lost.

Ugh.  Why do we have this limitation at all?
target_get_raw_trace_data works with an offset and size, so
I don't even see why we need to fetch the whole trace frame
in one go.

> 
>>> >+        }
>>> >+


>>> >+  /* Write to mark the definition part is end.  */
>> This doesn't parse correctly.  Something like:
>>
>>    /* Called to signal the end of the definition part.  */
>>
>> perhaps.
>>
> 
> Is it incorrect?  

I think so.

> The basic sentence pattern is "write (an intransitive verb) to do sth.", and "[that] the definition part is end" is a noun clause.

"to signal" is not intransitive.  But I don't think an
intransitive verb is what you wanted anyway.

"is end" isn't correct.

"is over", or "is done" or "is/has finished" or "has ended"
would be good.  By my order of preference:

 "Write to mark the end of the definition part."
 "Write to mark the definition part's end."
 "Write to mark the definition part has ended."

Though "Write to mark" still sounds a little odd to me.
It sounds to me like:

"Write (anything at all you wish, unrelated to the
definition part having ended.  Perhaps a poem.)
as celebration to mark the annual event that the
definition part has ended.  :-)

But one of the above is good enough for me.
Unless a native speaker chimes in, let's leave it at that.

-- 
Pedro Alves