[PATCH 1/5] Refactor 'tsave'

[Yao Qi]

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.  It is only about how data are stored in 
trace buffers.  See the comments on write_raw_data,

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

I'd like to differentiate the "raw data" stored in trace buffers and the 
data finally saved into files.

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.

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

>> >+	    }
>> >+
>> >+	  if (tf_size > 0)
>> >+	    {
>> >+	      offset += 6;
>> >+	      gotten = target_get_raw_trace_data (buf, offset,
>> >+						  read_length);
>> >+	      gdb_assert (gotten >= read_length);
> An assertion here doesn't seem appropriate, as this is handling input.
> Moreover we have:
>
> /* This is basically a memory transfer, but needs to be its own packet
>     because we don't know how the target actually organizes its trace
>     memory, plus we want to be able to ask for as much as possible, but
>     not be unhappy if we don't get as much as we ask for.  */
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> static LONGEST
> remote_get_raw_trace_data (gdb_byte *buf, ULONGEST offset, LONGEST len)
> {
>

The first sentence of header comment is "because we don't know how the 
target actually organizes its trace memory", however, we know it here, 
because we've got the size of the traceframe.  The 'gotten' should be 
greater than or equal to the size of the traceframe.

Considering that the remote target may return something wrong, it is too 
aggressive to use an assert here.  In the new version, I change it to 
throwing an error.


>> >+  /* 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?  The basic sentence pattern is "write (an intransitive 
verb) to do sth.", and "[that] the definition part is end" is a noun clause.

-- 
Yao (齐尧)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Refactor-tsave.patch
Type: text/x-patch
Size: 24918 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20130301/88931b8d/attachment.bin>