From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32762 invoked by alias); 27 Feb 2013 07:38:05 -0000 Received: (qmail 32647 invoked by uid 22791); 27 Feb 2013 07:38:04 -0000 X-SWARE-Spam-Status: No, hits=-5.7 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BJ,TW_BT,TW_EG 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, 27 Feb 2013 07:37:49 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1R7bkaY030749 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 27 Feb 2013 02:37:46 -0500 Received: from host2.jankratochvil.net (ovpn-116-19.ams2.redhat.com [10.36.116.19]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r1R7bVb6015708 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 27 Feb 2013 02:37:34 -0500 Date: Wed, 27 Feb 2013 07:38:00 -0000 From: Jan Kratochvil To: markus.t.metzger@intel.com Cc: gdb-patches@sourceware.org, markus.t.metzger@gmail.com Subject: Re: [PATCH 1/3] record, btrace: add record-btrace target Message-ID: <20130227073731.GA1378@host2.jankratochvil.net> References: <1361808917-16934-1-git-send-email-markus.t.metzger@intel.com> <1361808917-16934-2-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: <1361808917-16934-2-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: 2013-02/txt/msg00677.txt.bz2 On Mon, 25 Feb 2013 17:15:15 +0100, markus.t.metzger@intel.com wrote: > The target implements the new record sub-commands > "record instruction-history" and > "record function-call-history". > > The target does not support reverse execution or navigation in the > recorded execution log. When you do not plan now to support either "record source-lines-history" or "reverse execution or navigation in the recorded execution log" it won't be supported in any form in gdb-7.6? Do you plan to implement it afterwards? > I'm not quite sure when to use ui_out_~ functions and when to use > printf_unfiltered. There is no difference with normal CLI or TUI. But when you use MI protocol and provide a new MI command for your code then printf_unfiltered and ui_out_text outputs get ignored while the other ui_out_* functions automatically provide MI protocol named fields with the supplied data. (-interpreter-exec console "cmdname" is an MI command but its output is raw text, not the MI 'machine-parseable' named fields.) > I tried to copy what others are doing, but I'm > not sure whether I got it right. It does not matter until someone starts to implement proper MI commands for these new interfaces. > For the "record function-call-history" command, I assume that symbols can be > compared by comparing their pointers. For comparing files, I use > compare_filenames_for_search. I'm not sure whether this is the appropriate > function to use in this context. > > 2013-02-25 Markus Metzger > > * Makefile.in (SFILES): Add record-btrace.c > (COMMON_OBS): Add record-btrace.o > * record-btrace.c: New. > * btrace.h (struct btrace_insn_iterator, > struct btrace_function_iterator): New. > (struct btrace_thread_info) call_iterator>: New fields. > * common/btrace-common.h (struct btrace_inst): New. > (btrace_inst_s): New typedef. > > > --- > gdb/Makefile.in | 4 +- > gdb/btrace.h | 26 ++ > gdb/common/btrace-common.h | 20 +- > gdb/record-btrace.c | 1022 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1067 insertions(+), 5 deletions(-) > create mode 100644 gdb/record-btrace.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index a36d576..5366d9e 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -760,7 +760,7 @@ SFILES = ada-exp.y ada-lang.c ada-typeprint.c ada-valprint.c ada-tasks.c \ > regset.c sol-thread.c windows-termcap.c \ > common/gdb_vecs.c common/common-utils.c common/xml-utils.c \ > common/ptid.c common/buffer.c gdb-dlfcn.c common/agent.c \ > - common/format.c btrace.c > + common/format.c btrace.c record-btrace.c > > LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c > > @@ -930,7 +930,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ > inferior.o osdata.o gdb_usleep.o record.o record-full.o gcore.o \ > gdb_vecs.o jit.o progspace.o skip.o probe.o \ > common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o \ > - format.o registry.o btrace.o > + format.o registry.o btrace.o record-btrace.o > > TSOBS = inflow.o > > diff --git a/gdb/btrace.h b/gdb/btrace.h > index 26fafc7..d56bfa4 100644 > --- a/gdb/btrace.h > +++ b/gdb/btrace.h > @@ -29,6 +29,25 @@ > #include "btrace-common.h" > > struct thread_info; > +struct btrace_thread_info; > + > +/* Branch trace iteration state for "record instruction-history". */ > +struct btrace_insn_iterator > +{ > + /* The instruction index range [begin; end[ that has been covered last time. > + If end < begin, the branch trace has just been updated. */ > + unsigned int begin; > + unsigned int end; > +}; > + > +/* Branch trace iteration state for "record function-call-history". */ > +struct btrace_function_iterator > +{ > + /* The instruction index range [begin; end[ that has been covered last time. > + If end < begin, the branch trace has just been updated. */ > + unsigned int begin; > + unsigned int end; > +}; > > /* Branch trace information per thread. > > @@ -47,6 +66,7 @@ struct btrace_thread_info > > /* The current branch trace for this thread. */ > VEC (btrace_block_s) *btrace; > + VEC (btrace_inst_s) *itrace; > > /* The current iterator position in the above trace vector. > In additon to valid vector indices, the iterator can be: > @@ -54,6 +74,12 @@ struct btrace_thread_info > -1 one before the head > VEC_length() one after the tail */ > int iterator; > + > + /* The instruction history iterator. */ > + struct btrace_insn_iterator insn_iterator; > + > + /* The function call history iterator. */ > + struct btrace_function_iterator call_iterator; > }; > > /* Enable branch tracing for a thread. */ > diff --git a/gdb/common/btrace-common.h b/gdb/common/btrace-common.h > index 90372ba..c3f52f9 100644 > --- a/gdb/common/btrace-common.h > +++ b/gdb/common/btrace-common.h > @@ -49,12 +49,26 @@ struct btrace_block > CORE_ADDR end; > }; > > -/* Branch trace is represented as a vector of branch trace blocks starting with > - the most recent block. */ > +/* A branch trace instruction. > + > + This represents a single instruction in a branch trace. */ > +struct btrace_inst > +{ > + /* The address of this instruction. */ > + CORE_ADDR ip; Normal name for Program Counter in GDB is "pc"; "ip" is Intel x86-specific naming. btrace is x86-specific feature but still I believe GDB naming is preferred. > +}; > + > + > +/* Branch trace may be represented as a vector of either: > + > + - branch trace blocks starting with the most recent block. > + - branch trace instructions starting with the oldest instruction. */ > typedef struct btrace_block btrace_block_s; > +typedef struct btrace_inst btrace_inst_s; > > -/* Define functions operating on a vector of branch trace blocks. */ > +/* Define functions operating on branch trace vectors. */ > DEF_VEC_O (btrace_block_s); > +DEF_VEC_O (btrace_inst_s); > > /* Target specific branch trace information. */ > struct btrace_target_info; > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c > new file mode 100644 > index 0000000..e43b687 > --- /dev/null > +++ b/gdb/record-btrace.c > @@ -0,0 +1,1022 @@ > +/* Branch trace support for GDB, the GNU debugger. > + > + Copyright (C) 2013 Free Software Foundation, Inc. > + > + Contributed by Intel Corp. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include "defs.h" > +#include "record.h" > +#include "gdbthread.h" > +#include "regcache.h" > +#include "target.h" > +#include "gdbcmd.h" > +#include "disasm.h" > +#include "observer.h" > +#include "exceptions.h" > +#include "cli/cli-utils.h" > +#include "source.h" > +#include "ui-out.h" > +#include "symtab.h" > + > +/* The target_ops of record-btrace. */ > +static struct target_ops record_btrace_ops; > + > +/* A new thread observer enabling branch tracing for the new thread. */ > +static struct observer *record_btrace_thread_observer; > + > +/* A recorded function segment. */ > +struct btrace_function > +{ > + /* The function symbol. */ > + struct minimal_symbol *mfun; > + struct symbol *fun; When is one of them NULL or may be NULL? > + > + /* The name of the function. */ > + const char *function; Which of the forms SYMBOL_LINKAGE_NAME, SYMBOL_DEMANGLED_NAME, SYMBOL_PRINT_NAME etc.? > + > + /* The name of the file in which the function is defined. */ > + const char *filename; Absolute source filename? Or relative to compilation directory? > + > + /* The min and max line in the above file. */ min and max line (both inclusive) > + int begin; > + int end; > +}; > + > +/* A vector of recorded function segments. */ > +typedef struct btrace_function btr_fun_s; > +DEF_VEC_O (btr_fun_s); > + > +/* Debug output flags. */ > +enum record_btrace_debug_flag > +{ > + /* Print an overview of record-btrace functions. */ > + debug_functions = 1 << 0, > + > + /* Print details on "record function-call-history". */ > + debug_call_history = 1 << 1 > +}; > + > +/* Print a record-btrace debug message. Use do ... while (0) to avoid > + ambiguities when used in if statements. */ > + > +#define DEBUG(mask, msg, args...) \ > + do \ > + { \ > + if ((record_debug & mask) != 0) \ Use (mask) in parentheses. > + fprintf_unfiltered (gdb_stdlog, \ > + "record-btrace: " msg "\n", ##args); \ > + } \ > + while (0) > + > +#define DEBUG_FUN(msg, args...) DEBUG (debug_functions, msg, ##args) > +#define DEBUG_CALL(msg, args...) \ > + DEBUG (debug_call_history, "[hist: call] " msg, ##args) > + > + > +/* Initialize the instruction iterator. */ > + > +static void > +btrace_init_insn_iterator (struct btrace_thread_info *btinfo) > +{ > + DEBUG_FUN ("init insn iterator"); > + > + btinfo->insn_iterator.begin = 1; > + btinfo->insn_iterator.end = 0; > +} > + > +/* Initialize the function call iterator. */ > + > +static void > +btrace_init_call_iterator (struct btrace_thread_info *btinfo) > +{ > + DEBUG_FUN ("init call iterator"); > + > + btinfo->call_iterator.begin = 1; > + btinfo->call_iterator.end = 0; > +} > + > +/* Compute the instruction trace from the block trace. */ > + > +static VEC (btrace_inst_s) * > +compute_itrace (VEC (btrace_block_s) *btrace) > +{ > + VEC (btrace_inst_s) *itrace; > + struct gdbarch *gdbarch; > + unsigned int b; > + > + DEBUG_FUN ("compute itrace"); > + > + itrace = NULL; > + gdbarch = target_gdbarch (); > + b = VEC_length (btrace_block_s, btrace); > + > + while (b-- != 0) > + { > + btrace_block_s *block; > + CORE_ADDR ip; Normal name for Program Counter in GDB is "pc"; "ip" is Intel x86-specific naming. btrace is x86-specific feature but still I believe GDB naming is preferred. > + > + block = VEC_index (btrace_block_s, btrace, b); > + ip = block->begin; > + > + /* Add instructions for this block. */ > + for (;;) > + { > + btrace_inst_s *inst; > + int size; > + > + /* We should hit the end of the block. Warn if we went too far. */ > + if (block->end < ip) > + { > + warning (_("Recorded trace may be corrupted.")); > + break; > + } > + > + inst = VEC_safe_push (btrace_inst_s, itrace, NULL); > + inst->ip = ip; > + > + /* We're done once we pushed the instruction at the end. */ > + if (block->end == ip) > + break; > + > + size = gdb_insn_length (gdbarch, ip); > + > + /* Make sure we terminate if we fail to compute the size. */ > + if (size <= 0) > + { > + warning (_("Recorded trace may be incomplete.")); > + break; > + } > + > + ip += size; > + } > + } > + > + return itrace; > +} > + > +/* Fetch the branch trace for TP. */ > + > +static void > +fetch_btrace (struct thread_info *tp) > +{ > + struct btrace_thread_info *btinfo; > + > + DEBUG_FUN ("fetch trace"); > + > + btinfo = &tp->btrace; > + if (btinfo->target == NULL) > + return; > + > + if (!target_btrace_has_changed (btinfo->target)) > + return; > + > + VEC_free (btrace_block_s, btinfo->btrace); > + VEC_free (btrace_inst_s, btinfo->itrace); > + > + btinfo->btrace = target_read_btrace (btinfo->target); In btrace.c/update_btrace from archer-mmetzger-btrace: btp->btrace = target_read_btrace (btp->target); without freeing btp->btrace there beforehand, does it leak there? And gdb/ has only two calls of target_btrace_has_changed and target_read_btrace and both are called this similar way. Couldn't just be always called just target_read_btrace and the target would return some error if it has not changed? This also reduces one gdbserver round-trip-time for the read. > + > + /* The first block ends at the current pc. */ I do not understand here why the target does not know the current PC and does not set it up on its own. > + if (!VEC_empty (btrace_block_s, btinfo->btrace)) > + { > + struct regcache *regcache; > + struct btrace_block *head; > + > + regcache = get_thread_regcache (tp->ptid); > + head = VEC_index (btrace_block_s, btinfo->btrace, 0); > + if (head != NULL && head->end == 0) > + head->end = regcache_read_pc (regcache); > + } > + > + btinfo->itrace = compute_itrace (btinfo->btrace); > + > + /* Initialize branch trace iterators. */ > + btrace_init_insn_iterator (btinfo); > + btrace_init_call_iterator (btinfo); > +} > + > +/* Update the branch trace for the current thread and return a pointer to its > + branch trace information struct. > + > + Fails if there is no thread or no trace. */ /* Throws an error if there is no thread or no trace. This function never returns NULL. */ > + > +static struct btrace_thread_info * > +require_btrace (void) > +{ > + struct thread_info *tp; > + struct btrace_thread_info *btinfo; > + > + DEBUG_FUN ("require trace"); > + > + tp = find_thread_ptid (inferior_ptid); When possible/feasible we try to no longer use inferior_ptid in GDB, make it a ptid_t parameter instead. > + btinfo = &tp->btrace; > + > + if (tp == NULL) Do not initialize btinfo from NULL tp (I know it does not crash but still it is not clean). > + error (_("No thread.")); > + > + fetch_btrace (tp); > + > + if (VEC_empty (btrace_inst_s, btinfo->itrace)) > + error (_("No trace.")); > + > + return btinfo; > +} > + > +/* Enable branch tracing for one thread. */ > + > +static void > +record_btrace_enable (struct thread_info *tp) > +{ > + DEBUG_FUN ("enable thread %d (%s)", tp->num, target_pid_to_str (tp->ptid)); > + > + if (tp->btrace.target != NULL) > + error (_("Thread %d (%s) is already traced."), > + tp->num, target_pid_to_str (tp->ptid)); > + > + tp->btrace.target = target_enable_btrace (tp->ptid); > +} > + > +/* Enable branch tracing for one thread. Warn on errors. */ > + > +static void > +record_btrace_enable_warn (struct thread_info *tp) > +{ > + volatile struct gdb_exception error; > + > + TRY_CATCH (error, RETURN_MASK_ERROR) > + record_btrace_enable (tp); > + > + if (error.message != NULL) > + warning ("%s", error.message); > +} > + > +/* Disable branch tracing for one thread. */ > + > +static void > +record_btrace_disable (struct thread_info *tp) > +{ > + struct btrace_thread_info *btp; > + > + DEBUG_FUN ("disable thread %d (%s)", tp->num, target_pid_to_str (tp->ptid)); > + > + btp = &tp->btrace; > + if (btp->target == NULL) > + error (_("Thread %d (%s) is not traced."), > + tp->num, target_pid_to_str (tp->ptid)); > + > + target_disable_btrace (btp->target); > + btp->target = NULL; > + > + VEC_free (btrace_block_s, btp->btrace); No need to VEC_free btp->itrace? > +} > + > +/* Callback function to enable branch tracing for one thread. */ > + > +static void > +record_btrace_disable_callback (void *arg) > +{ > + volatile struct gdb_exception error; > + struct thread_info *tp; > + > + tp = arg; > + > + TRY_CATCH (error, RETURN_MASK_ERROR) > + record_btrace_disable (tp); > + > + if (error.message != NULL) > + warning ("%s", error.message); > +} > + > +/* Enable automatic tracing of new threads. */ > + > +static void > +record_btrace_auto_enable (void) > +{ > + DEBUG_FUN ("attach thread observer"); > + > + record_btrace_thread_observer > + = observer_attach_new_thread (record_btrace_enable_warn); > +} > + > +/* Disable automatic tracing of new threads. */ > + > +static void > +record_btrace_auto_disable (void) > +{ > + /* The observer may have been detached, already. */ > + if (record_btrace_thread_observer == NULL) > + return; > + > + DEBUG_FUN ("detach thread observer"); > + > + observer_detach_new_thread (record_btrace_thread_observer); > + record_btrace_thread_observer = NULL; > +} > + > +/* The to_open method of target record-btrace. */ > + > +static void > +record_btrace_open (char *args, int from_tty) > +{ > + struct cleanup *disable_chain; > + struct thread_info *tp; > + > + DEBUG_FUN ("open"); > + > + if (RECORD_IS_USED) > + error (_("The process is already being recorded.")); > + > + if (!target_has_execution) > + error (_("The program is not being run.")); > + > + if (!target_supports_btrace ()) > + error (_("Target does not support branch tracing.")); > + > + gdb_assert (record_btrace_thread_observer == NULL); > + > + disable_chain = make_cleanup (null_cleanup, NULL); > + ALL_THREADS (tp) > + if (args == NULL || *args == 0 || number_is_in_list (args, tp->num)) > + { > + record_btrace_enable (tp); > + > + (void) make_cleanup (record_btrace_disable_callback, tp); > + } > + > + record_btrace_auto_enable (); > + > + push_target (&record_btrace_ops); > + > + observer_notify_record_changed (current_inferior (), 1); > + > + discard_cleanups (disable_chain); > +} > + > +/* The to_close method of target record-btrace. */ > + > +static void > +record_btrace_close (int quitting) > +{ > + struct thread_info *tp; > + > + DEBUG_FUN ("close"); > + > + /* Disable tracing for traced threads. We use the ~_callback variant to > + turn errors into warnings since we cannot afford to throw an error. */ > + ALL_THREADS (tp) > + if (tp->btrace.target) > + record_btrace_disable_callback (tp); > + > + record_btrace_auto_disable (); > +} > + > +/* The to_info_record method of target record-btrace. */ > + > +static void > +record_btrace_info (void) > +{ > + struct btrace_thread_info *btinfo; > + unsigned int blocks, insts; > + > + DEBUG_FUN ("info"); > + > + btinfo = require_btrace (); > + blocks = VEC_length (btrace_block_s, btinfo->btrace); > + insts = VEC_length (btrace_inst_s, btinfo->itrace); > + > + printf_unfiltered (_("Recorded %u instructions in %u blocks.\n"), > + insts, blocks); It may be nice to print also the current thread target_pid_to_str there to make clear the information is thread-specific. > +} > + > +/* Disassemble a section of the recorded instruction trace. */ > + > +static void > +btrace_insn_history (struct btrace_thread_info *btinfo, struct ui_out *uiout, > + unsigned int begin, unsigned int end, int flags) > +{ > + struct gdbarch *gdbarch; > + unsigned int idx; > + > + DEBUG_FUN ("itrace (0x%x): [%u; %u[", flags, begin, end); > + > + gdbarch = target_gdbarch (); > + > + for (idx = begin; idx < end; ++idx) btrace_function->{begin,end} are inclusive but these are apparently begin inclusive but end exclusive parameters. > + { > + struct btrace_inst *inst; > + > + inst = VEC_index (btrace_inst_s, btinfo->itrace, idx); > + > + /* Disassembly with '/m' flag may not produce the expected result. > + See PR gdb/11833. */ > + gdb_disassembly (gdbarch, uiout, NULL, flags, 1, inst->ip, inst->ip + 1); > + } > +} > + > +/* The to_insn_history method of target record-btrace. */ > + > +static void > +record_btrace_insn_history (int size, int flags) > +{ > + struct btrace_thread_info *btinfo; > + struct cleanup *uiout_cleanup; > + struct ui_out *uiout; > + unsigned int context, last, begin, end; > + > + uiout = current_uiout; > + uiout_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, > + "insn history"); > + btinfo = require_btrace (); > + last = VEC_length (btrace_inst_s, btinfo->itrace); > + > + context = abs (size); > + begin = btinfo->insn_iterator.begin; > + end = btinfo->insn_iterator.end; > + > + DEBUG_FUN ("insn-history (0x%x): %d, prev: [%u; %u[", > + flags, size, begin, end); > + > + if (context == 0) > + error (_("Bad record instruction-history-size.")); > + > + /* We start at the end. */ > + if (end < begin) > + { > + /* Truncate the context, if necessary. */ > + context = min (context, last); > + > + end = last; > + begin = end - context; > + } > + else if (size < 0) > + { > + if (begin == 0) > + { > + printf_unfiltered (_("At the start of the branch trace record.\n")); > + > + btinfo->insn_iterator.end = 0; > + return; > + } > + > + /* Truncate the context, if necessary. */ > + context = min (context, begin); > + > + end = begin; > + begin -= context; > + } > + else > + { > + if (end == last) > + { > + printf_unfiltered (_("At the end of the branch trace record.\n")); > + > + btinfo->insn_iterator.begin = last; > + return; > + } > + > + /* Truncate the context, if necessary. */ > + context = min (context, last - end); > + > + begin = end; > + end += context; > + } > + > + btrace_insn_history (btinfo, uiout, begin, end, flags); > + > + btinfo->insn_iterator.begin = begin; > + btinfo->insn_iterator.end = end; > + > + do_cleanups (uiout_cleanup); > +} > + > +/* The to_insn_history_range method of target record-btrace. */ > + > +static void > +record_btrace_insn_history_range (ULONGEST from, ULONGEST to, int flags) > +{ > + struct btrace_thread_info *btinfo; > + struct cleanup *uiout_cleanup; > + struct ui_out *uiout; > + unsigned int last, begin, end; > + > + uiout = current_uiout; > + uiout_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, > + "insn history"); > + btinfo = require_btrace (); > + last = VEC_length (btrace_inst_s, btinfo->itrace); > + > + begin = (unsigned int) from; > + end = (unsigned int) to; > + > + DEBUG_FUN ("insn history (0x%x): [%u; %u[", flags, begin, end); > + > + /* Check for wrap-arounds. */ > + if (begin != from || end != to) > + error (_("Bad range.")); > + > + if (end <= begin) > + error (_("Bad range.")); > + > + if (last <= begin) > + error (_("Range out of bounds.")); > + > + /* Truncate the range, if necessary. */ > + if (last < end) > + end = last; > + > + btrace_insn_history (btinfo, uiout, begin, end, flags); > + > + btinfo->insn_iterator.begin = begin; > + btinfo->insn_iterator.end = end; > + > + do_cleanups (uiout_cleanup); > +} > + > +/* Initialize a recorded function segment. */ > + > +static void > +btrace_init_function (struct btrace_function *bfun, > + struct minimal_symbol *mfun, > + struct symbol *fun) > +{ > + memset (bfun, 0, sizeof (*bfun)); > + > + bfun->mfun = mfun; > + bfun->fun = fun; > + bfun->begin = INT_MAX; > + > + /* Initialize file and function name based on the information we have. */ > + if (fun != NULL) > + { > + bfun->filename = symtab_to_filename_for_display (fun->symtab); Do not store the result of symtab_to_filename_for_display, it is intended only for immediate use - its output may change by "set filename-display". Moreover the result is _for_display - so not for any comparisons. You can store fun->symtab itself. But in such case one needs to hook a function to free_objfile like there is now hooked breakpoint_free_objfile so that there do not remain stale symtab pointers. > + bfun->function = SYMBOL_PRINT_NAME (fun); Do not store the output of SYMBOL_PRINT_NAME. You can store FUN itself (in such case sure you no longer need to store fun->symtab as suggested above). Again you need a hook in free_objfile to prevent stale FUN pointers. > + } > + else if (mfun != NULL) > + { > + bfun->filename = mfun->filename; mfun->filename is not used in GDB, it is just a basename available only in some cases, for minimal symbols GDB does not know their source filename. > + bfun->function = SYMBOL_PRINT_NAME (mfun); I think the best is to just store FUN and MFUN themselves. > + } > + else > + internal_error (__FILE__, __LINE__, _("Require function.")); > + > + DEBUG_CALL ("init: fun = %s, file = %s, begin = %d, end = %d", > + bfun->function, bfun->filename ? bfun->filename : "", > + bfun->begin, bfun->end); > +} > + > +/* Update the line information in a recorded function segment. */ > + > +static void > +btrace_function_update_lines (struct btrace_function *bfun, int line, > + CORE_ADDR ip) > +{ > + bfun->begin = min (bfun->begin, line); > + bfun->end = max (bfun->end, line); > + > + DEBUG_CALL ("lines at %s: begin: %d, end: %d", > + core_addr_to_string_nz (ip), bfun->begin, bfun->end); > +} > + > +/* Check if we should skip this file when generating the function call > + history. > + Update the recorded function segment. */ I do not see a reason for this functionality. There really may be a valid one but I do not see it. Could you provide an example in the comment when it is useful? I cannot much review it when I do not see what is it good for. > + > +static int > +btrace_function_skip_file (struct btrace_function *bfun, > + const char *filename) > +{ > + /* Update the filename in case we didn't get it from the function symbol. */ > + if (bfun->filename == NULL) > + { > + DEBUG_CALL ("file: '%s'", filename); > + > + bfun->filename = filename; > + return 0; > + } > + > + /* Check if we're still in the same file. */ > + if (compare_filenames_for_search (bfun->filename, filename)) > + return 0; Use filename_cmp for full pathnames. Therefore always use symtab_to_fullname. The result of symtab_to_fullname call as 'const char *fullname'. > + > + /* We switched files, but not functions. Skip this file. */ > + DEBUG_CALL ("ignoring file '%s' for %s in %s.", filename, > + bfun->function, bfun->filename); > + return 1; > +} > + > +/* Print a line in the function call history. */ > + > +static void > +btrace_print_function (struct ui_out *uiout, struct btrace_function *bfun, > + int flags) flags should be enum. > +{ > + DEBUG_CALL ("print (0x%x): fun = %s, file = %s, begin = %d, end = %d", flags, > + bfun->function, bfun->filename ? bfun->filename : "", > + bfun->begin, bfun->end); > + > + if (flags & record_print_src_line) > + { > + const char *filename; > + int begin, end; > + > + filename = bfun->filename; > + if (filename == NULL || *filename == 0) > + filename = ""; > + > + ui_out_field_string (uiout, "file", filename); Just do not print anything (no "filename:" prefix) for minimal symbols; also omit the ui_out_field_string call so that in (future) MI the field "file" just does not exist. > + > + begin = bfun->begin; > + end = bfun->end; > + > + if (end != 0) > + { > + ui_out_text (uiout, ":"); > + ui_out_field_int (uiout, "min line", begin); > + > + if (end != begin) > + { > + ui_out_text (uiout, "-"); > + ui_out_field_int (uiout, "max line", end); > + } > + } > + > + ui_out_text (uiout, "\t "); > + } > + > + ui_out_field_string (uiout, "function", bfun->function); > + ui_out_text (uiout, "\n"); > +} > + > +/* Compute the function call history. > + Called for each instruction in the specified range. */ > + > +static void > +btrace_update_function (VEC (btr_fun_s) **bt, const struct btrace_inst *inst) > +{ > + struct btrace_function *bfun; > + struct symtab_and_line line; > + struct minimal_symbol *mfun; > + struct symbol *fun; > + CORE_ADDR ip; > + > + mfun = NULL; > + fun = NULL; > + ip = inst->ip; > + > + /* Try to determine the function we're in. We use both types of symbols to > + avoid surprises when we sometimes get a full symbol and sometimes only a > + minimal symbol. > + Not sure whether this is possible, at all. */ One can get all 4 combinations of full vs. minimal symbols availability. Although the case of missing minimal symbol with existing full symbol should not normally happen. > + fun = find_pc_function (ip); > + mfun = lookup_minimal_symbol_by_pc (ip); > + > + if (fun == NULL && mfun == NULL) > + return; > + > + /* Get the current function, if we already have one. */ > + if (*bt != NULL) You want rather more safe (against allocated but zero-sized): if (!VEC_empty (btr_fun_s, *bt)) > + bfun = VEC_last (btr_fun_s, *bt); > + else > + bfun = NULL; > + > + /* If we're switching functions, we start over. */ > + if (bfun == NULL || (fun != bfun->fun && mfun != bfun->mfun)) One should compare them using strcmp_iw for SYMBOL_PRINT_NAME strings of the functions. 'struct symbol *' will differ for example for an inlined function in two different compilation units (=two different symtabs). Besides SYMBOL_PRINT_NAME one should also use filename_cmp to compare symtab_to_fullname of both locations so that file1.c:staticfunc and file2.c:staticfunc get recognized as different functions (as you also print the 'file1.c:' prefix in the output). > + { > + bfun = VEC_safe_push (btr_fun_s, *bt, NULL); > + > + btrace_init_function (bfun, mfun, fun); > + } > + > + /* Let's see if we have source correlation, as well. */ > + line = find_pc_line (ip, 0); Such variable is commonly called 'sal' (source-and-line) in GDB. > + if (line.symtab != NULL) > + { > + const char *filename; > + > + /* Without a filename, we ignore this instruction. */ > + filename = symtab_to_filename_for_display (line.symtab); > + if (filename == NULL) > + return; As you have non-NULL symtab here you always have non-NULL filename. You should use symtab_to_fullname here as discussed elsewhere. > + > + /* Do this check first to make sure we get a filename even if we don't > + have line information. */ > + if (btrace_function_skip_file (bfun, filename)) But I do not see a reason for this function as written at btrace_function_skip_file. > + return; > + > + if (line.line == 0) > + return; > + > + btrace_function_update_lines (bfun, line.line, ip); > + } > +} > + > +/* Print the function call history of an instruction trace section. > + > + This would be faster on block trace level, but we might miss inlined > + functions, in this case. */ One should always document the function parameters, begin/end/size/flags is unclear for a new reader. > + > +static void > +btrace_call_history (struct btrace_thread_info *btinfo, struct ui_out *uiout, > + unsigned int begin, unsigned int end, unsigned int size, > + int flags) flags should be enum. > +{ > + struct cleanup *cleanup; > + struct btrace_function *bfun; > + VEC (btr_fun_s) *history; > + unsigned int idx; > + > + if (size == -1) > + DEBUG_FUN ("ftrace (0x%x): [%u; %u[", flags, begin, end); > + else > + DEBUG_FUN ("ftrace (0x%x): [%u; %u[, max %u", flags, begin, end, size); > + > + history = NULL; > + cleanup = make_cleanup (VEC_cleanup (btr_fun_s), &history); > + > + for (idx = begin; idx < end && VEC_length (btr_fun_s, history) <= size; ++idx) > + { > + struct btrace_inst *inst; > + > + inst = VEC_index (btrace_inst_s, btinfo->itrace, idx); > + btrace_update_function (&history, inst); > + } > + > + /* Update end to how far we actually got. */ > + end = idx; > + > + /* If we reached the size limit, there will be an extra function segment. */ > + if (size < VEC_length (btr_fun_s, history)) > + { > + VEC_pop (btr_fun_s, history); > + end -= 1; > + } > + > + /* Print the recorded function segments. */ > + for (idx = 0; VEC_iterate (btr_fun_s, history, idx, bfun); ++idx) > + btrace_print_function (uiout, bfun, flags); > + > + do_cleanups (cleanup); > + > + btinfo->call_iterator.begin = begin; > + btinfo->call_iterator.end = end; > +} > + > +/* Print the function call history of an instruction trace section. > + > + This function is similar to the above, except that it collects the s/above/btrace_call_history/ > + function call history in reverse order from end to begin. It is > + then printed in reverse, i.e. control-flow, order. */ > + > +static void > +btrace_reverse_call_history (struct btrace_thread_info *btinfo, > + struct ui_out *uiout, unsigned int begin, > + unsigned int end, unsigned int size, int flags) > +{ > + struct cleanup *cleanup; > + VEC (btr_fun_s) *history; > + unsigned int idx; > + > + if (size == -1) > + DEBUG_FUN ("ftrace-r itrace (0x%x): [%u; %u[", flags, begin, end); > + else > + DEBUG_FUN ("ftrace-r itrace (0x%x): [%u; %u[, max %u", > + flags, begin, end, size); > + > + history = NULL; > + cleanup = make_cleanup (VEC_cleanup (btr_fun_s), &history); > + > + for (idx = end; begin < idx && VEC_length (btr_fun_s, history) <= size;) > + { > + struct btrace_inst *inst; > + > + inst = VEC_index (btrace_inst_s, btinfo->itrace, --idx); > + btrace_update_function (&history, inst); > + } > + > + /* Update begin to how far we actually got. */ > + begin = idx; > + > + /* If we reached the size limit, there will be an extra function segment. */ > + if (size < VEC_length (btr_fun_s, history)) > + { > + VEC_pop (btr_fun_s, history); > + begin += 1; > + } > + > + /* Print the recorded function segments in reverse order. */ > + for (idx = VEC_length (btr_fun_s, history); idx != 0;) > + { > + struct btrace_function *bfun; > + > + bfun = VEC_index (btr_fun_s, history, --idx); > + btrace_print_function (uiout, bfun, flags); > + } > + > + do_cleanups (cleanup); > + > + btinfo->call_iterator.begin = begin; > + btinfo->call_iterator.end = end; > +} > + > +/* The to_call_history method of target record-btrace. */ > + > +static void > +record_btrace_call_history (int size, int flags) > +{ > + struct btrace_thread_info *btinfo; > + struct cleanup *uiout_cleanup; > + struct ui_out *uiout; > + unsigned int context, last, begin, end; > + > + uiout = current_uiout; > + uiout_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, > + "call history"); > + btinfo = require_btrace (); > + last = VEC_length (btrace_inst_s, btinfo->itrace); > + > + begin = btinfo->call_iterator.begin; > + end = btinfo->call_iterator.end; > + > + DEBUG_FUN ("ftrace (0x%x): %d, prev: [%u; %u[", flags, size, begin, end); > + > + context = abs (size); > + if (context == 0) > + error (_("Bad record function-call-history-size.")); > + > + /* We start at the end. */ > + if (end < begin) > + btrace_reverse_call_history (btinfo, uiout, 0, last, context, flags); > + else if (size < 0) > + { > + if (begin == 0) > + { > + printf_unfiltered (_("At the start of the branch trace record.\n")); > + > + btinfo->call_iterator.end = 0; > + return; > + } > + > + btrace_reverse_call_history (btinfo, uiout, 0, begin, context, flags); > + } > + else > + { > + if (end == last) > + { > + printf_unfiltered (_("At the end of the branch trace record.\n")); > + > + btinfo->call_iterator.begin = last; > + return; > + } > + > + btrace_call_history (btinfo, uiout, end, last, context, flags); > + } > + > + do_cleanups (uiout_cleanup); > +} > + > +/* The to_call_history_from method of target record-btrace. */ > + > +static void > +record_btrace_call_history_from (ULONGEST from, int size, int flags) > +{ > + struct btrace_thread_info *btinfo; > + struct cleanup *uiout_cleanup; > + struct ui_out *uiout; > + unsigned int context, last, begin; > + > + begin = (unsigned int) from; > + > + DEBUG_FUN ("ftrace-from (0x%x): %u, %d", flags, begin, size); > + > + uiout = current_uiout; > + uiout_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, > + "call history"); > + btinfo = require_btrace (); > + last = VEC_length (btrace_inst_s, btinfo->itrace); > + > + context = abs (size); > + if (context == 0) > + error (_("Bad record function-call-history-size.")); > + > + /* Check for wrap-arounds. */ > + if (begin != from) > + error (_("Bad range.")); > + > + if (last <= begin) > + error (_("Range out of bounds.")); > + > + if (size < 0) > + btrace_reverse_call_history (btinfo, uiout, 0, begin, context, flags); > + else > + btrace_call_history (btinfo, uiout, begin, last, context, flags); > + > + do_cleanups (uiout_cleanup); > +} > + > +/* The to_call_history_range method of target record-btrace. */ > + > +static void > +record_btrace_call_history_range (ULONGEST from, ULONGEST to, int flags) > +{ > + struct btrace_thread_info *btinfo; > + struct cleanup *uiout_cleanup; > + struct ui_out *uiout; > + unsigned int last, begin, end; > + > + begin = (unsigned int) from; > + end = (unsigned int) to; > + > + DEBUG_FUN ("bt range (0x%x): [%u; %u[", flags, begin, end); > + > + uiout = current_uiout; > + uiout_cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, > + "call history"); > + btinfo = require_btrace (); > + last = VEC_length (btrace_inst_s, btinfo->itrace); > + > + /* Check for wrap-arounds. */ > + if (begin != from || end != to) > + error (_("Bad range.")); > + > + if (end <= begin) > + error (_("Bad range.")); > + > + if (last <= begin) > + error (_("Range out of bounds.")); > + > + /* Truncate the range, if necessary. */ > + if (last < end) > + end = last; > + > + btrace_call_history (btinfo, uiout, begin, end, -1, flags); > + > + do_cleanups (uiout_cleanup); > +} > + > +/* Initialize the record-btrace target ops. */ > + > +static void > +init_record_btrace_ops (void) > +{ > + struct target_ops *ops; > + > + ops = &record_btrace_ops; > + ops->to_shortname = "record-btrace"; > + ops->to_longname = "Branch tracing target"; > + ops->to_doc = "Collect control-flow trace and provide the execution history."; > + ops->to_open = record_btrace_open; > + ops->to_close = record_btrace_close; > + ops->to_detach = record_detach; > + ops->to_disconnect = record_disconnect; > + ops->to_mourn_inferior = record_mourn_inferior; > + ops->to_kill = record_kill; > + ops->to_info_record = record_btrace_info; > + ops->to_insn_history = record_btrace_insn_history; > + ops->to_insn_history_range = record_btrace_insn_history_range; > + ops->to_call_history = record_btrace_call_history; > + ops->to_call_history_from = record_btrace_call_history_from; > + ops->to_call_history_range = record_btrace_call_history_range; > + ops->to_stratum = record_stratum; > + ops->to_magic = OPS_MAGIC; > +} > + > +/* Alias for "target record". */ > + > +static void > +cmd_record_btrace_start (char *args, int from_tty) > +{ > + if (args != NULL && *args != 0) > + error (_("Invalid argument.")); > + > + execute_command ("target record-btrace", from_tty); > +} > + > +void _initialize_record_btrace (void); > + > +/* Initialize btrace commands. */ > + > +void > +_initialize_record_btrace (void) > +{ > + add_cmd ("btrace", class_obscure, cmd_record_btrace_start, > + _("Start branch trace recording."), > + &record_cmdlist); > + add_alias_cmd ("b", "btrace", class_obscure, 1, &record_cmdlist); > + > + init_record_btrace_ops (); > + add_target (&record_btrace_ops); > +} > -- > 1.7.1 Thanks, Jan