[PATCH 0/3] target record-btrace

[PATCH 0/3] target record-btrace

[markus.t.metzger]

From: Markus Metzger <markus.t.metzger@intel.com>

This patch series adds a new target record-btrace and implements the
two commands I added previously.

What's missing for the btrace series is porting the tests from the
previously added gdb.btrace suite and removing the old btrace cli.
I plan to send patches this week to complete the series.

I will keep track of approvals for individual patches.  When the
series is complete, I will send out the entire series for final
approval.

The complete series except for the doc changes in the last patch of
this series has been committed into archer-mmetzger-btrace.


Markus Metzger (3):
  record, btrace: add record-btrace target
  record-btrace, disas: omit pc prefix
  doc, record: document record changes

 gdb/Makefile.in            |    4 +-
 gdb/NEWS                   |   26 ++
 gdb/btrace.h               |   26 ++
 gdb/common/btrace-common.h |   20 +-
 gdb/disasm.c               |    4 +-
 gdb/disasm.h               |    1 +
 gdb/doc/gdb.texinfo        |  200 +++++++--
 gdb/record-btrace.c        | 1025 ++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 1265 insertions(+), 41 deletions(-)
 create mode 100644 gdb/record-btrace.c

[PATCH 3/3] doc, record: document record changes

[markus.t.metzger]

From: Markus Metzger <markus.t.metzger@intel.com>

Document changes to the record target resulting from the renaming into
record-full.

Document two new record sub-commands "record instruction-history" and
"record function-call-history" and two associated set/show commands
"set record instruction-history-size" and "set record
function-call-history-size".

Add this to NEWS.

2013-02-25 Markus Metzger <markus.t.metzger@intel.com>

	* NEWS: Add record changes.

doc/
	* gdb.texinfo (Process Record and Replay): Document record
	changes.


---
 gdb/NEWS            |   26 +++++++
 gdb/doc/gdb.texinfo |  200 ++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 191 insertions(+), 35 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 38b36c2..fbd01af 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,32 @@
 
 *** Changes since GDB 7.5
 
+* Target record has been renamed to record-full.
+  Record/replay is now enabled with the "record full" command.
+  This also affects settings that are associated with full record/replay
+  that have been moved from "set/show record" to "set/show record full":
+
+record insn-number-max
+record stop-at-limit
+record memory-query
+
+* Two new commands have been added for record/replay to give information
+  about the recorded execution without having to replay the execution.
+
+"record instruction-history" disassembles instructions stored in the
+execution log.
+
+"record function-call-history" prints the names of the functions
+from instructions stored in the execution log.
+
+* A new record target "record-btrace" has been added.  The new target
+  uses hardware support to record the control-flow of a process.  It
+  does not support replaying the execution, but it implements the
+  above commands for investigating the recorded execution log.
+
+  The "record-btrace" target is only available on Intel Atom processors
+  and requires a Linux kernel 2.6.32 or later.
+
 * New native configurations
 
 ARM AArch64 GNU/Linux		aarch64*-*-linux-gnu
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 26dced6..b5fb76c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6118,16 +6118,38 @@ For architecture environments that support process record and replay,
 
 @table @code
 @kindex target record
+@kindex target record-full
+@kindex target record-btrace
 @kindex record
+@kindex record full
+@kindex record btrace
 @kindex rec
-@item target record
-This command starts the process record and replay target.  The process
-record and replay target can only debug a process that is already
-running.  Therefore, you need first to start the process with the
-@kbd{run} or @kbd{start} commands, and then start the recording with
-the @kbd{target record} command.
+@kindex rec full
+@kindex rec btrace
+@item record @var{method}
+This command starts the process record and replay target.  The
+recording method can be specified as parameter.  Without a parameter
+the command uses the @var{full} recording method.  The following
+recording methods are available:
 
-Both @code{record} and @code{rec} are aliases of @code{target record}.
+@table @code
+@item full
+Full record/replay recording using @value{GDBN}'s software record and
+replay implementation.  This method allows replaying and reverse
+execution.
+
+@item btrace
+Hardware-supported control-flow recording.  This method does not allow
+replaying and reverse execution.
+@end table
+
+The process record and replay target can only debug a process that is
+already running.  Therefore, you need first to start the process with
+the @kbd{run} or @kbd{start} commands, and then start the recording
+with the @kbd{record <method>} command.
+
+Both @code{record <method>} and @code{rec <method>} are aliases of
+@code{target record-<method>}.
 
 @cindex displaced stepping, and process record and replay
 Displaced stepping (@pxref{Maintenance Commands,, displaced stepping})
@@ -6138,9 +6160,9 @@ doesn't support displaced stepping.
 @cindex non-stop mode, and process record and replay
 @cindex asynchronous execution, and process record and replay
 If the inferior is in the non-stop mode (@pxref{Non-Stop Mode}) or in
-the asynchronous execution mode (@pxref{Background Execution}), the
-process record and replay target cannot be started because it doesn't
-support these two modes.
+the asynchronous execution mode (@pxref{Background Execution}), not
+all recording methods are available.  The @var{full} recording method
+does not support these two modes.
 
 @kindex record stop
 @kindex rec s
@@ -6170,14 +6192,17 @@ Save the execution log to a file @file{@var{filename}}.
 Default filename is @file{gdb_record.@var{process_id}}, where
 @var{process_id} is the process ID of the inferior.
 
+This command may not be available for all recording methods.
+
 @kindex record restore
 @item record restore @var{filename}
 Restore the execution log from a file @file{@var{filename}}.
 File must have been created with @code{record save}.
 
-@kindex set record insn-number-max
-@item set record insn-number-max @var{limit}
-Set the limit of instructions to be recorded.  Default value is 200000.
+@kindex set record full insn-number-max
+@item set record full insn-number-max @var{limit}
+Set the limit of instructions to be recorded for the @var{full}
+recording method.  Default value is 200000.
 
 If @var{limit} is a positive number, then @value{GDBN} will start
 deleting instructions from the log once the number of the record
@@ -6192,31 +6217,34 @@ If @var{limit} is zero, @value{GDBN} will never delete recorded
 instructions from the execution log.  The number of recorded
 instructions is unlimited in this case.
 
