This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/5] Save trace into CTF format
- From: Tom Tromey <tromey at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Fri, 01 Mar 2013 12:22:41 -0700
- Subject: Re: [PATCH 2/5] Save trace into CTF format
- References: <1361931459-3953-1-git-send-email-yao@codesourcery.com> <1361931459-3953-3-git-send-email-yao@codesourcery.com> <512F38CE.5030802@codesourcery.com>
>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
Yao> +/* Handler of writing trace of CTF format. */
Yao> +
Yao> +struct trace_write_handler
I don't think that comment is very useful.
Perhaps it could just say that this is the state kept while writing a
CTF file. Assuming that is what it is.
Yao> + if (fwrite (buf, size, 1, fd) != 1)
Yao> + error (_("Unable to write file for saving trace data (%s)"),
Yao> + safe_strerror (errno));
We had this whole thread about using ui-out with patches and everything.
Upthread you said that it was abandoned due to lack of error checking on
write. But I thought that was addressed in the previous thread?
I suppose I don't really care enough to press it.
I don't recall offhand why I thought it was important; maybe some code
duplication, but I don't really see it any more.
Yao> +static void
Yao> +ctf_save_fwrite_format_1 (FILE *fd, const char *format, va_list args)
Yao> +{
Yao> + char *linebuffer;
Yao> + struct cleanup *old_cleanups;
Yao> +
Yao> + linebuffer = xstrvprintf (format, args);
Yao> + old_cleanups = make_cleanup (xfree, linebuffer);
Yao> + ctf_save_fwrite (fd, linebuffer, strlen (linebuffer));
Yao> + do_cleanups (old_cleanups);
Yao> +}
Yao> +
Yao> +/* Write data in FORMAT to FD. */
Yao> +
Yao> +static void
Yao> +ctf_save_fwrite_format (FILE *fd, const char *format, ...)
Yao> +{
Yao> + va_list args;
Yao> +
Yao> + va_start (args, format);
Yao> + ctf_save_fwrite_format_1 (fd, format, args);
Yao> + va_end (args);
Yao> +}
Why not just use vfprintf directly and drop ctf_save_fwrite_format_1 and
the cleanups and the rest?
Yao> +static int
Yao> +ctf_save_fseek (struct trace_write_handler *handler, long offset,
Yao> + int whence)
Yao> +{
Yao> + if (fseek (handler->datastream_fd, offset, whence))
Yao> + error (_("Unable to seek file for saving trace data (%s)"),
Yao> + safe_strerror (errno));
Yao> +
Yao> + if (whence == SEEK_CUR)
Yao> + handler->content_size += offset;
Why only update the size in this one case?
If it is because the callers only use it in specific ways, then I
suggest assertions to cover this, like maybe:
gdb_assert (whence != SEEK_END);
gdb_assert (whence != SEEK_SET || offset < handler->content_size);
Yao> +/* Change the datastream file position to align on ALIGN_SIZE,
Yao> + and Write BUF to datastream file. The size of BUF is SIZE. */
Lowercase "w" in "write".
Yao> + if (handler->metadata_fd)
Yao> + fclose (handler->metadata_fd);
Yao> +
Yao> + if (handler->datastream_fd)
Yao> + fclose (handler->datastream_fd);
Use the "!= NULL" form.
Yao> + /* Create DIRNAME. */
Yao> + file_name = alloca (strlen (dirname) + 1
Yao> + + strlen (CTF_DATASTREAM_NAME) + 1);
I think this assignment is dead.
Yao> + if (!data->tcs.datastream_fd)
Use "!= NULL".
I think there are some other cases I didn't point out.
Yao> + /* Tracepoint number. */
Yao> + ctf_save_write (&data->tcs, (gdb_byte *) &tpnum, 2);
Does endianness matter?
Yao> + /* Step 2: Write event "frame". */
Yao> + /* Event Id. */
Yao> + ctf_save_align_write (&data->tcs, (gdb_byte *) &two, 4, 4);
The comments Andreas and Pedro made apply here.
Yao> + /* Event Id. */
Yao> + ctf_save_align_write (&data->tcs, (gdb_byte *) &event_id, 4, 4);
Yao> +
Yao> + /* Address. */
Yao> + ctf_save_align_write (&data->tcs, (gdb_byte *) &addr, 8, 8);
Here too I think.
There are more spots as well.
Yao> +/* Operations to write various types of trace frames into CTF
Yao> + format. */
Yao> +
Yao> +static struct trace_frame_write_ops ctf_write_frame_ops =
Why not const?
Yao> +static struct trace_file_write_ops ctf_write_ops =
Likewise.
Yao> +extern struct trace_file_writer *ctf_trace_file_writer (void);
Yao> +#endif
Newline between these two.
Yao> mi_cmd_trace_save (char *command, char **argv, int argc)
Yao> {
Yao> int target_saves = 0;
Yao> + int generate_ctf = 0;
Yao> char *filename;
Yao> if (argc != 1 && argc != 2)
Yao> - error (_("Usage: -trace-save [-r] filename"));
Yao> + error (_("Usage: -trace-save [-r] [-ctf] filename"));
Yao> if (argc == 2)
Yao> {
Yao> filename = argv[1];
Yao> if (strcmp (argv[0], "-r") == 0)
Yao> target_saves = 1;
Yao> + if (strcmp (argv[0], "-ctf") == 0)
Yao> + generate_ctf = 1;
I wonder why this doesn't use mi_getopt.
(Not your problem unless you feel like cleaning it up.)
Yao> add_com ("tsave", class_trace, trace_save_command, _("\
Yao> Save the trace data to a file.\n\
Yao> Use the '-r' option to direct the target to save directly to the file,\n\
Yao> +Use the '-ctf' option to save the data to CTF format,\n\
Yao> using its own filesystem."));
This reads strangely after the change.
Does -ctf direct the target to use its own filesystem?
Perhaps rewriting all this into a more "--help" style would be better;
or otherwise at least repeating the final clause.
Tom