This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [patch v8 24/24] record-btrace: add (reverse-)stepping support
- From: "Metzger, Markus T" <markus dot t dot metzger at intel dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "jan dot kratochvil at redhat dot com" <jan dot kratochvil at redhat dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, Eli Zaretskii <eliz at gnu dot org>
- Date: Mon, 16 Dec 2013 15:54:30 +0000
- Subject: RE: [patch v8 24/24] record-btrace: add (reverse-)stepping support
- Authentication-results: sourceware.org; auth=none
- References: <1386839747-8860-1-git-send-email-markus dot t dot metzger at intel dot com> <1386839747-8860-25-git-send-email-markus dot t dot metzger at intel dot com> <52AB5E6A dot 1010105 at redhat dot com>
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Friday, December 13, 2013 8:22 PM
Thanks for your review.
> > + /* Clear the executing flag to allow changes to the current frame. */
> > + executing = is_executing (tp->ptid);
> > + set_executing (tp->ptid, 0);
>
> Why is this necessary? Is this so you can start replaying
> even when the current thread is really executing?
No. When we just start replaying with a reverse stepping command,
we need to recompute stack frames so we can compare them to
detect subroutine calls in infrun.c.
See also https://sourceware.org/ml/gdb-patches/2013-11/msg00874.html
and https://sourceware.org/ml/gdb-patches/2013-11/msg00891.html.
This function is called from to_wait to enable replaying as part of
reverse stepping. We need to temporarily set is_executing to false
in order to be able to call get_current_frame below.
I extended the existing comments here and below to explain this in more detail.
> > + /* Restore the previous execution state. */
> > + set_executing (tp->ptid, executing);
> > +
> > + if (except.reason < 0)
> > + throw_exception (except);
>
> If something throws, things are being left
> possibly in an inconsistent state, it seems to me.
> Also, "replay" leaks.
Replay is stored in BTINFO. We checked that we have trace before
the try block, so btrace_insn_end () won't throw and we're not
leaking REPLAY - we just transitioned from not replaying to replaying.
It may still be better to revert the changes and not start replaying
in case of errors. Changed that. I'm also calling registers_changed_ptid
and get_current_frame again to avoid leaving things behind that are
based on replaying. This might trigger another throw.
> > + if (tp != NULL && !btrace_is_replaying (tp)
> > + && execution_direction != EXEC_REVERSE)
> > + ALL_THREADS (other)
> > + record_btrace_stop_replaying (other);
>
> Why's that?
Imagine the user does:
(gdb) thread 1
(gdb) reverse-steop
(gdb) thread 2
(gdb) record goto 42
(gdb) thread 3
(gdb) continue
We could either error out and make him go to thread 1 and thread 2
again to stop replaying those threads. Or we could more or less
silently stop replaying those other threads before we continue.
I chose to silently stop replaying; no warnings and no questions.
I find both somewhat annoying after some time.
> > + /* Find the thread to move. */
> > + if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid))
> > + {
> > + ALL_THREADS (tp)
> > + record_btrace_resume_thread (tp, flag);
>
> Seems like this steps all threads, when gdb only wants to
> step inferior_ptid and continue others?
Only if gdb passes -1 or the ptid of the process.
In the end, we will move exactly one thread and keep all
others where they are. This one thread will hit a breakpoint
or run out of execution history.
Are you suggesting that I should only mark inferior_ptid
in this case? If I marked the others as continue, I would later
on need to prefer a thread that only wanted to step.
I'm preferring inferior_ptid in to_wait later on when the
threads are actually moved. The effect should be the same,
but it might be more clear if I also did not mark the other
threads.
> > + if (breakpoint_here_p (aspace, insn->pc))
> > + return btrace_step_stopped ();
>
> How is adjust_pc_after_break handled?
I'm not doing anything special. The PC register is writable,
so the PC can be adjusted. There is already code to omit
the adjustment when reverse-executing.
> > +# navigate in the trace history for both threads
> > +gdb_test "thread 1" ".*" "mts, 1.1"
> > +gdb_test "record goto begin" ".*" "mts, 1.2"
> > +gdb_test "info record" ".*Replay in progress\. At instruction 1\." "mts,
> 1.3"
> > +gdb_test "thread 2" ".*" "mts, 1.4"
> > +gdb_test "record goto begin" ".*" "mts, 1.5"
> > +gdb_test "info record" ".*Replay in progress\. At instruction 1\." "mts,
> 1.6"
>
> What does this "mts" that appears everywhere mean?
> (with_test_prefix could help here to create more meaningful test names.)
It's a simple tag so I find the failing test quickly.
With_test_prefix wouldn't really help. I could use "mts, " as test prefix, but
this would leave the raw numbers "1.1", "1.2", ... in the tests.
I don't really need the prefix since the failing .exp file is already included
in the fail message. If it is confusing, I might as well omit it and use the numbers
only.
> > diff --git a/gdb/testsuite/gdb.btrace/rn-dl-bind.exp
> b/gdb/testsuite/gdb.btrace/rn-dl-bind.exp
> > new file mode 100644
> > index 0000000..4d803f9
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.btrace/rn-dl-bind.exp
>
> What's this testing? I can't infer it from the test name.
> Please add a comment.
It's testing a reverse-next over the dynamic linker's symbol lookup
code. I added a comment to the top of the test.
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