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

["Metzger, Markus T" <markus.t.metzger@intel.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