-@kindex show record insn-number-max
-@item show record insn-number-max
-Show the limit of instructions to be recorded.
-
-@kindex set record stop-at-limit
-@item set record stop-at-limit
-Control the behavior when the number of recorded instructions reaches
-the limit.  If ON (the default), @value{GDBN} will stop when the limit
-is reached for the first time and ask you whether you want to stop the
-inferior or continue running it and recording the execution log.  If
-you decide to continue recording, each new recorded instruction will
-cause the oldest one to be deleted.
+@kindex show record full insn-number-max
+@item show record full insn-number-max
+Show the limit of instructions to be recorded with the @var{full}
+recording method.
+
+@kindex set record full stop-at-limit
+@item set record full stop-at-limit
+Control the behavior of the  @var{full} recording method when the
+number of recorded instructions reaches the limit.  If ON (the
+default), @value{GDBN} will stop when the limit is reached for the
+first time and ask you whether you want to stop the inferior or
+continue running it and recording the execution log.  If you decide
+to continue recording, each new recorded instruction will cause the
+oldest one to be deleted.
 
 If this option is OFF, @value{GDBN} will automatically delete the
 oldest record to make room for each new one, without asking.
 
-@kindex show record stop-at-limit
-@item show record stop-at-limit
+@kindex show record full stop-at-limit
+@item show record full stop-at-limit
 Show the current setting of @code{stop-at-limit}.
 
-@kindex set record memory-query
-@item set record memory-query
+@kindex set record full memory-query
+@item set record full memory-query
 Control the behavior when @value{GDBN} is unable to record memory
-changes caused by an instruction.  If ON, @value{GDBN} will query
-whether to stop the inferior in that case.
+changes caused by an instruction for the @var{full} recording method.
+If ON, @value{GDBN} will query whether to stop the inferior in that
+case.
 
 If this option is OFF (the default), @value{GDBN} will automatically
 ignore the effect of such instructions on memory.  Later, when
@@ -6224,14 +6252,19 @@ ignore the effect of such instructions on memory.  Later, when
 instruction as not accessible, and it will not affect the replay
 results.
 
-@kindex show record memory-query
-@item show record memory-query
+@kindex show record full memory-query
+@item show record full memory-query
 Show the current setting of @code{memory-query}.
 
 @kindex info record
 @item info record
-Show various statistics about the state of process record and its
-in-memory execution log buffer, including:
+Show various statistics about the recording depending on the recording
+method:
+
+@table @code
+@item full
+For the @var{full} recording method, it shows the state of process
+record and its in-memory execution log buffer, including:
 
 @itemize @bullet
 @item
@@ -6248,6 +6281,12 @@ Number of instructions contained in the execution log.
 Maximum number of instructions that may be contained in the execution log.
 @end itemize
 
+@item btrace
+For the @var{btrace} recording method, it shows the number of
+instructions that have been recorded and the number of blocks of
+sequential control-flow that is formed by the recorded instructions.
+@end table
+
 @kindex record delete
 @kindex rec del
 @item record delete
@@ -6255,6 +6294,97 @@ When record target runs in replay mode (``in the past''), delete the
 subsequent execution log and begin to record a new execution log starting
 from the current address.  This means you will abandon the previously
 recorded ``future'' and begin recording a new ``future''.
+
+@kindex record instruction-history
+@kindex rec instruction-history
+@item record instruction-history
+Disassembles instructions from the recorded execution log.  By
+default, ten instructions are disassembled.  This can be changed using
+the @code{set record instruction-history-size} command.  Instructions
+are printed in control-flow order.  There are several ways to specify
+what part of the execution log to disassemble:
+
+@table @code
+@item record instruction-history @var{insn}
+Disassembles ten instructions around instruction number @var{insn}.
+
+@item record instruction-history @var{insn}, +/-@var{context}
+Disassembles @var{context} instructions around instruction number
+@var{insn}.  If @var{context} is preceded with @code{+}, disassembles
+@var{context} instructions after instruction number @var{insn}.  If
+@var{context} is preceded with @code{-}, disassembles @var{context}
+instructions before instruction number @var{insn}.
+
+@item record instruction-history
+Disassembles ten more instructions after the last disassembly.
+
+@item record instruction-history -
+Disassembles ten more instructions before the last disassembly.
+
+@item record instruction-history @var{begin} @var{end}
+Disassembles instructions beginning with instruction number
+@var{begin} until instruction number @var{end}.  The instruction
+number @var{end} is not included.
+@end table
+
+This command may not be available for all recording methods.
+
+@kindex set record instruction-history-size
+@item set record instruction-history-size
+Define how many instructions to disassemble in the @code{record
+instruction-history} command.  The default value is 10.
+
+@kindex show record instruction-history-size
+@item show record instruction-history-size
+Show how many instructions to disassemble in the @code{record
+instruction-history} command.
+
+@kindex record function-call-history
+@kindex rec function-call-history
+@item record function-call-history
+Print function names for instructions stored in the recorded execution
+log.  Prints one line for each sequence of instructions that is
+correlated to the same function.  By default, ten function names are
+printed.  This can be changed using the @code{set record
+function-call-history-size} command.  Functions are printed in
+control-flow order.  There are several ways to specify what part of
+the execution log to consider:
+
+@table @code
+@item record function-call-history @var{insn}
+Prints ten function names starting from instruction number @var{insn}.
+
+@item record function-call-history @var{insn}, +/-@var{context}
+Prints @var{context} function names starting from instruction number
+@var{insn}.  If @var{context} is preceded with @code{+}, @var{context}
+function names are printed correlating to instructions preceding
+@var{insn}.  If @var{context} is preceded with @code{-}, @var{context}
+function names are printed correlating to instructions succeeding
+@var{insn}.
+
+@item record function-call-history
+Prints ten more function names after the last ten-line print.
+
+@item record function-call-history -
+Prints ten more function names before the last ten-line print.
+
+@item record function-call-history @var{begin} @var{end}
+Prints function names for instructions beginning with instruction
+number @var{begin} until instruction number @var{end}.  Instruction
+number @var{end} is not included.
+@end table
+
+This command may not be available for all recording methods.
+
+@kindex set record function-call-history-size
+@item set record function-call-history-size
+Define how many function names to print in the
+@code{record function-call-history} command.  The default value is 10.
+
+@kindex show record function-call-history-size
+@item show record function-call-history-size
+Show how many function names to print in the
+@code{record function-call-history} command.
 @end table
 
 
