This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/5] Refactor 'tsave'
On 02/27/2013 02:17 AM, Yao Qi wrote:
> Hello,
> The existing GDB code on saving trace data to trace file is highly
> related to TFILE format, which makes some difficulties on supporting a
> new trace file format, such as CTF. It is idea to de-couple trace
> saving with actual trace file format. This is what this patch does.
> With this patch applied, "tsave" command uses some operations vectors
> to write data and the logic in "tsave" is not related to any specific
> trace format.
>
> gdb:
>
> 2013-02-27 Yao Qi <yao@codesourcery.com>
>
> * tracepoint.c (trace_file_writer_xfree): New.
> (struct tfile_writer_data): New.
> (tfile_can_target_save, tfile_start): New.
> (tfile_write_header, tfile_write_regblock_type): New.
> (tfile_write_status, tfile_write_uploaded_tsv): New.
> (tfile_write_uploaded_tp, tfile_write_definition_end): New.
> (tfile_write_raw_data, (tfile_end): New.
> (tfile_write_ops): New global variable.
> (TRACE_WRITE_R_BLOCK, TRACE_WRITE_M_BLOCK): New macros.
> (TRACE_WRITE_V_BLOCK): New macro.
> (trace_save): Add extra one parameter WRITER. Make it static.
> Use WRITER to writer trace.
> (trace_save_command): Caller update.
> (trace_save_tfile): Write trace data in TFILE format.
> * tracepoint.h (struct trace_frame_write_ops): New.
> (struct trace_file_write_ops): New.
> (struct trace_file_writer): New.
> (trace_save): Remove its declaration.
> (trace_save_tfile): Declare it.
> * mi/mi-main.c (mi_cmd_trace_save): Call trace_save_tfile
> instead of trace_save.
>
> ---
> gdb/mi/mi-main.c | 2 +-
> gdb/tracepoint.c | 534 +++++++++++++++++++++++++++++++++++++++++-------------
> gdb/tracepoint.h | 92 +++++++++-
> 3 files changed, 504 insertions(+), 124 deletions(-)
>
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 37294e0..206b626 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -2495,7 +2495,7 @@ mi_cmd_trace_save (char *command, char **argv, int argc)
> filename = argv[0];
> }
>
> - trace_save (filename, target_saves);
> + trace_save_tfile (filename, target_saves);
> }
>
> void
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index f7a3650..43bd80f 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -2983,90 +2983,317 @@ encode_source_string (int tpnum, ULONGEST addr,
> return -1;
> }
>
> -extern int trace_regblock_size;
> +/* Free trace file writer and its data. */
>
> -/* Save tracepoint data to file named FILENAME. If TARGET_DOES_SAVE is
> - non-zero, the save is performed on the target, otherwise GDB obtains all
> - trace data and saves it locally. */
> +static void
> +trace_file_writer_xfree (void *arg)
> +{
> + struct trace_file_writer *writer = (struct trace_file_writer *) arg;
>
> -void
> -trace_save (const char *filename, int target_does_save)
> + if (writer != NULL)
> + xfree (writer->data);
> +
> + xfree (writer);
As long as you need a NULL check, move both calls under it:
if (writer != NULL)
{
xfree (writer->data);
xfree (writer);
}
> +}
> +
> +/* TFILE writer specific data. */
> +
> +struct tfile_writer_data
> {
> - struct cleanup *cleanup;
> - char *pathname;
> - struct trace_status *ts = current_trace_status ();
> - int err, status;
> + /* File pointer to tfile trace file. */
> FILE *fp;
> - struct uploaded_tp *uploaded_tps = NULL, *utp;
> - struct uploaded_tsv *uploaded_tsvs = NULL, *utsv;
> - int a;
> - char *act;
> - LONGEST gotten = 0;
> - ULONGEST offset = 0;
> -#define MAX_TRACE_UPLOAD 2000
> - gdb_byte buf[MAX_TRACE_UPLOAD];
> - int written;
> + /* Path name of the tfile trace file. */
> + char *pathname;
>
> - /* If the target is to save the data to a file on its own, then just
> - send the command and be done with it. */
> - if (target_does_save)
> - {
> - err = target_save_trace_data (filename);
> - if (err < 0)
> - error (_("Target failed to save trace data to '%s'."),
> - filename);
> - return;
> - }
> + struct cleanup *cleanup;
> +};
>
> - /* Get the trace status first before opening the file, so if the
> - target is losing, we can get out without touching files. */
> - status = target_get_trace_status (ts);
> +/* This is the implementation of trace_file_write_ops method
> + can_target_save. We just call the generic target
> + target_save_trace_data to decide about target-side saving. */
This looks odd to me. The query to check whether the writer
_can_ save, actually saves? What's the point of querying then?
This might just be a naming issue.
> +
> +static int
> +tfile_can_target_save (struct trace_file_writer *self,
> + const char *filename)
> +{
> + int err = target_save_trace_data (filename);
> +
> + return (err >= 0);
> +}
>
> - pathname = tilde_expand (filename);
> - cleanup = make_cleanup (xfree, pathname);
> +/* This is the implementation of trace_file_write_ops method
> + start. It creates the trace file FILENAME and register some
> + cleanups. */
"it (...) registers"
> +
> +static void
> +tfile_start (struct trace_file_writer *self, const char *filename)
> +{
> + struct tfile_writer_data *data = self->data;
>
> - fp = fopen (pathname, "wb");
> - if (!fp)
> + data->pathname = tilde_expand (filename);
> + data->cleanup = make_cleanup (xfree, data->pathname);
> + data->fp = fopen (data->pathname, "wb");
> + if (data->fp == NULL)
> error (_("Unable to open file '%s' for saving trace data (%s)"),
> filename, safe_strerror (errno));
> - make_cleanup_fclose (fp);
> + make_cleanup_fclose (data->fp);
> +}
> +
> +/* This is the implementation of trace_file_write_ops method
> + write_header. Write the TFILE header. */
> +
> +static void
> +tfile_write_header (struct trace_file_writer *self)
> +{
> + struct tfile_writer_data *data = self->data;
> + int written;
>
> /* Write a file header, with a high-bit-set char to indicate a
> 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);
> + written = fwrite ("\x7fTRACE0\n", 8, 1, data->fp);
> if (written < 1)
> - perror_with_name (pathname);
> + perror_with_name (data->pathname);
> +}
>
> - /* Write descriptive info. */
> +/* This is the implementation of trace_file_write_ops method
> + write_regblock_type. Write the size of register block. */
>
> - /* Write out the size of a register block. */
> - fprintf (fp, "R %x\n", trace_regblock_size);
> +static void
> +tfile_write_regblock_type (struct trace_file_writer *self, int size)
> +{
> + struct tfile_writer_data *data = self->data;
>
> - /* Write out status of the tracing run (aka "tstatus" info). */
> - fprintf (fp, "status %c;%s",
> + fprintf (data->fp, "R %x\n", size);
> +}
> +
> +/* This is the implementation of trace_file_write_ops method
> + write_status. */
> +
> +static void
> +tfile_write_status (struct trace_file_writer *self,
> + struct trace_status *ts)
> +{
> + struct tfile_writer_data *data = self->data;
> +
> + fprintf (data->fp, "status %c;%s",
> (ts->running ? '1' : '0'), stop_reason_names[ts->stop_reason]);
> if (ts->stop_reason == tracepoint_error)
> {
> char *buf = (char *) alloca (strlen (ts->stop_desc) * 2 + 1);
>
> bin2hex ((gdb_byte *) ts->stop_desc, buf, 0);
> - fprintf (fp, ":%s", buf);
> + fprintf (data->fp, ":%s", buf);
> }
> - fprintf (fp, ":%x", ts->stopping_tracepoint);
> + fprintf (data->fp, ":%x", ts->stopping_tracepoint);
> if (ts->traceframe_count >= 0)
> - fprintf (fp, ";tframes:%x", ts->traceframe_count);
> + fprintf (data->fp, ";tframes:%x", ts->traceframe_count);
> if (ts->traceframes_created >= 0)
> - fprintf (fp, ";tcreated:%x", ts->traceframes_created);
> + fprintf (data->fp, ";tcreated:%x", ts->traceframes_created);
> if (ts->buffer_free >= 0)
> - fprintf (fp, ";tfree:%x", ts->buffer_free);
> + fprintf (data->fp, ";tfree:%x", ts->buffer_free);
> if (ts->buffer_size >= 0)
> - fprintf (fp, ";tsize:%x", ts->buffer_size);
> + fprintf (data->fp, ";tsize:%x", ts->buffer_size);
> if (ts->disconnected_tracing)
> - fprintf (fp, ";disconn:%x", ts->disconnected_tracing);
> + fprintf (data->fp, ";disconn:%x", ts->disconnected_tracing);
> if (ts->circular_buffer)
> - fprintf (fp, ";circular:%x", ts->circular_buffer);
> - fprintf (fp, "\n");
> + fprintf (data->fp, ";circular:%x", ts->circular_buffer);
> + fprintf (data->fp, "\n");
> +}
> +
> +/* This is the implementation of trace_file_write_ops method
> + write_uploaded_tsv. */
> +
> +static void
> +tfile_write_uploaded_tsv (struct trace_file_writer *self,
> + struct uploaded_tsv *utsv)
> +{
> + char *buf = "";
> + struct tfile_writer_data *data = self->data;
> +
> + if (utsv->name)
> + {
> + buf = (char *) xmalloc (strlen (utsv->name) * 2 + 1);
> + bin2hex ((gdb_byte *) (utsv->name), buf, 0);
> + }
> +
> + fprintf (data->fp, "tsv %x:%s:%x:%s\n",
> + utsv->number, phex_nz (utsv->initial_value, 8),
> + utsv->builtin, buf);
> +
> + if (utsv->name)
> + xfree (buf);
> +}
> +
> +#define MAX_TRACE_UPLOAD 2000
> +
> +/* This is the implementation of trace_file_write_ops method
> + write_uploaded_tp. */
> +
> +static void
> +tfile_write_uploaded_tp (struct trace_file_writer *self,
> + struct uploaded_tp *utp)
> +{
> + struct tfile_writer_data *data = self->data;
> + int a;
> + char *act;
> + gdb_byte buf[MAX_TRACE_UPLOAD];
> +
> + fprintf (data->fp, "tp T%x:%s:%c:%x:%x",
> + utp->number, phex_nz (utp->addr, sizeof (utp->addr)),
> + (utp->enabled ? 'E' : 'D'), utp->step, utp->pass);
> + if (utp->type == bp_fast_tracepoint)
> + fprintf (data->fp, ":F%x", utp->orig_size);
> + if (utp->cond)
> + fprintf (data->fp,
> + ":X%x,%s", (unsigned int) strlen (utp->cond) / 2,
> + utp->cond);
> + fprintf (data->fp, "\n");
> + for (a = 0; VEC_iterate (char_ptr, utp->actions, a, act); ++a)
> + fprintf (data->fp, "tp A%x:%s:%s\n",
> + utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act);
> + for (a = 0; VEC_iterate (char_ptr, utp->step_actions, a, act); ++a)
> + fprintf (data->fp, "tp S%x:%s:%s\n",
> + utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act);
> + if (utp->at_string)
> + {
> + encode_source_string (utp->number, utp->addr,
> + "at", utp->at_string, buf, MAX_TRACE_UPLOAD);
> + fprintf (data->fp, "tp Z%s\n", buf);
> + }
> + if (utp->cond_string)
> + {
> + encode_source_string (utp->number, utp->addr,
> + "cond", utp->cond_string,
> + buf, MAX_TRACE_UPLOAD);
> + fprintf (data->fp, "tp Z%s\n", buf);
> + }
> + for (a = 0; VEC_iterate (char_ptr, utp->cmd_strings, a, act); ++a)
> + {
> + encode_source_string (utp->number, utp->addr, "cmd", act,
> + buf, MAX_TRACE_UPLOAD);
> + fprintf (data->fp, "tp Z%s\n", buf);
> + }
> + fprintf (data->fp, "tp V%x:%s:%x:%s\n",
> + utp->number, phex_nz (utp->addr, sizeof (utp->addr)),
> + utp->hit_count,
> + phex_nz (utp->traceframe_usage,
> + sizeof (utp->traceframe_usage)));
> +}
> +
> +/* This is the implementation of trace_file_write_ops method
> + write_definition_end. */
> +
> +static void
> +tfile_write_definition_end (struct trace_file_writer *self)
> +{
> + struct tfile_writer_data *data = self->data;
> +
> + fprintf (data->fp, "\n");
> +}
> +
> +/* This is the implementation of trace_file_write_ops method
> + write_raw_data. */
> +
> +static void
> +tfile_write_raw_data (struct trace_file_writer *self, gdb_byte *buf,
> + LONGEST len)
> +{
> + struct tfile_writer_data *data = self->data;
> +
> + if (fwrite (buf, len, 1, data->fp) < 1)
> + perror_with_name (data->pathname);
> +}
> +
> +/* This is the implementation of trace_file_write_ops method
> + end. */
> +
> +static void
> +tfile_end (struct trace_file_writer *self)
> +{
> + struct tfile_writer_data *data = self->data;
> + uint32_t gotten = 0;
> +
> + /* Mark the end of trace data. */
> + if (fwrite (&gotten, 4, 1, data->fp) < 1)
> + perror_with_name (data->pathname);
> +
> + do_cleanups (data->cleanup);
> +}
> +
> +/* Operations to write trace buffers into TFILE format. */
> +
> +static struct trace_file_write_ops tfile_write_ops =
> +{
> + tfile_can_target_save,
> + tfile_start,
> + tfile_write_header,
> + tfile_write_regblock_type,
> + tfile_write_status,
> + tfile_write_uploaded_tsv,
> + tfile_write_uploaded_tp,
> + tfile_write_definition_end,
> + tfile_write_raw_data,
> + NULL,
> + tfile_end,
> +};
> +
> +/* Helper macros. */
> +
> +#define TRACE_WRITE_R_BLOCK(writer, buf, size) \
> + writer->ops->frame_ops->write_r_block ((writer), (buf), (size))
> +#define TRACE_WRITE_M_BLOCK(writer, addr, buf, size) \
> + writer->ops->frame_ops->write_m_block ((writer), (addr), (buf), \
> + (size))
> +#define TRACE_WRITE_V_BLOCK(writer, num, val) \
> + writer->ops->frame_ops->write_v_block ((writer), (num), (val))
> +
> +extern int trace_regblock_size;
> +
> +/* Save tracepoint data to file named FILENAME through WRITER. WRITER
> + determines the trace file format. If TARGET_DOES_SAVE is non-zero,
> + the save is performed on the target, otherwise GDB obtains all trace
> + data and saves it locally. */
> +
> +static void
> +trace_save (const char *filename, struct trace_file_writer *writer,
> + int target_does_save)
> +{
> + struct trace_status *ts = current_trace_status ();
> + int status;
> + struct uploaded_tp *uploaded_tps = NULL, *utp;
> + struct uploaded_tsv *uploaded_tsvs = NULL, *utsv;
> +
> + ULONGEST offset = 0;
> + gdb_byte buf[MAX_TRACE_UPLOAD];
> + int written;
> + enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
> +
> + /* If the target is to save the data to a file on its own, then just
> + send the command and be done with it. */
> + if (target_does_save)
> + {
> + if (!writer->ops->can_target_save (writer, filename))
> + error (_("Target failed to save trace data to '%s'."),
> + filename);
> + return;
> + }
> +
> + /* Get the trace status first before opening the file, so if the
> + target is losing, we can get out without touching files. */
> + status = target_get_trace_status (ts);
> +
> + writer->ops->start (writer, filename);
> +
> + writer->ops->write_header (writer);
> +
> + /* Write descriptive info. */
> +
> + /* Write out the size of a register block. */
> + writer->ops->write_regblock_type (writer, trace_regblock_size);
> +
> + /* Write out status of the tracing run (aka "tstatus" info). */
> + writer->ops->write_status (writer, ts);
>
> /* Note that we want to upload tracepoints and save those, rather
> than simply writing out the local ones, because the user may have
> @@ -3081,22 +3308,7 @@ trace_save (const char *filename, int target_does_save)
> target_upload_trace_state_variables (&uploaded_tsvs);
>
> for (utsv = uploaded_tsvs; utsv; utsv = utsv->next)
> - {
> - char *buf = "";
> -
> - if (utsv->name)
> - {
> - buf = (char *) xmalloc (strlen (utsv->name) * 2 + 1);
> - bin2hex ((gdb_byte *) (utsv->name), buf, 0);
> - }
> -
> - fprintf (fp, "tsv %x:%s:%x:%s\n",
> - utsv->number, phex_nz (utsv->initial_value, 8),
> - utsv->builtin, buf);
> -
> - if (utsv->name)
> - xfree (buf);
> - }
> + writer->ops->write_uploaded_tsv (writer, utsv);
>
> free_uploaded_tsvs (&uploaded_tsvs);
>
> @@ -3106,76 +3318,133 @@ trace_save (const char *filename, int target_does_save)
> target_get_tracepoint_status (NULL, utp);
>
> for (utp = uploaded_tps; utp; utp = utp->next)
> - {
> - fprintf (fp, "tp T%x:%s:%c:%x:%x",
> - utp->number, phex_nz (utp->addr, sizeof (utp->addr)),
> - (utp->enabled ? 'E' : 'D'), utp->step, utp->pass);
> - if (utp->type == bp_fast_tracepoint)
> - fprintf (fp, ":F%x", utp->orig_size);
> - if (utp->cond)
> - fprintf (fp, ":X%x,%s", (unsigned int) strlen (utp->cond) / 2,
> - utp->cond);
> - fprintf (fp, "\n");
> - for (a = 0; VEC_iterate (char_ptr, utp->actions, a, act); ++a)
> - fprintf (fp, "tp A%x:%s:%s\n",
> - utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act);
> - for (a = 0; VEC_iterate (char_ptr, utp->step_actions, a, act); ++a)
> - fprintf (fp, "tp S%x:%s:%s\n",
> - utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act);
> - if (utp->at_string)
> - {
> - encode_source_string (utp->number, utp->addr,
> - "at", utp->at_string, buf, MAX_TRACE_UPLOAD);
> - fprintf (fp, "tp Z%s\n", buf);
> - }
> - if (utp->cond_string)
> - {
> - encode_source_string (utp->number, utp->addr,
> - "cond", utp->cond_string,
> - buf, MAX_TRACE_UPLOAD);
> - fprintf (fp, "tp Z%s\n", buf);
> - }
> - for (a = 0; VEC_iterate (char_ptr, utp->cmd_strings, a, act); ++a)
> - {
> - encode_source_string (utp->number, utp->addr, "cmd", act,
> - buf, MAX_TRACE_UPLOAD);
> - fprintf (fp, "tp Z%s\n", buf);
> - }
> - fprintf (fp, "tp V%x:%s:%x:%s\n",
> - utp->number, phex_nz (utp->addr, sizeof (utp->addr)),
> - utp->hit_count,
> - phex_nz (utp->traceframe_usage,
> - sizeof (utp->traceframe_usage)));
> - }
> + writer->ops->write_uploaded_tp (writer, utp);
>
> free_uploaded_tps (&uploaded_tps);
>
> /* Mark the end of the definition section. */
> - fprintf (fp, "\n");
> + writer->ops->write_definition_end (writer);
>
> /* Get and write the trace data proper. We ask for big blocks, in
> the hopes of efficiency, but will take less if the target has
> packet size limitations or some such. */
> while (1)
> {
> - gotten = target_get_raw_trace_data (buf, offset, MAX_TRACE_UPLOAD);
> + LONGEST gotten = 0;
> +
> + if (writer->ops->write_raw_data != NULL)
I got confused while reading this. As it stands, why do we
need this check?
> + {
> + gotten = target_get_raw_trace_data (buf, offset,
> + MAX_TRACE_UPLOAD);
> + if (gotten == 0)
> + break;
> + writer->ops->write_raw_data (writer, buf, gotten);
> + }
> + else
and this whole else block? This didn't seem to exist
before the patch. At least a comment is missing here.
(time passes)
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/ ?
> + {
> + 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?
> + }
> +
> + 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)
{
> +
> + gotten = read_length;
> +
> + for (block = 0; block < read_length; )
> + {
> + gdb_byte block_type = buf[block++];
> +
> + switch (block_type)
> + {
> + case 'R':
> + TRACE_WRITE_R_BLOCK (writer, &buf[block],
> + trace_regblock_size);
> + block += trace_regblock_size;
> + break;
> + case 'M':
> + {
> + unsigned short mlen;
> + ULONGEST addr;
> +
> + addr = (ULONGEST)
> + extract_unsigned_integer (&buf[block], 8,
> + byte_order);
> + block += 8;
> + mlen = (unsigned short)
> + extract_unsigned_integer (&buf[block], 2,
> + byte_order);
> +
> + block += 2;
> + TRACE_WRITE_M_BLOCK (writer, addr,
> + &buf[block], mlen);
> +
> + block += mlen;
> + break;
> + }
> + case 'V':
> + {
> + int vnum
> + = (int) extract_signed_integer (&buf[block],
> + 4,
> + byte_order);
> + LONGEST val
> + = extract_signed_integer (&buf[block + 4],
> + 8,
> + byte_order);
> +
> + block += (4 + 8);
> + TRACE_WRITE_V_BLOCK (writer, vnum, val);
> + }
> + break;
> + default:
> + error (_("Unknown block type '%c' (0x%x) in"
> + " trace frame"),
> + block_type, block_type);
> + }
> + }
> + }
> +
> + writer->ops->frame_ops->end (writer);
> + }
> +
> if (gotten < 0)
> error (_("Failure to get requested trace buffer data"));
> /* No more data is forthcoming, we're done. */
> if (gotten == 0)
> break;
> - written = fwrite (buf, gotten, 1, fp);
> - if (written < 1)
> - perror_with_name (pathname);
> offset += gotten;
> }
>
> - /* Mark the end of trace data. (We know that gotten is 0 at this point.) */
> - written = fwrite (&gotten, 4, 1, fp);
> - if (written < 1)
> - perror_with_name (pathname);
> -
> - do_cleanups (cleanup);
> + writer->ops->end (writer);
> }
>
> static void
> @@ -3185,6 +3454,7 @@ trace_save_command (char *args, int from_tty)
> char **argv;
> char *filename = NULL;
> struct cleanup *back_to;
> + struct trace_file_writer *writer = NULL;
>
> if (args == NULL)
> error_no_arg (_("file in which to save trace data"));
> @@ -3205,7 +3475,12 @@ trace_save_command (char *args, int from_tty)
> if (!filename)
> error_no_arg (_("file in which to save trace data"));
>
> - trace_save (filename, target_does_save);
> + writer = xmalloc (sizeof (struct trace_file_writer));
> + writer->ops = &tfile_write_ops;
> + writer->data = xmalloc (sizeof (struct tfile_writer_data));
I suggest moving these three lines to a new "tfile_writer_data_new"
function that returns a new writer...
> + make_cleanup (trace_file_writer_xfree, writer);
> +
> + trace_save (filename, writer, target_does_save);
>
> if (from_tty)
> printf_filtered (_("Trace data saved to file '%s'.\n"), filename);
> @@ -3213,6 +3488,21 @@ trace_save_command (char *args, int from_tty)
> do_cleanups (back_to);
> }
>
> +/* Save the trace data to file FILENAME of tfile format. */
> +
> +void
> +trace_save_tfile (const char *filename, int target_does_save)
> +{
> + struct trace_file_writer *writer = xmalloc (sizeof (*writer));
> + struct cleanup *back_to;
> +
> + writer->ops = &tfile_write_ops;
> + writer->data = xmalloc (sizeof (struct tfile_writer_data));
... and call it here too.
> +
> + back_to = make_cleanup (trace_file_writer_xfree, writer);
> + do_cleanups (back_to);
> +}
> +
> /* Tell the target what to do with an ongoing tracing run if GDB
> disconnects for some reason. */
>
> diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
> index b2b9d7c..1db6b69 100644
> --- a/gdb/tracepoint.h
> +++ b/gdb/tracepoint.h
> @@ -205,6 +205,95 @@ struct static_tracepoint_marker
> char *extra;
> };
>
> +struct trace_file_writer;
> +
> +/* Operations to write trace frames to a specific trace format. */
> +
> +struct trace_frame_write_ops
> +{
> + /* Write a new trace frame. The tracepoint number of this trace
> + frame is TPNUM. */
> + void (*start) (struct trace_file_writer *self, uint16_t tpnum);
> +
> + /* Write a 'R' block. Buffer BUF contains its content and SIZE is
> + its size. */
"an 'R'". I believe "contents" is more appropriate.
> + void (*write_r_block) (struct trace_file_writer *self,
> + gdb_byte *buf, int size);
> +
> + /* Write a 'M' block. Buffer BUF contains its content and LENGTH is
> + the length of it. ADDR is the start address of collected
> + memory. */
an 'M'.
> + void (*write_m_block) (struct trace_file_writer *self,
> + ULONGEST addr, gdb_byte *buf,
> + uint16_t length);
> +
> + /* Write a 'V' block. NUM is the trace variable number and VAL is
> + the value of the trace variable. */
> + void (*write_v_block) (struct trace_file_writer *self, int num,
> + LONGEST val);
Though, talking about 'R', 'M' and 'V' is tfile specific, right?
> +
> + /* The end of the trace frame. */
> + void (*end) (struct trace_file_writer *self);
> +};
> +
> +/* Operations to write trace buggers to a specific trace format. */
s/buggers/buffers/ I believe. More instances of the typo below.
> +
> +struct trace_file_write_ops
> +{
> + /* Return true if the target is able to save the data to file
> + or directory NAME of desired format. */
> + int (*can_target_save) (struct trace_file_writer *self,
> + const char *ame);
> +
> + /* Write the trace buggers to file or directory NAME. */
> + void (*start) (struct trace_file_writer *self,
> + const char *name);
> +
> + /* Write the trace header. */
> + void (*write_header) (struct trace_file_writer *self);
> +
> + /* Write the type of block about registers. SIZE is the size of
> + all registers on the target. */
> + void (*write_regblock_type) (struct trace_file_writer *self,
> + int size);
> +
> + /* Write trace status TS. */
> + void (*write_status) (struct trace_file_writer *self,
> + struct trace_status *ts);
> +
> + /* Write the uploaded TSV. */
> + void (*write_uploaded_tsv) (struct trace_file_writer *self,
> + struct uploaded_tsv *tsv);
> +
> + /* Write the uploaded tracepoint TP. */
> + void (*write_uploaded_tp) (struct trace_file_writer *self,
> + struct uploaded_tp *tp);
> +
> + /* 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.
> + void (*write_definition_end) (struct trace_file_writer *self);
> +
> + /* Write the raw data of trace bugger. The content is in BUF and
> + length is LEN. */
> + void (*write_raw_data) (struct trace_file_writer *self,
> + gdb_byte *buf, LONGEST len);
> +
> + /* Operations to write trace frames. */
> + struct trace_frame_write_ops *frame_ops;
> +
> + /* The end of writing trace buggers. */
> + void (*end) (struct trace_file_writer *self);
> +};
> +
> +/* Trace file writer for a given format. */
> +
> +struct trace_file_writer
> +{
> + struct trace_file_write_ops *ops;
> +
> + /* Writer specific data. */
> + void *data;
> +};
I think it's better design to get rid of this
"data" pointer, which as is needs to be xmalloc'ed
separately. Get rid of "struct tfile_writer_data", and do
instead:
struct trace_file_writer
{
struct trace_file_write_ops *ops;
};
struct tfile_writer
{
struct trace_file_writer base;
/* Path name of the tfile trace file. */
char *pathname;
struct cleanup *cleanup;
};
struct whatever_writer
{
struct trace_file_writer base;
/* whatever fields. */
};
And then:
- struct tfile_writer_data *data = self->data;
+ struct tfile_writer *writer = (struct tfile_writer *) self;
--
Pedro Alves