RE: [patch v8 24/24] record-btrace: add (reverse-)stepping support

Metzger, Markus T
2013-12-16T15:56:07Z

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