-- 
1.7.1

Re: [PATCH 3/3] doc, record: document record changes

[Eli Zaretskii]

> From: markus.t.metzger@intel.com
> Cc: gdb-patches@sourceware.org, markus.t.metzger@gmail.com,        Markus Metzger <markus.t.metzger@intel.com>
> Date: Mon, 25 Feb 2013 17:15:17 +0100

[May I ask that you use your address fewer than 3 times in the headers?]

> Document changes to the record target resulting from the renaming into
> record-full.
> 
> Document two new record sub-commands "record instruction-history" and
> "record function-call-history" and two associated set/show commands
> "set record instruction-history-size" and "set record
> function-call-history-size".
> 
> Add this to NEWS.

Thanks.  Some comments below.

> +"record function-call-history" prints the names of the functions
> +from instructions stored in the execution log.

"prints the names of the functions called by instructions in the
execution log"

> +The process record and replay target can only debug a process that is
> +already running.  Therefore, you need first to start the process with
> +the @kbd{run} or @kbd{start} commands, and then start the recording
> +with the @kbd{record <method>} command.
> +
> +Both @code{record <method>} and @code{rec <method>} are aliases of
> +@code{target record-<method>}.

In the above, instead of "<method>", please use "@var{method}".

> +@kindex show record full memory-query
> +@item show record full memory-query

I think it is good enough to have only one "@kindex set record" and
one "@kindex show record" entry (which you already have at the
beginning of this description), without the entries that advertise the
rest of the command arguments.  These varieties are all described
together, so the multitude of index entries does not have any useful
effect, it just bloats the index.

> +                                                         Instructions
> +are printed in control-flow order.

Is that the same as the execution order?  If so, perhaps use the
latter, as I think it is more easily understandable.

> +@item record instruction-history @var{insn}, +/-@var{context}
> +Disassembles @var{context} instructions around instruction number
> +@var{insn}.  If @var{context} is preceded with @code{+}, disassembles
> +@var{context} instructions after instruction number @var{insn}.  If
> +@var{context} is preceded with @code{-}, disassembles @var{context}
> +instructions before instruction number @var{insn}.

Perhaps it would be better to use @var{n} instead of @var{context}
here.  The latter does not specifically say that it is a number.

> +@item record function-call-history
> +Print function names for instructions stored in the recorded execution
> +log.  Prints one line for each sequence of instructions that is
> +correlated to the same function.

Isn't the last sentence equivalent to saying

  Prints one line for each function call in the execution log.

?  If it is equivalent, I think my suggested wording is more clear and
less technical.

> +                                      Functions are printed in
> +control-flow order.

Again, isn't that the order of execution?

> +@item record function-call-history @var{insn}, +/-@var{context}

Same comment as above regarding @var{context}.

> +Prints @var{context} function names starting from instruction number
> +@var{insn}.  If @var{context} is preceded with @code{+}, @var{context}
> +function names are printed correlating to instructions preceding
> +@var{insn}.  If @var{context} is preceded with @code{-}, @var{context}
> +function names are printed correlating to instructions succeeding
> +@var{insn}.

Isn't this backwards?  IOW, shouldn't + print functions _after_ the
instruction, while - should print functions called _before_ the
instruction?

RE: [PATCH 3/3] doc, record: document record changes

[Metzger, Markus T]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Tuesday, February 26, 2013 6:24 PM
> To: Metzger, Markus T

Thanks for your review.

> [May I ask that you use your address fewer than 3 times in the headers?]

I reduced it by one.


> > +"record function-call-history" prints the names of the functions
> > +from instructions stored in the execution log.
> 
> "prints the names of the functions called by instructions in the
> execution log"

I'm not sure whether "called" is the right term. The algorithm walks over
all instructions and collects the functions from which these instructions
originated.

Example:

1  void foo (void)
2 {
3    ...
4    bar ();
5    ...
6  }

When we record the execution of foo, there will be instructions for the
first ..., then instructions for bar, and then instructions for the second ....

