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

[Pedro Alves <palves@redhat.com>]

On 12/16/2013 03:54 PM, Metzger, Markus T wrote:
>> -----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.

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.

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

Sounds good, thanks.

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

> 
> 
>>> +  /* 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".

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

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

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

>>> +# 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.

Right, I was just wondering what is stood
for --  "Markus T ...", I guess?

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

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.

-- 
Pedro Alves