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: Monday, December 16, 2013 9:30 PM
> To: Metzger, Markus T


> > 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.
> 
> I guess this goes in a similar direction of what I was asking
> in the other thread.  When we enable replaying, is still sitting
> at the same location the live inferior is, it seems like if
> you allow reading registers/memory from the live inferior,
> then you can also use the regular dwarf unwinder for that frame,
> and then you wouldn't need to frob step_frame_id, etc.

I can't.  Because after the first reverse-single-step, I will be somewhere
in the execution history and suddenly, the frame_id of my caller does
no longer compare equal to the frame_id we started stepping - assuming
I stepped into a subroutine.

I'm using the fact that I'm actually at the same position as the live
target, yet I'm already replaying, so I get the btrace frame_id's I need
later on.


> > 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.
> 
> Ah, because target_set called set_executing (..., 1), right?
> I forgot record full does something like that too.

I don't know who does it but it is set when I am called.


> >>> +  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.
> 
> Hmm.  I see.  Please consider scheduler-locking though.  That is,
> 
> (gdb) set scheduler-locking on
> (gdb) thread 1
> (gdb) reverse-steop
> (gdb) thread 2
> (gdb) record goto 42
> (gdb) thread 3
> (gdb) continue
> 
> The last continue doesn't apply to threads 1 and 2, so not
> clear what it should do to them then.  My knee jerk would
> be to leave them alone.

Does scheduler locking set inferior_ptid?
If it does, we should already get the desired behaviour.
If it doesn't, how would I know which thread to step?


> >>> +  /* 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.
> 
> Yes, but -1 or "ptid of the process" means "step inferior_ptid
> and let others run free".

See below.


> > 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.
> 
> Some of the other threads can hit a breakpoint before the
> current thread hits one too.

That depends on thread interleaving.  There is no guarantee that
the other threads make progress.  The way it is currently
implemented, we step the chosen thread until the next event.
This event is either a breakpoint, step completed, or out of
execution history.

We're effectively only stepping a single thread.  I believe this
is also how the s/w record target works.

The alternative would be to lock-step all threads, i.e. single-step
each thread until the first thread produces an event.  We could
switch between those two based on scheduler-locking.


> > 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.
> 
> It's better to not rely on inferior_ptid in to_wait at all.
> E.g., in non-stop/async (I now you don't support it now, but
> still), there's no connection between inferior_ptid and
> the thread that was stepped when you get to target_wait.
> It really can't --- in principle, how could core GDB
> know which thread would get the event _before_ the target
> reported it to the core exactly with target_wait ?

OK.  So in to_wait I only step marked threads.  We can still
extend it to lock-stepping later on by marking more than one
thread.

But this means that I would rely on inferior_ptid in to_resume
if -1 or the process id is passed.


> >>> +	  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.
> 
> Eh, it's writable even when forward executing through history?
> 
> I see this in patch 14:
> 
> > +static void
> > +record_btrace_store_registers (struct target_ops *ops,
> > +			       struct regcache *regcache, int regno)
> > +{
> > +  struct target_ops *t;
> > +
> > +  if (record_btrace_is_replaying ())
> > +    error (_("This record target does not allow writing registers."));
> > +
> 
> Was it changed in later patches?

Oops.  You are right.  It is read-only.

After some playing around, I also managed to trigger the error
when adjust_pc_after_break tries to write the adjusted PC.

I'm inclined to simply allow it.  The adjusted PC will be ignored for
stepping, of course.  We just follow the recorded execution.

Concerns?


> I meant things like:
> 
> # navigate in the trace history for both threads
> with_test_prefix "nativ-hist-both-thrds" {
>   gdb_test "thread 1" ".*"
>   with_test_prefix "thrd1" {
>     gdb_test "record goto begin" ".*"
>     gdb_test "info record" ".*Replay in progress\.  At instruction 1\."
>   }
>   gdb_test "thread 2" ".*"
>   with_test_prefix "thrd2" {
>     gdb_test "record goto begin" ".*"
>     gdb_test "info record" ".*Replay in progress\.  At instruction 1\."
>   }
> }
> 
> This way you don't need numbers at all, and gdb.sum shows
> something meaningful (if not specified, the gdb.sum out defaults
> to the gdb command passed to gdb_test).  Some of the old gdb.trace/
> tests also used numbers like this, and over time, was we
> add/remove bits from the tests, we end up with outdated numbers,
> holes in the numeric sequence, etc.  Sometimes you need to update
> the numbers of the following tests to make room for one other tests.
> Etc.

I already spent some time renumbering...

OK, I'll use test prefixes for the different test groups.

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