This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/3] record, btrace: add record-btrace target
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 <markus.t.metzger@intel.com>
>
> * 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) <itrace, insn_iterator,
> 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. <markus.t.metzger@intel.com>
> +
> + 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 <http://www.gnu.org/licenses/>. */
> +
> +#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 : "<null>",
> + 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 : "<null>",
> + 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 = "<unknown>";
> +
> + 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