The "record function-call-history" will print:
  foo.c:1-4	foo (void)
  bar.c:8-12	bar (void)
  foo.c:5-6	foo (void


> > +@kindex show record full memory-query
> > +@item show record full memory-query
> 
> I think it is good enough to have only one "@kindex set record" and
> one "@kindex show record" entry (which you already have at the
> beginning of this description), without the entries that advertise the
> rest of the command arguments.  These varieties are all described
> together, so the multitude of index entries does not have any useful
> effect, it just bloats the index.

I just renamed the text for existing indices. Do you still want me to
merge them?


> > +@item record function-call-history
> > +Print function names for instructions stored in the recorded execution
> > +log.  Prints one line for each sequence of instructions that is
> > +correlated to the same function.
> 
> Isn't the last sentence equivalent to saying
> 
>   Prints one line for each function call in the execution log.
> 
> ?  If it is equivalent, I think my suggested wording is more clear and
> less technical.

See above.

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 3/3] doc, record: document record changes

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "jan.kratochvil@redhat.com" <jan.kratochvil@redhat.com>,
> 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Fri, 1 Mar 2013 14:06:23 +0000
> 
> > > +"record function-call-history" prints the names of the functions
> > > +from instructions stored in the execution log.
> > 
> > "prints the names of the functions called by instructions in the
> > execution log"
> 
> I'm not sure whether "called" is the right term. The algorithm walks over
> all instructions and collects the functions from which these instructions
> originated.
> 
> Example:
> 
> 1  void foo (void)
> 2 {
> 3    ...
> 4    bar ();
> 5    ...
> 6  }
> 
> When we record the execution of foo, there will be instructions for the
> first ..., then instructions for bar, and then instructions for the second ....
> 
> The "record function-call-history" will print:
>   foo.c:1-4	foo (void)
>   bar.c:8-12	bar (void)
>   foo.c:5-6	foo (void

This indicates that "record function-execution-history" might be a
better name.  But in any case, weren't 'bar' and 'foo' called in this
example?

Your original text, "prints the names of the functions from
instructions", is confusing, since instructions don't store function
names, they store addresses and numbers.

> > > +@kindex show record full memory-query
> > > +@item show record full memory-query
> > 
> > I think it is good enough to have only one "@kindex set record" and
> > one "@kindex show record" entry (which you already have at the
> > beginning of this description), without the entries that advertise the
> > rest of the command arguments.  These varieties are all described
> > together, so the multitude of index entries does not have any useful
> > effect, it just bloats the index.
> 
> I just renamed the text for existing indices. Do you still want me to
> merge them?

Can you give one example of such renaming?  I'm not sure I understand
what you did.

> > > +@item record function-call-history
> > > +Print function names for instructions stored in the recorded execution
> > > +log.  Prints one line for each sequence of instructions that is
> > > +correlated to the same function.
> > 
> > Isn't the last sentence equivalent to saying
> > 
> >   Prints one line for each function call in the execution log.
> > 
> > ?  If it is equivalent, I think my suggested wording is more clear and
> > less technical.
> 
> See above.

Given the example (which I think it would be good to have in the
manual), I suggest this wording instead:

  Prints one line for each sequence of instructions that all belong to
  the same function.

RE: [PATCH 3/3] doc, record: document record changes

[Metzger, Markus T]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Friday, March 01, 2013 3:26 PM


> > > > +"record function-call-history" prints the names of the functions
> > > > +from instructions stored in the execution log.
> > >
> > > "prints the names of the functions called by instructions in the
> > > execution log"
> >
> > I'm not sure whether "called" is the right term. The algorithm walks over
> > all instructions and collects the functions from which these instructions
> > originated.
> >
> > Example:
> >
> > 1  void foo (void)
> > 2 {
> > 3    ...
> > 4    bar ();
> > 5    ...
> > 6  }
> >
> > When we record the execution of foo, there will be instructions for the
> > first ..., then instructions for bar, and then instructions for the second ....
> >
> > The "record function-call-history" will print:
> >   foo.c:1-4	foo (void)
> >   bar.c:8-12	bar (void)
> >   foo.c:5-6	foo (void
> 
> This indicates that "record function-execution-history" might be a
> better name.  But in any case, weren't 'bar' and 'foo' called in this
> example?

Yes, they were called.

What I have problems with is that foo was called once but we print
two lines for foo, one for instructions before the call to bar and one
for instructions after the return from bar.

Wouldn't "called" suggest that there be only one line for foo in the
above example?

 
> Your original text, "prints the names of the functions from
> instructions", is confusing, since instructions don't store function
> names, they store addresses and numbers.

I'm not exactly happy with my formulation, either.


> > > > +@kindex show record full memory-query
> > > > +@item show record full memory-query
> > >
> > > I think it is good enough to have only one "@kindex set record" and
> > > one "@kindex show record" entry (which you already have at the
> > > beginning of this description), without the entries that advertise the
> > > rest of the command arguments.  These varieties are all described
> > > together, so the multitude of index entries does not have any useful
> > > effect, it just bloats the index.
> >
> > I just renamed the text for existing indices. Do you still want me to
> > merge them?
> 
> Can you give one example of such renaming?  I'm not sure I understand
> what you did.

There had been a target "record" that I renamed to "record-full".
Then I added a new target "record-btrace" that provides similar, yet
different functionality.

There's a prefix-command "record" whose sub-commands operate on
target record.

Those sub-commands that are applicable to both targets, I left unchanged.
Those sub-commands that are specific to target "record-full", I put
under a new prefix-command "record full".

Same for "set record" and "show record".

In the documentation, there had been a @kindex show record memory-query.
I changed it to @kindex show record full memory-query.


> > > > +@item record function-call-history
> > > > +Print function names for instructions stored in the recorded execution
> > > > +log.  Prints one line for each sequence of instructions that is
> > > > +correlated to the same function.
> > >
> > > Isn't the last sentence equivalent to saying
> > >
> > >   Prints one line for each function call in the execution log.
> > >
> > > ?  If it is equivalent, I think my suggested wording is more clear and
> > > less technical.
> >
> > See above.
> 
> Given the example (which I think it would be good to have in the
> manual), I suggest this wording instead:
> 
>   Prints one line for each sequence of instructions that all belong to
>   the same function.

That's good. Thanks!

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 3/3] doc, record: document record changes

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "jan.kratochvil@redhat.com" <jan.kratochvil@redhat.com>,
> 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Fri, 1 Mar 2013 14:45:35 +0000
> 
> > > 1  void foo (void)
> > > 2 {
> > > 3    ...
> > > 4    bar ();
> > > 5    ...
> > > 6  }
> > >
> > > When we record the execution of foo, there will be instructions for the
> > > first ..., then instructions for bar, and then instructions for the second ....
> > >
> > > The "record function-call-history" will print:
> > >   foo.c:1-4	foo (void)
> > >   bar.c:8-12	bar (void)
> > >   foo.c:5-6	foo (void
> > 
> > This indicates that "record function-execution-history" might be a
> > better name.  But in any case, weren't 'bar' and 'foo' called in this
> > example?
> 
> Yes, they were called.
> 
> What I have problems with is that foo was called once but we print
> two lines for foo, one for instructions before the call to bar and one
> for instructions after the return from bar.
> 
> Wouldn't "called" suggest that there be only one line for foo in the
> above example?

Perhaps we should change our aspect angle and use this:

  "record function-call-history" prints the execution history at
  function granularity.

> In the documentation, there had been a @kindex show record memory-query.
> I changed it to @kindex show record full memory-query.

OK, but there should be only one entry that begins with "@kindex show
record full".

RE: [PATCH 3/3] doc, record: document record changes

[Metzger, Markus T]

> -----Original Message-----
> From: Eli Zaretskii [mailto:eliz@gnu.org]
> Sent: Friday, March 01, 2013 4:12 PM


> > > > 1  void foo (void)
> > > > 2 {
> > > > 3    ...
> > > > 4    bar ();
> > > > 5    ...
> > > > 6  }
> > > >
> > > > When we record the execution of foo, there will be instructions for the
> > > > first ..., then instructions for bar, and then instructions for the second ....
> > > >
> > > > The "record function-call-history" will print:
> > > >   foo.c:1-4	foo (void)
> > > >   bar.c:8-12	bar (void)
> > > >   foo.c:5-6	foo (void
> > >
> > > This indicates that "record function-execution-history" might be a
> > > better name.  But in any case, weren't 'bar' and 'foo' called in this
> > > example?
> >
> > Yes, they were called.
> >
> > What I have problems with is that foo was called once but we print
> > two lines for foo, one for instructions before the call to bar and one
> > for instructions after the return from bar.
> >
> > Wouldn't "called" suggest that there be only one line for foo in the
> > above example?
> 
> Perhaps we should change our aspect angle and use this:
> 
>   "record function-call-history" prints the execution history at
>   function granularity.

OK.


> > In the documentation, there had been a @kindex show record memory-query.
> > I changed it to @kindex show record full memory-query.
> 
> OK, but there should be only one entry that begins with "@kindex show
> record full".

There were several entries that begin with "@kindex show record" and now
there are several entries that begin with "@kindex show record full".

When I fix this, do I need to reorder the documentation such that all the
"set record full" and all the "show record full" are listed together?

At the moment, corresponding set and show commands are grouped together.

To make things more interesting, there are "set/show record", "set/show
record full", and "set/show record btrace" sub-commands.

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 3/3] doc, record: document record changes

[Eli Zaretskii]

> From: "Metzger, Markus T" <markus.t.metzger@intel.com>
> CC: "jan.kratochvil@redhat.com" <jan.kratochvil@redhat.com>,
> 	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> Date: Fri, 1 Mar 2013 15:20:15 +0000
> 
> > > In the documentation, there had been a @kindex show record memory-query.
> > > I changed it to @kindex show record full memory-query.
> > 
> > OK, but there should be only one entry that begins with "@kindex show
> > record full".
> 
> There were several entries that begin with "@kindex show record" and now
> there are several entries that begin with "@kindex show record full".

There should be only one of each.  And the same wrt "@kindex set record".

> When I fix this, do I need to reorder the documentation such that all the
> "set record full" and all the "show record full" are listed together?

As long as they are close to one another, no need to reorder.

> At the moment, corresponding set and show commands are grouped together.

That's fine.

> To make things more interesting, there are "set/show record", "set/show
> record full", and "set/show record btrace" sub-commands.

Each one should have only one @kindex for "set" and another one for
"show".

Thanks.

[PATCH 1/3] record, btrace: add record-btrace target

[markus.t.metzger]

From: Markus Metzger <markus.t.metzger@intel.com>

Add a target for branch trace recording.  This will replace the btrace
commands added earlier in the patch series.

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.

I'm not quite sure when to use ui_out_~ functions and when to use
printf_unfiltered.  I tried to copy what others are doing, but I'm
not sure whether I got it right.

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;
+};
+
+
+/* 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;
+
+  /* The name of the function.  */
+  const char *function;
+
+  /* The name of the file in which the function is defined.  */
+  const char *filename;
+
+  /* The min and max line in the above file.  */
+  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)					\
+        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;
+
+      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);
+
+  /* The first block ends at the current pc.  */
+  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.  */
+
+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);
+  btinfo = &tp->btrace;
+
+  if (tp == NULL)
+    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);
+}
+
+/* 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);
+}
+
+/* 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)
+    {
+      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);
+      bfun->function = SYMBOL_PRINT_NAME (fun);
+    }
+  else if (mfun != NULL)
+    {
+      bfun->filename = mfun->filename;
+      bfun->function = SYMBOL_PRINT_NAME (mfun);
+    }
+  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.  */
+
+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;
+
+  /* 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)
+{
+  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);
+
+      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.  */
+  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)
+    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))
+    {
+      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);
+  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;
+
+      /* 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))
+	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.  */
+
+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)
+{
+  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
+   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

Re: [PATCH 1/3] record, btrace: add record-btrace target

[Jan Kratochvil]

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

Re: [PATCH 1/3] record, btrace: add record-btrace target

[Jan Kratochvil]

On Wed, 27 Feb 2013 08:37:31 +0100, Jan Kratochvil wrote:
[...]
> > +/* 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)


Probably if one does:

file.c:
void func(void)
{
#include "file.def"
}

Then we want a single output line for file.c:func() without another output
line for file.def:func().  (It is not tested by the testsuite; OK.)

Therefore you apparently do not want to include the line numbers range of
file.def:func() into the range of lines of file.c:func(), that makes sense.

So my comments are valid, just use filename_cmp and compare symtab_to_fullname
of both symtabs (this is sub-optimal, comparing two source files for
equivalence can be made cheaper than finding out their full pathname; but that
is an optimization for another patch).





> > +{
> > +  /* 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;
> > +}


[...]
> > +  /* 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);
> > +    }
> > +}


Jan

RE: [PATCH 1/3] record, btrace: add record-btrace target

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Jan Kratochvil
> Sent: Wednesday, February 27, 2013 8:38 AM

Thanks for your review!


> 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?

Yes. With your help, I hope, on the reverse execution part.

The "record source-lines-history" command needs some experimentation and
feedback to get non-code source lines right and also to suppress the noise of
instruction scheduling.


> > +/* 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?

I think I should always have a minimal symbol. I may not always have a
full symbol. For example, when there is no debug information.

The algorithm will also work with a full but no minimal symbol, so
I added a comment saying that one of them 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.?

This field has been removed in response to other comments below.


> > +
> > +  /* The name of the file in which the function is defined.  */
> > +  const char *filename;
> 
> Absolute source filename? Or relative to compilation directory?

In response to other comments below, this has been changed to
the full name for full symbols and to whatever a minimal symbol
provides.


> > +  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?

Yes. I have updated the archer branch before sending this
patch series, though. 

 
> 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.

I don't think we should give an error if GDB wants to read the trace twice.

This extra call to target_btrace_has_changed was meant to save the
time of repeatedly sending and processing the same trace.


What we could do is free the trace when the target continues (or stops) and
set the trace pointers to NULL.  Would that be better?
If yes, is there already some notifier or hook that I could use?


> > +
> > +  /* 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.

I wanted to avoid the #ifdef GDBSERVER in the common code.

I added a separate patch to fill this in when reading the branch trace in
common/linux-btrace.c.


> > +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.

This function is called from command functions to get the branch trace for
the current thread.

At some point, I do need to get the current thread. If I made ptid_t a
parameter here, I would have to get the current ptid in all callers.


> > +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.

The [begin; end] range in struct btrace_function refers to source lines.
The [begin; end[ range here refers to indices into the instruction trace vector.


> > +  /* 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.

I can compute the filename when I intend to print a line, so I don't need to store
the symtab pointer separately.

I changed it to obtain the filename via a call to symtab_to_fullname. I store that
while I traverse the instruction trace vector so I can compare it to other filenames.
Would I need to duplicate the string or is it safe to store it as-is for my purpose?


> > +      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.

I changed it to compute the function name whenever I want to print it.

I'm only storing symbol and minimal symbol pointers for computing the output
of "record function-call-history". I guess I won't need the hook in that case.


> > +    }
> > +  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.

I use it only for functions for which I do not have a full symbol. The alternative
would be to not provide a filename in that case. Would that be preferable?

An example would be library functions for which we don't have debug info.


> On Wed, 27 Feb 2013 08:37:31 +0100, Jan Kratochvil wrote:
> > > +/* 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)
> 
> 
> Probably if one does:
> 
> file.c:
> void func(void)
> {
> #include "file.def"
> }

Suppose you're using a macro in func that has been defined in another file.
I added a comment for the macro.


> Then we want a single output line for file.c:func() without another output
> line for file.def:func().  (It is not tested by the testsuite; OK.)
> 
> Therefore you apparently do not want to include the line numbers range of
> file.def:func() into the range of lines of file.c:func(), that makes sense.
> 
> So my comments are valid, just use filename_cmp and compare symtab_to_fullname
> of both symtabs (this is sub-optimal, comparing two source files for
> equivalence can be made cheaper than finding out their full pathname; but that
> is an optimization for another patch).

OK. I would still use symtab_to_filename_for_display when displaying the filename, right?

So I would store the fullname for the above comparison and compute the filename to
display when printing the line.

What would I do if I only have a minimal symbol? Can I still use ->filename for this check?
See above for a related question on printing filenames.


> > +  /* 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'.

Can I store the returned pointer? Would I need to duplicate the string?


> > +      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.

That might affect a lot of library code for which we don't have debug info.


> > +    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).

OK. I assume that a full symbol will always have a symtab. Is this correct?


> > +
> > +      /* 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.

I believe this has been clarified, meanwhile. See above.


Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 1/3] record, btrace: add record-btrace target

[Jan Kratochvil]

On Thu, 28 Feb 2013 18:04:00 +0100, Metzger, Markus T wrote:
> The "record source-lines-history" command needs
[...]
> also to suppress the noise of instruction scheduling.

I do not think this is doable in GDB, or at least it is not right to
workaround it in GDB.  It expects a proper support of DW_LNS_negate_stmt in
-O2 -g code from GCC.  This would also fix the current "jumping" during
step-ping in -O2 -g code.


> > > +/* 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?
> 
> I think I should always have a minimal symbol. I may not always have a
> full symbol. For example, when there is no debug information.

Without debug information you commonly do not have even the minimal symbol.


> I added a comment saying that one of them may be NULL.

Thanks.


> > > +  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?
> 
> Yes. I have updated the archer branch before sending this
> patch series, though. 

I do not understand it much now - the archer branch
5dd7aa460b03c158b938fa76ff6c83927d597bd8 still contains update_btrace not
using VEC_free and fetch_btrace using VEC_free; so either one is wrong.


> I don't think we should give an error if GDB wants to read the trace twice.
> 
> This extra call to target_btrace_has_changed was meant to save the
> time of repeatedly sending and processing the same trace.

The problem is with large RTT (round-trip-time) gdb<->gdbserver it is even
cheaper to transfer more data than to ask twice (even for less data).


> What we could do is free the trace when the target continues (or stops) and
> set the trace pointers to NULL.  Would that be better?
> If yes, is there already some notifier or hook that I could use?

to_resume is there.

But I would find safer to leave the decision whether something changed or not
still to the target.  One could modify remote_read_btrace:

static VEC (btrace_block_s) *
remote_read_btrace (struct btrace_target_info *tinfo, int if_changed)
  xml = target_read_stralloc (&current_target,
                              TARGET_OBJECT_BTRACE, if_changed ? "if-changed" : NULL);
			                            /* == annex field */

Then I guess remote_btrace_has_changed and even its packet do not have to
exist at all.


> > > +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.
> 
> This function is called from command functions to get the branch trace for
> the current thread.
> 
> At some point, I do need to get the current thread. If I made ptid_t a
> parameter here, I would have to get the current ptid in all callers.

Yes, the direct access to inferior_ptid should be done only at the topmost
callers so that it can be easier changed in the future.  But it is not much
a requirement, you can keep it as is.


> > > +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.
> 
> The [begin; end] range in struct btrace_function refers to source lines.
> The [begin; end[ range here refers to indices into the instruction trace vector.

OK so could you comment it in all such cases?  Sorry I was not explicit
I request such a comment there.


> > > +  /* 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".

As any 'struct btrace_function' is alive only during the single execution of
a command there should not be any problems with symtab lifetime, my suggesions
about free_objfile discarding of stale symtab pointers where therefore
irrelevant.


> > Moreover the result is _for_display - so not for any comparisons.

In such case call the field bfun->filename_for_display.

Although TBH I would find easier to store symtab itself as one can use it for
any purpose then and calling symtab_to_filename_for_display each time during
its use is zero-cost.


> > 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.
> 
> I can compute the filename when I intend to print a line, so I don't need to store
> the symtab pointer separately.
> 
> I changed it to obtain the filename via a call to symtab_to_fullname.

In such case call the field bfun->fullname.


> I store that
> while I traverse the instruction trace vector so I can compare it to other filenames.
> Would I need to duplicate the string or is it safe to store it as-is for my purpose?

The string is alive as long as symtab is alive.  Both
symtab_to_filename_for_display and symtab_to_fullname return 'const char *' as
the string is stored in the symtab; you get only a readonly reference to it.


> > > +      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.
> 
> I changed it to compute the function name whenever I want to print it.
> 
> I'm only storing symbol and minimal symbol pointers for computing the output
> of "record function-call-history". I guess I won't need the hook in that case.

You are right; sorry I originally expected "bfun" has longer lifetime.


> > > +    }
> > > +  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.
> 
> I use it only for functions for which I do not have a full symbol. The alternative
> would be to not provide a filename in that case. Would that be preferable?

Yes, that would be preferable.


> OK. I would still use symtab_to_filename_for_display when displaying the filename, right?

Yes.


> So I would store the fullname for the above comparison and compute the filename to
> display when printing the line.

I do not see the code - but you need the symtab when printing the filename to
display.  Isn't it easier to use the same symtab also for symtab_to_fullname?
But that is a coding detail, I agree with the principle.


> What would I do if I only have a minimal symbol? Can I still use ->filename for this check?
> See above for a related question on printing filenames.

No, if you have only a minimal symbol you have no filename.  But in such case
there is also no problem with macros from other files etc. as all the
addresses will still resolve to the same minimal symbol.


> > > +  /* 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'.
> 
> Can I store the returned pointer? Would I need to duplicate the string?

It is preferred to store the symtab pointer, not the various kinds of
filenames it provides.

You do not need to duplicate the strings as long as you do not store them
longer than the single command execution (which here you do not).


> > 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.
> 
> That might affect a lot of library code for which we don't have debug info.

Yes, for libraries without debug info one knows only the function names.
Install the system libraries separate debug info if you want more.


> OK. I assume that a full symbol will always have a symtab. Is this correct?

Correct.


Thanks,
Jan

[PATCH 2/3] record-btrace, disas: omit pc prefix

[markus.t.metzger]

From: Markus Metzger <markus.t.metzger@intel.com>

Add a disassembly flag to omit the pc prefix and use it in the "record
instruction-history" command of record-btrace.

The pc prefix would appear multiple times in the branch trace disassembly,
which is more confusing than helpful.

2013-02-25 Markus Metzger  <markus.t.metzger@intel.com>

	* record-btrace.c (btrace_insn_history): Omit the pc prefix in
	the instruction history disassembly.
	* disasm.c (dump_insns): Omit the pc prefix, if requested.
	* disasm.h (DISASSEMBLY_OMIT_PC): New.


---
 gdb/disasm.c        |    4 +++-
 gdb/disasm.h        |    1 +
 gdb/record-btrace.c |    3 +++
 3 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 9d61379..e643c2d 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -122,7 +122,9 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
 	    num_displayed++;
 	}
       ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
-      ui_out_text (uiout, pc_prefix (pc));
+
+      if ((flags & DISASSEMBLY_OMIT_PC) == 0)
+	ui_out_text (uiout, pc_prefix (pc));
       ui_out_field_core_addr (uiout, "address", gdbarch, pc);
 
       if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
diff --git a/gdb/disasm.h b/gdb/disasm.h
index 20ceb2b..3743ccc 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -23,6 +23,7 @@
 #define DISASSEMBLY_RAW_INSN	(0x1 << 1)
 #define DISASSEMBLY_OMIT_FNAME	(0x1 << 2)
 #define DISASSEMBLY_FILENAME	(0x1 << 3)
+#define DISASSEMBLY_OMIT_PC	(0x1 << 4)
 
 struct ui_out;
 struct ui_file;
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index e43b687..0e0e976 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -415,6 +415,9 @@ btrace_insn_history (struct btrace_thread_info *btinfo, struct ui_out *uiout,
 
   gdbarch = target_gdbarch ();
 
+  /* We don't want to print the pc prefix in the branch trace.  */
+  flags |= DISASSEMBLY_OMIT_PC;
+
   for (idx = begin; idx < end; ++idx)
     {
       struct btrace_inst *inst;
-- 
1.7.1

Re: [PATCH 2/3] record-btrace, disas: omit pc prefix

[Jan Kratochvil]

On Mon, 25 Feb 2013 17:15:16 +0100, markus.t.metzger@intel.com wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> Add a disassembly flag to omit the pc prefix and use it in the "record
> instruction-history" command of record-btrace.
> 
> The pc prefix would appear multiple times in the branch trace disassembly,
> which is more confusing than helpful.

I do not see it, moreover I find the output better without this patch:

with the patch:

(gdb) record instruction-history 
warning: Recorded trace may be corrupted.
warning: Recorded trace may be corrupted.
warning: Recorded trace may be corrupted.
0x00007ffff62fe56a <_int_malloc+298>:	add    $0xa8,%rsp
0x00007ffff62fe571 <_int_malloc+305>:	mov    %r12,%rax
0x00007ffff62fe574 <_int_malloc+308>:	pop    %rbx
0x00007ffff62fe575 <_int_malloc+309>:	pop    %rbp
0x00007ffff62fe576 <_int_malloc+310>:	pop    %r12
0x00007ffff62fe578 <_int_malloc+312>:	pop    %r13
0x00007ffff62fe57a <_int_malloc+314>:	pop    %r14
0x00007ffff62fe57c <_int_malloc+316>:	pop    %r15
0x00007ffff62fe57e <_int_malloc+318>:	retq   
0x00007ffff6300ecc <__GI___libc_malloc+92>:	test   %rax,%rax
(gdb) p/x $pc
$1 = 0x7ffff6300ecc

without the patch:
(gdb) record instruction-history 
   0x00007ffff6303f51 <freehook+209>:	mov    %rax,0x0(%rbp)
   0x00007ffff6303f55 <freehook+213>:	pop    %rbx
   0x00007ffff6303f56 <freehook+214>:	pop    %rbp
   0x00007ffff6303f57 <freehook+215>:	pop    %r12
   0x00007ffff6303f59 <freehook+217>:	pop    %r13
   0x00007ffff6303f5b <freehook+219>:	pop    %r14
   0x00007ffff6303f5d <freehook+221>:	retq   
   0x00007ffff62c8921 <_IO_vfprintf_internal+257>:	mov    %r12,%rdi
   0x00007ffff62c8924 <_IO_vfprintf_internal+260>:	callq  0x7ffff62a03d0 <free@plt+64>
=> 0x00007ffff62a03d0 <free@plt+64>:	jmpq   *0x399c9a(%rip)        # 0x7ffff663a070
(gdb) p/x $pc
$1 = 0x7ffff62a03d0


It seems to me like the preference of "=> " or not "=> " is not related to
"record instruction-history".

If you do like disassemblies more without "=> " it can be made some new global
option unrelated to "record".


> 2013-02-25 Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* record-btrace.c (btrace_insn_history): Omit the pc prefix in
> 	the instruction history disassembly.
> 	* disasm.c (dump_insns): Omit the pc prefix, if requested.
> 	* disasm.h (DISASSEMBLY_OMIT_PC): New.


Thanks,
Jan

RE: [PATCH 2/3] record-btrace, disas: omit pc prefix

[Metzger, Markus T]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Wednesday, February 27, 2013 8:59 AM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org; markus.t.metzger@gmail.com
> Subject: Re: [PATCH 2/3] record-btrace, disas: omit pc prefix
> 
> On Mon, 25 Feb 2013 17:15:16 +0100, markus.t.metzger@intel.com wrote:
> > From: Markus Metzger <markus.t.metzger@intel.com>
> >
> > Add a disassembly flag to omit the pc prefix and use it in the "record
> > instruction-history" command of record-btrace.
> >
> > The pc prefix would appear multiple times in the branch trace disassembly,
> > which is more confusing than helpful.
> 
> I do not see it, moreover I find the output better without this patch:

Within a loop, you will see repeating => very frequently.

(gdb) rec inst -
   0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
=> 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
   0x00000000004006f7 <_ZL4loopPv+19>:	mov    %rax,0x20094a(%rip)        # 0x601048 <_ZL6shared>
   0x00000000004006fe <_ZL4loopPv+26>:	jmp    0x4006ec <_ZL4loopPv+8>
   0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
=> 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
   0x00000000004006f7 <_ZL4loopPv+19>:	mov    %rax,0x20094a(%rip)        # 0x601048 <_ZL6shared>
   0x00000000004006fe <_ZL4loopPv+26>:	jmp    0x4006ec <_ZL4loopPv+8>
   0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
=> 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
(gdb)

[...]

> (gdb) record instruction-history
> warning: Recorded trace may be corrupted.
> warning: Recorded trace may be corrupted.
> warning: Recorded trace may be corrupted.

That's unexpected. If you can reproduce this, would you please send
me the reproducer so I can have a look at it?

[...]

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 2/3] record-btrace, disas: omit pc prefix

[Jan Kratochvil]

On Thu, 28 Feb 2013 08:03:04 +0100, Metzger, Markus T wrote:
> Within a loop, you will see repeating => very frequently.
> 
> (gdb) rec inst -
>    0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
> => 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
>    0x00000000004006f7 <_ZL4loopPv+19>:	mov    %rax,0x20094a(%rip)        # 0x601048 <_ZL6shared>
>    0x00000000004006fe <_ZL4loopPv+26>:	jmp    0x4006ec <_ZL4loopPv+8>
>    0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
> => 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
>    0x00000000004006f7 <_ZL4loopPv+19>:	mov    %rax,0x20094a(%rip)        # 0x601048 <_ZL6shared>
>    0x00000000004006fe <_ZL4loopPv+26>:	jmp    0x4006ec <_ZL4loopPv+8>
>    0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
> => 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
> (gdb)

I did not think about this case but I still find it correct, => should display
the current PC and it is the current PC.


> > (gdb) record instruction-history
> > warning: Recorded trace may be corrupted.
> > warning: Recorded trace may be corrupted.
> > warning: Recorded trace may be corrupted.
> 
> That's unexpected. If you can reproduce this, would you please send
> me the reproducer so I can have a look at it?

I have patched intel_supports_btrace on my model 0x1a, couldn't it be the
reason?


Jan

RE: [PATCH 2/3] record-btrace, disas: omit pc prefix

[Metzger, Markus T]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Thursday, February 28, 2013 8:17 AM


> On Thu, 28 Feb 2013 08:03:04 +0100, Metzger, Markus T wrote:
> > Within a loop, you will see repeating => very frequently.
> >
> > (gdb) rec inst -
> >    0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
> > => 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
> >    0x00000000004006f7 <_ZL4loopPv+19>:	mov    %rax,0x20094a(%rip)        # 0x601048 <_ZL6shared>
> >    0x00000000004006fe <_ZL4loopPv+26>:	jmp    0x4006ec <_ZL4loopPv+8>
> >    0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
> > => 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
> >    0x00000000004006f7 <_ZL4loopPv+19>:	mov    %rax,0x20094a(%rip)        # 0x601048 <_ZL6shared>
> >    0x00000000004006fe <_ZL4loopPv+26>:	jmp    0x4006ec <_ZL4loopPv+8>
> >    0x00000000004006ec <_ZL4loopPv+8>:	mov    0x200955(%rip),%rax        # 0x601048 <_ZL6shared>
> > => 0x00000000004006f3 <_ZL4loopPv+15>:	add    $0x1,%rax
> > (gdb)
> 
> I did not think about this case but I still find it correct, => should display
> the current PC and it is the current PC.

I did not say that it is incorrect. I said I find it confusing if => appears several
times within the trace.

OK to make it optional, say, with a /p modifier?


> > > (gdb) record instruction-history
> > > warning: Recorded trace may be corrupted.
> > > warning: Recorded trace may be corrupted.
> > > warning: Recorded trace may be corrupted.
> >
> > That's unexpected. If you can reproduce this, would you please send
> > me the reproducer so I can have a look at it?
> 
> I have patched intel_supports_btrace on my model 0x1a, couldn't it be the
> reason?

Yes, that might explain it.

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [PATCH 2/3] record-btrace, disas: omit pc prefix

[Jan Kratochvil]

On Thu, 28 Feb 2013 08:45:44 +0100, Metzger, Markus T wrote:
> OK to make it optional, say, with a /p modifier?

OK.


Jan