Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Jan Kratochvil <jan.kratochvil@redhat.com>
To: "Metzger, Markus T" <markus.t.metzger@intel.com>
Cc: "palves@redhat.com" <palves@redhat.com>,
	       "tromey@redhat.com" <tromey@redhat.com>,
	       "kettenis@gnu.org" <kettenis@gnu.org>,
	       "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
	       "markus.t.metzger@gmail.com" <markus.t.metzger@gmail.com>
Subject: Re: [patch v6 00/12] branch tracing support for Atom
Date: Tue, 18 Dec 2012 13:55:00 -0000	[thread overview]
Message-ID: <20121218135437.GA16636@host2.jankratochvil.net> (raw)
In-Reply-To: <A78C989F6D9628469189715575E55B2307B421D6@IRSMSX102.ger.corp.intel.com>

On Tue, 18 Dec 2012 11:12:49 +0100, Metzger, Markus T wrote:
> I like the idea of adding BTS as a configuration option to "record" to
> provide a control-flow only trace for the reverse- commands. This will be
> quite limited since the only thing you can really do is iterate over
> disassembly and source lines

Yes.  But without teaching users new commands, one of the issues of GDB.
I find myself to prefer the already used reverse-stepi even with BTS.


> - plus some fake back trace.

I would exclude that feature from the current discussion as it can be
implemented as an add-on patch without changing much other code.


> We should not remove the existing btrace commands, though.

In general I agree.  Just they should be moved under the "record" prefix.
Command "btrace" then should be renamed to some "record disassemble".

And for example the "btrace" current position should be shared with
"reverse-*" current position and the btrace args parser/syntax should be
unified with "record goto" syntax.  That means only supporting start/begin/end
as otherwise btrace syntax is more rich than the record one - but the goal is
to make the syntax compatible for users.


> Using the remote- commands, you would need a sequence of reverse-stepi or
> reverse-step commands. And even then, the output would be clobbered with
> intermixed prompts and would be in reverse control-flow order, unless you
> navigated back and forth again.

I agree.  Just "reverse-step" is a more general command - even for record
- while still usable for the basic use case of BTS.


> Depending on your use case, either the btrace or the remote- commands are
> more effective.

Yes.


> As far as I understand, the btrace commands could also be implemented for
> "record" to provide a more compact view of the recorded control-flow. They
> could thus also help improve the "record" feature.

Exactly.  That both record navigation commands are useful (or at least usable)
for BTS and that BTS view commands are definitely useful for record proves to
me they should share the same backend/data.


> Given the different use cases, it would make sense to establish two sets of
> commands for all kinds of control-flow tracing/replay: one for viewing and
> one for navigating. In a first step, BTS tracing will only implement the
> former and "record" will only implement the latter. In a second step,
> "record" could implement the viewing commands, and in  third step, BTS
> tracing can implement the navigation commands.
> 
> Would that be OK with you?

I believe you a heading for an approval of this patchset as is, so that the
unification can be implemented in an add-on patch series.

I do not think the unification patch series will get checked-in soon enough
for gdb-7.6.  That means the currently defined commands will need to remain
backward comparible indefinitely in the future.  So one should make the
commands syntax final already (there is deprecate_cmd but it is not good to
plan its use).

I believe the internal API will change a lot for the unification with record
backend/data so it seems to me a large part of the patch will need to get
replaced afterwards.

But I am OK with that if we at least make the commands unified with record
with some hope the backend functionality unification will come later.


Thanks,
Jan


  reply	other threads:[~2012-12-18 13:55 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-17 16:02 markus.t.metzger
2012-12-17 16:02 ` [patch v6 06/12] remote, btrace: add branch trace remote ops markus.t.metzger
2012-12-17 19:57   ` Jan Kratochvil
2012-12-17 16:02 ` [patch v6 02/12] cli, btrace: add btrace cli markus.t.metzger
2012-12-17 18:32   ` Jan Kratochvil
2012-12-18  7:36     ` Metzger, Markus T
2012-12-18  8:35       ` Jan Kratochvil
2012-12-18  9:04   ` Jan Kratochvil
2012-12-18  9:11     ` Metzger, Markus T
2012-12-17 16:02 ` [patch v6 09/12] gdbserver, linux, btrace: add btrace support for linux-low markus.t.metzger
2012-12-17 16:02 ` [patch v6 08/12] gdbserver, btrace: add generic btrace support markus.t.metzger
2012-12-17 20:43   ` Jan Kratochvil
2012-12-17 16:02 ` [patch v6 01/12] thread, btrace: add generic branch trace support markus.t.metzger
2012-12-17 16:02 ` [patch v6 12/12] btrace, x86: disable on some processors markus.t.metzger
2012-12-17 17:11   ` Mark Kettenis
2012-12-19 16:13     ` Metzger, Markus T
2012-12-19 16:36       ` Mark Kettenis
2012-12-21 10:38       ` Jan Kratochvil
2012-12-17 17:37   ` H.J. Lu
2012-12-19 15:58     ` Metzger, Markus T
2012-12-17 20:35   ` Jan Kratochvil
2012-12-17 16:03 ` [patch v6 05/12] xml, btrace: define btrace xml document style markus.t.metzger
2012-12-17 19:53   ` Jan Kratochvil
2012-12-18  7:43     ` Metzger, Markus T
2012-12-17 16:03 ` [patch v6 10/12] test, btrace: add branch tracing tests markus.t.metzger
2012-12-17 20:26   ` Jan Kratochvil
2012-12-17 16:03 ` [patch v6 04/12] linux, i386, amd64: enable btrace for 32bit and 64bit linux native markus.t.metzger
2012-12-17 16:03 ` [patch v6 11/12] test, btrace: more branch tracing tests markus.t.metzger
2012-12-17 16:03 ` [patch v6 03/12] linux, btrace: perf_event based branch tracing markus.t.metzger
2012-12-17 16:03 ` [patch v6 07/12] btrace, doc: document remote serial protocol markus.t.metzger
2012-12-17 18:45 ` [patch v6 00/12] branch tracing support for Atom Jan Kratochvil
2012-12-17 19:34   ` Tom Tromey
2012-12-18  7:24     ` Metzger, Markus T
2012-12-18  9:20 ` Jan Kratochvil
2012-12-18 10:14   ` Metzger, Markus T
2012-12-18 13:55     ` Jan Kratochvil [this message]
2012-12-19  9:59       ` Metzger, Markus T
2012-12-19 12:13         ` Mark Kettenis
2012-12-19 12:37           ` Jan Kratochvil
2012-12-20  7:17         ` Jan Kratochvil
2012-12-20  9:14           ` Metzger, Markus T
2012-12-20 11:43             ` Jan Kratochvil
2012-12-20 15:20               ` Metzger, Markus T
2012-12-21 19:12                 ` Jan Kratochvil
2012-12-22 13:08         ` Jan Kratochvil
2013-01-01 16:35           ` Jan Kratochvil

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121218135437.GA16636@host2.jankratochvil.net \
    --to=jan.kratochvil@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=kettenis@gnu.org \
    --cc=markus.t.metzger@gmail.com \
    --cc=markus.t.metzger@intel.com \
    --cc=palves@redhat.com \
    --cc=tromey@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox