From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2163 invoked by alias); 30 May 2012 20:42:52 -0000 Received: (qmail 2144 invoked by uid 22791); 30 May 2012 20:42:46 -0000 X-SWARE-Spam-Status: No, hits=-5.5 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 30 May 2012 20:42:29 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q4UKgQVl028674 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 30 May 2012 16:42:26 -0400 Received: from host2.jankratochvil.net (ovpn-116-47.ams2.redhat.com [10.36.116.47]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q4UKgC1w014262 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 30 May 2012 16:42:15 -0400 Date: Wed, 30 May 2012 20:42:00 -0000 From: Jan Kratochvil To: markus.t.metzger@intel.com Cc: kettenis@gnu.org, gdb-patches@sourceware.org, markus.t.metzger@gmail.com Subject: Re: [PATCH 05/16] cli, btrace: add btrace cli Message-ID: <20120530204212.GE20633@host2.jankratochvil.net> References: <1337772151-20265-1-git-send-email-markus.t.metzger@intel.com> <1337772151-20265-6-git-send-email-markus.t.metzger@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1337772151-20265-6-git-send-email-markus.t.metzger@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-05/txt/msg01060.txt.bz2 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 > +#include > +#include > + > +/* 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 \"+[]\" argument, selects the n-th chronologically preceding block.\n\ > +With \"-[]\" argument, selects the n-th chronologically succeeding block.\n\ > +With one positive integer argument, selects the respective block.\n\ > +With a range argument \"-\", 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 ([-]) 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