This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 05/16] cli, btrace: add btrace cli
On Wed, 23 May 2012 13:22:20 +0200, markus.t.metzger@intel.com wrote:
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index e66a986..2b57ca7 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -23,8 +23,24 @@
> #include "gdbthread.h"
> #include "frame.h"
> #include "exceptions.h"
> +#include "observer.h"
> +#include "command.h"
> +#include "cli/cli-cmds.h"
> +#include "cli/cli-utils.h"
> +#include "arch-utils.h"
> +#include "disasm.h"
>
> #include <errno.h>
> +#include <ctype.h>
> +#include <string.h>
> +
> +/* A new thread observer enabling branch tracing for the new thread. */
> +static struct observer *btrace_thread_observer;
> +
> +/* Branch tracing command list. */
> +static struct cmd_list_element *btcmdlist;
> +static struct cmd_list_element *btencmdlist;
> +static struct cmd_list_element *btdiscmdlist;
The variables should be each commented.
>
> #if !defined(EALREADY)
> /* Remap EALREADY for systems that do not define it, e.g. mingw. */
> @@ -252,3 +268,591 @@ next_btrace (struct thread_info *tinfo)
>
> return VEC_index (btrace_block_s, btinfo->btrace, btinfo->iterator);
> }
> +
Function comment.
> +static int
> +thread_callback (struct thread_info *tinfo, void *arg)
Despite it is static there are some wishes for GDB all the functions should
have the filename prefix, therefore btrace_thread_callback here.
> +{
> + observer_new_thread_ftype *tfun = arg;
Empty line after declarations.
> + tfun (tinfo);
> +
> + return 0;
> +}
> +
> +static void
> +for_each_thread (observer_new_thread_ftype *tfun)
> +{
> + iterate_over_threads (thread_callback, tfun);
> +}
> +
> +static void
> +do_enable_btrace (struct thread_info *tinfo)
> +{
> + int errcode = enable_btrace (tinfo);
> + if (errcode)
> + {
> + int pid = ptid_get_lwp (tinfo->ptid);
Empty line after declarations.
> + if (!pid)
> + pid = ptid_get_pid (tinfo->ptid);
> +
> + error (_("Couldn't enable branch tracing for %d: %s"),
> + pid, safe_strerror (errcode));
Just use %s and as argument: target_pid_to_str (tinfo)
But enable_btrace should error on its own as I have recommended.
> + }
> +}
> +
> +static void
> +safe_do_enable_btrace (struct thread_info *tinfo)
> +{
> + if (!target_supports_btrace ())
> + warning (_("Target does not support branch tracing."));
> + else
> + {
> + volatile struct gdb_exception error;
> +
> + TRY_CATCH (error, RETURN_MASK_ERROR)
> + {
> + do_enable_btrace (tinfo);
> + }
> + if (error.message)
> + warning (_("%s"), error.message);
> + }
> +}
> +
> +static void
> +do_disable_btrace (struct thread_info *tinfo)
> +{
> + int errcode = disable_btrace (tinfo);
> + if (errcode)
> + {
> + int pid = ptid_get_lwp (tinfo->ptid);
> + if (!pid)
> + pid = ptid_get_pid (tinfo->ptid);
> +
> + error (_("Couldn't disable branch tracing for %d: %s"),
> + pid, safe_strerror (errcode));
> + }
Likewise above.
> +}
> +
> +static void
> +safe_do_disable_btrace (struct thread_info *tinfo)
> +{
> + if (!target_supports_btrace ())
> + warning (_("Target does not support branch tracing."));
> + else
> + {
> + volatile struct gdb_exception error;
> +
> + TRY_CATCH (error, RETURN_MASK_ERROR)
> + {
> + do_disable_btrace (tinfo);
> + }
> + if (error.message)
> + warning (_("%s"), error.message);
> + }
> +}
> +
> +static int
> +do_enable_btrace_list (struct thread_info *tinfo, void *arg)
> +{
> + char *range = (char *)arg;
> +
> + if (number_is_in_list (range, tinfo->num))
> + {
> + /* Switching threads makes it easier for targets like kgdb, where we need
> + to switch cpus, as well. */
> + switch_to_thread (tinfo->ptid);
In such case it should be switched in the most bottom function.
> + safe_do_enable_btrace (tinfo);
Here is TINFO still passed as a parameter so it is not required to call
switch_to_thread.
> + }
> +
> + return 0;
> +}
> +
> +static void
> +cmd_btrace_enable (char *args, int from_tty)
> +{
> + if (args)
So far I think GDB considers ARGS may be either NULL or "" as no-parameters.
> + {
> + ptid_t ptid = inferior_ptid;
Again use save_inferior_ptid like I suggested before.
> +
> + iterate_over_threads (do_enable_btrace_list, args);
> +
> + switch_to_thread (ptid);
And here likewise.
> + }
> + else
> + {
> + struct thread_info *tinfo = find_thread_ptid (inferior_ptid);
Empty line after declarations.
> + if (!tinfo)
> + error (_("Couldn't enable branch tracing: no inferior thread."));
> + else
'else' is not needed, 'error' doe snot return.
> + do_enable_btrace (tinfo);
> + }
> +}
> +
> +static void
> +cmd_btrace_enable_all (char *args, int from_tty)
> +{
> + if (args)
> + error (_("Invalid argument."));
> + else
Redundant 'else'.
> + for_each_thread (safe_do_enable_btrace);
> +}
> +
> +static void
> +cmd_btrace_enable_auto (char *args, int from_tty)
> +{
> + if (args)
> + error (_("Invalid argument."));
> + else if (btrace_thread_observer)
> + error (_("Automatic branch trace enabling already on."));
> + else
> + btrace_thread_observer =
> + observer_attach_new_thread (safe_do_enable_btrace);
Again redundant 'else's.
> +}
> +
> +static int
> +do_disable_btrace_list (struct thread_info *tinfo, void *arg)
> +{
> + char *range = (char *)arg;
> +
> + if (number_is_in_list (range, tinfo->num))
> + {
> + /* Switching threads makes it easier for targets like kgdb, where we need
> + to switch cpus, as well. */
> + switch_to_thread (tinfo->ptid);
Likewise above.
> + safe_do_disable_btrace (tinfo);
> + }
> +
> + return 0;
> +}
> +
> +static void
> +cmd_btrace_disable (char *args, int from_tty)
> +{
> + if (args)
> + {
> + ptid_t ptid = inferior_ptid;
> +
> + iterate_over_threads (do_disable_btrace_list, args);
> +
> + switch_to_thread (ptid);
> + }
> + else
> + {
> + struct thread_info *tinfo = find_thread_ptid (inferior_ptid);
> + if (!tinfo)
> + error (_("Couldn't disable branch tracing: no inferior thread."));
> + else
> + do_disable_btrace (tinfo);
> + }
All likewise in the enable case.
> +}
> +
> +static void
> +cmd_btrace_disable_all (char *args, int from_tty)
> +{
> + if (args)
> + error (_("Invalid argument."));
> + else
> + for_each_thread (safe_do_disable_btrace);
> +}
> +
> +static void
> +cmd_btrace_disable_auto (char *args, int from_tty)
> +{
> + if (args)
> + error (_("Invalid argument."));
> + else if (!btrace_thread_observer)
> + error (_("Automatic branch trace enabling already off."));
> + else
> + {
> + observer_detach_new_thread (btrace_thread_observer);
> + btrace_thread_observer = NULL;
> + }
> +}
> +
> +#define BTR_LIST_ADDRESS (1 << 0)
> +#define BTR_LIST_FUNCTION (1 << 1)
> +#define BTR_LIST_LINE (1 << 2)
> +#define BTR_LIST_TOTAL (1 << 3)
It should be rather enum. Each item needs its comment.
> +
> +static void
> +do_btrace_list_item (unsigned int block, struct btrace_block *trace,
> + unsigned int flags)
> +{
> + struct gdbarch *gdbarch = get_current_arch ();
gdbarch depends on TRACE, not on current frame. Each frame can have different
gdbarch. But putting gdbarch into each TRACE is probably too expensive.
I would guess target_gdbarch is more acceptable to use for printing CORE_ADDR.
> + struct symtab *symtab = NULL;
> + struct symbol *symbol = NULL;
> +
> + if (!trace)
> + return;
Cannot happen, remove or gdb_assert.
> +
> + ui_out_field_fmt_int (current_uiout, 5, ui_left, "block", block);
> +
> + if (flags & BTR_LIST_ADDRESS)
> + {
> + ui_out_field_core_addr (current_uiout, "begin", gdbarch, trace->begin);
> + ui_out_text (current_uiout, " - ");
> + ui_out_field_core_addr (current_uiout, "end", gdbarch, trace->end);
> + }
> +
> + if (flags & BTR_LIST_FUNCTION)
> + {
> + ui_out_text (current_uiout, " in ");
> +
> + symbol = find_pc_function (trace->begin);
> + if (symbol)
> + ui_out_field_string (current_uiout, "function",
> + SYMBOL_PRINT_NAME (symbol));
> + else
> + {
> + struct minimal_symbol *msymbol =
> + lookup_minimal_symbol_by_pc (trace->begin);
Empty line.
> + if (msymbol)
> + ui_out_field_string (current_uiout, "function",
> + SYMBOL_PRINT_NAME (msymbol));
> + else
> + ui_out_text (current_uiout, "??");
> + }
> +
> + ui_out_text (current_uiout, " ()");
> + }
> +
> + if (flags & BTR_LIST_LINE)
> + {
> + struct linetable *linetable = NULL;
> + const char *filename = NULL;
> +
> + if (symbol)
> + symtab = symbol->symtab;
> + if (!symtab)
> + symtab = find_pc_symtab (trace->begin);
> + if (symtab)
> + {
> + linetable = symtab->linetable;
> + filename = symtab->filename;
> + }
> +
> + if (filename)
> + {
> + ui_out_text (current_uiout, " at ");
> + ui_out_field_string (current_uiout, "file", filename);
> +
> + if (linetable)
> + {
> + struct linetable_entry *line = linetable->item;
> + int nitems = linetable->nitems, i = 0;
> + int begin = INT_MAX, end = 0;
> +
> + /* Skip all preceding entries. */
> + for (; i < (nitems - 1) && line[i + 1].pc <= trace->begin; ++i);
> +
> + for (; i < nitems && line[i].pc <= trace->end; ++i)
> + {
> + begin = min (begin, line[i].line);
> + end = max (end, line[i].line);
> + }
> +
> + if (begin <= end)
> + {
> + ui_out_text (current_uiout, ":");
> + ui_out_field_int (current_uiout, "lbegin", begin);
> +
> + if (begin < end)
> + {
> + ui_out_text (current_uiout, "-");
> + ui_out_field_int (current_uiout, "lend", end);
> + }
> + }
> + }
> + }
> + }
> +
> + ui_out_text (current_uiout, "\n");
> +}
> +
> +static void
> +do_btrace_list (VEC (btrace_block_s) *btrace, char *range,
> + unsigned int flags)
> +{
> + struct gdbarch *gdbarch = get_current_arch ();
Unused variable.
> + struct cleanup *ui_out_chain =
> + make_cleanup_ui_out_tuple_begin_end (current_uiout, "btrace blocks");
> + unsigned int block = 0;
> + struct btrace_block *trace;
> +
> + while (VEC_iterate (btrace_block_s, btrace, block, trace))
> + {
> + ++block;
> +
> + if (number_is_in_list (range, block))
> + do_btrace_list_item (block, trace, flags);
> + }
> +
> + do_cleanups (ui_out_chain);
> +}
> +
> +static void
> +cmd_btrace_list (char *args, int from_tty)
> +{
> + struct thread_info *thread = find_thread_ptid (inferior_ptid);
> + VEC (btrace_block_s) *btrace = get_btrace (thread);
> + unsigned int flags = BTR_LIST_FUNCTION | BTR_LIST_LINE;
> +
> + /* Parse modifier. */
> + if (args)
> + {
> + if (*args == '/')
> + flags = 0;
> +
> + while (*args == '/')
> + {
> + ++args;
> +
> + if (*args == '\0')
> + error (_("Missing modifier."));
> +
> + for (; *args; ++args)
> + {
> + if (isspace (*args))
> + break;
> +
> + if (*args == '/')
> + continue;
> +
> + switch (*args)
> + {
> + case 'a':
> + flags |= BTR_LIST_ADDRESS;
> + break;
> + case 'f':
> + flags |= BTR_LIST_FUNCTION;
> + break;
> + case 'l':
> + flags |= BTR_LIST_LINE;
> + break;
> + case 't':
> + flags |= BTR_LIST_TOTAL;
> + break;
> + default:
> + error (_("Invalid modifier: %c."), *args);
> + }
> + }
> +
> + args = skip_spaces (args);
> + }
> + }
> +
> + if (flags & BTR_LIST_TOTAL)
> + {
> + struct cleanup *ui_out_chain;
> + unsigned int total = VEC_length (btrace_block_s, btrace);
> +
> + ui_out_chain =
> + make_cleanup_ui_out_tuple_begin_end (current_uiout, "btrace blocks");
> +
> + ui_out_text (current_uiout, "total: ");
> + ui_out_field_int (current_uiout, "total", total);
> + ui_out_text (current_uiout, "\n");
> +
> + do_cleanups (ui_out_chain);
> + }
> + else if (!btrace)
> + error (_("No trace"));
dot:
error (_("No trace."));
> + else
> + do_btrace_list (btrace, args, flags);
> +}
> +
> +static void
> +do_btrace (struct btrace_block *trace, int flags)
> +{
> + struct gdbarch *gdbarch = get_current_arch ();
> +
> + if (!trace)
> + {
> + error (_("No trace."));
> + return;
Excessive return.
> + }
> +
> + if (trace->end < trace->begin)
> + error (_("Bad trace: 0x%" BFD_VMA_FMT "x - 0x%" BFD_VMA_FMT "x."),
> + trace->begin, trace->end);
It is CORE_ADDR, not bfd_vma. Therefore use paddress.
> +
> + gdb_disassembly (gdbarch, current_uiout, 0, flags, -1,
> + trace->begin, trace->end + 1);
> +}
> +
> +static void
> +cmd_btrace (char *args, int from_tty)
> +{
> + struct thread_info *thread = find_thread_ptid (inferior_ptid);
> + struct btrace_block *trace = NULL;
> + int flags = 0;
Use enum.
> +
> + if (!thread)
> + {
> + error (_("No thread."));
> + return;
Excessive return.
> + }
> +
> + /* Parse modifier. We accept the same modifiers as the disas command. */
> + if (args)
> + {
> + while (*args == '/')
> + {
> + ++args;
> +
> + if (*args == '\0')
> + error (_("Missing modifier."));
> +
> + for (; *args; ++args)
> + {
> + if (isspace (*args))
> + break;
> +
> + if (*args == '/')
> + continue;
> +
> + switch (*args)
> + {
> + case 'm':
> + flags |= DISASSEMBLY_SOURCE;
> + flags |= DISASSEMBLY_PRECISE_INSN;
> + flags |= DISASSEMBLY_FILENAME;
> + break;
> + case 'r':
> + flags |= DISASSEMBLY_RAW_INSN;
> + break;
> + default:
> + error (_("Invalid modifier: %c."), *args);
> + }
> + }
> +
> + args = skip_spaces (args);
> + }
> + }
> +
> +
> + if (!args || !*args)
> + trace = prev_btrace (thread);
> + else if (args[0] == '+')
> + {
> + size_t skip = 1;
> +
> + args++;
> + if (args[0])
> + skip = get_number (&args);
> +
> + while (skip--)
> + trace = prev_btrace (thread);
> + }
> + else if (args[0] == '-')
> + {
> + size_t skip = 1;
> +
> + args++;
> + if (args[0])
> + skip = get_number (&args);
> +
> + while (skip--)
> + trace = next_btrace (thread);
> + }
> + else
> + {
> + /* We store the relevant blocks into a separate vector, so we can display
> + them in reverse order. */
> + VEC (btrace_block_s) *btrace = NULL;
> + struct cleanup *cleanup;
> + struct get_number_or_range_state state;
> + unsigned int i;
> +
> + cleanup = make_cleanup (xfree, btrace);
You must use VEC_cleanup here, not xfree.
> + init_number_or_range (&state, args);
> + while (!state.finished)
> + {
> + int index = get_number_or_range (&state);
Empty line after declarations.
> + if (!index)
> + {
> + error (_("Args must be numbers or '$' variables."));
> + return;
Excessive return.
> + }
> +
> + trace = read_btrace (thread, index - 1);
> + if (!trace)
> + continue;
> +
> + VEC_safe_push (btrace_block_s, btrace, trace);
> + }
> +
> + i = VEC_length (btrace_block_s, btrace);
> + while (i--)
> + do_btrace (VEC_index (btrace_block_s, btrace, i), flags);
> +
> + do_cleanups (cleanup);
> + return;
The function is a bit spaghetting, maybe you could split it for the different
cases. Just optional.
> + }
> +
> + if (args && *args)
> + {
> + error (_("Junk after argument: %s"), args);
> + return;
Excessive return.
> + }
> +
> + do_btrace (trace, flags);
> +}
> +
> +void _initialize_btrace (void);
> +
> +void
> +_initialize_btrace (void)
> +{
> + struct cmd_list_element *c;
> +
> + add_cmd ("branchtrace", class_btrace, NULL,
> + _("Recording a branch trace."), &cmdlist);
Incorrect indentation, it would be (using tab):
add_cmd ("branchtrace", class_btrace, NULL,
_("Recording a branch trace."), &cmdlist);
but you can:
add_cmd ("branchtrace", class_btrace, NULL, _("Recording a branch trace."),
&cmdlist);
> +
> + add_prefix_cmd ("btrace", class_btrace, cmd_btrace, _("\
> +Disassemble the selected branch trace block.\n\n\
> +With a /m modifier, source lines are included (if available).\n\
> +With a /r modifier, raw instructions in hex are included.\n\n\
> +Without arguments, selects the chronologically preceding block.\n\
> +With \"+[<n>]\" argument, selects the n-th chronologically preceding block.\n\
> +With \"-[<n>]\" argument, selects the n-th chronologically succeeding block.\n\
> +With one positive integer argument, selects the respective block.\n\
> +With a range argument \"<l>-<h>\", selects the h-th block and disassembles \
> +blocks in the range in reverse (i.e. original control flow) order.\n"),
> + &btcmdlist, "btrace ", 1, &cmdlist);
In many cases - you should use tab character instead of 8 spaces.
> +
> + add_prefix_cmd ("enable", class_btrace, cmd_btrace_enable, _("\
> +Enable branch trace recording for the current thread.\n"),
> + &btencmdlist, "btrace enable ", 1, &btcmdlist);
> +
> + add_cmd ("all", class_btrace, cmd_btrace_enable_all, _("\
> +Enable branch trace recording for all existing threads.\n"),
> + &btencmdlist);
> +
> + add_cmd ("auto", class_btrace, cmd_btrace_enable_auto, _("\
> +Enable branch trace recording for each new thread.\n"),
> + &btencmdlist);
> +
> + add_prefix_cmd ("disable", class_btrace, cmd_btrace_disable, _("\
> +Disable branch trace recording for the current thread.\n"),
> + &btdiscmdlist, "btrace disable ", 1, &btcmdlist);
> +
> + add_cmd ("all", class_btrace, cmd_btrace_disable_all, _("\
> +Disable branch trace recording for all existing threads.\n"),
> + &btdiscmdlist);
> +
> + add_cmd ("auto", class_btrace, cmd_btrace_disable_auto, _("\
> +Stop enabling branch trace recording for each new thread.\n"),
> + &btdiscmdlist);
> +
> + add_cmd ("list", class_btrace, cmd_btrace_list, _("\
> +List branch trace blocks.\n\n\
> +Prints a list of all blocks for which branch trace is available.\n\
> +With a /a modifier, addresses are included.\n\
> +With a /f modifier, the function name is included (if available).\n\
> +With a /l modifier, source lines are include (if available).\n\
> +With a /t modifier, prints the total number of trace blocks and stops.\n\
> +Without any modifier, behaves as if /fl were specified.\n\n\
> +Without arguments, the full list of blocks is listed.\n\
> +With a range (<start>[-<end>]) argument, the blocks in that range are listed.\n"),
> + &btcmdlist);
> +}
> diff --git a/gdb/command.h b/gdb/command.h
> index c18e2dd..999ec1d 100644
> --- a/gdb/command.h
> +++ b/gdb/command.h
> @@ -37,7 +37,7 @@ enum command_class
> no_class = -1, class_run = 0, class_vars, class_stack, class_files,
> class_support, class_info, class_breakpoint, class_trace,
> class_alias, class_bookmark, class_obscure, class_maintenance,
> - class_pseudo, class_tui, class_user, class_xdb,
> + class_pseudo, class_tui, class_user, class_xdb, class_btrace,
> no_set_class /* Used for "show" commands that have no corresponding
> "set" command. */
> };
> diff --git a/gdb/testsuite/gdb.base/page.exp b/gdb/testsuite/gdb.base/page.exp
> index 0629807..d9b3899 100644
> --- a/gdb/testsuite/gdb.base/page.exp
> +++ b/gdb/testsuite/gdb.base/page.exp
> @@ -25,6 +25,7 @@ gdb_test_sequence "help" "unpaged help" {
> "List of classes of commands:"
> ""
> "aliases -- Aliases of other commands"
> + "branchtrace -- Recording a branch trace"
I am not sure if it is worth a new class but it does not fit much in
'tracepoints'.
> "breakpoints -- Making program stop at certain points"
> "data -- Examining data"
> "files -- Specifying and examining files"
> @@ -52,12 +53,12 @@ gdb_expect_list "paged help" \
> "List of classes of commands:"
> ""
> "aliases -- Aliases of other commands"
> + "branchtrace -- Recording a branch trace"
> "breakpoints -- Making program stop at certain points"
> "data -- Examining data"
> "files -- Specifying and examining files"
> "internals -- Maintenance commands"
> "obscure -- Obscure features"
> - "running -- Running the program"
> }
> gdb_test "q"
>
> --
> 1.7.1
Thanks,
Jan