RE: [PATCH 4/4] gdb, infrun: fix multi-threaded reverse stepping

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Thanks for the review, Pedro.

>> I would have expected everything to be completed by the time the infcmd
>> function returns but I cannot say whether the behaviour I'm seeing is
>> intentional.
>
>The going via the event loop to handle the pending event is intentional,
>simplifies
>things by only having one code path.

Thanks for clarifying.  I rephrased the commit message.


>> Assuming it is, this patch addresses the loss of the execution direction
>> by storing the direction in a thread's control state and changing most of
>> infrun to take it from there rather than using the global variable.
>
>Do you envision that you'll support having some threads moving forward and
>other threads moving backwards, all in the same inferior or target?
>
>Like:
>
> (gdb) thread 1.1
> (gdb) c&               // thread 1.1 is now continuously recording
> (gdb) thread 1.2
> (gdb) reverse-continue // thread 1.2 is running backwards

The current implementation does not allow that but the interface would.


>Which makes me question -- seems odd to have both the direction recorded in
>all the threads, and then still have target_execution_direction() ?
>
>Do we want to eliminate one of these?  Either the target direction, or the threads
>direction?
>
>If we don't want to support threads of the same target executing in different
>directions,
>then I suspect that we can reimplement this patch by adding a new
>target_set_execution_direction
>target method, that would be called in the case where infrun skips calling
>target_resume
>because of a pending event.

We already allow one thread to move forward and another to move backward
in non-stop mode.  What we do not allow is one thread to continue recording
while another is replaying.

This works since commands are issued sequentially and the execution direction
is toggled between commands.  When a thread is actually resumed, record-btrace
stores the execution direction as part of the stepping command:

enum btrace_thread_flag : unsigned
{
  /* The thread is to be stepped forwards.  */
  BTHR_STEP = (1 << 0),

  /* The thread is to be stepped backwards.  */
  BTHR_RSTEP = (1 << 1),

  /* The thread is to be continued forwards.  */
  BTHR_CONT = (1 << 2),

  /* The thread is to be continued backwards.  */
  BTHR_RCONT = (1 << 3),

  /* The thread is to be moved.  */
  BTHR_MOVE = (BTHR_STEP | BTHR_RSTEP | BTHR_CONT | BTHR_RCONT),

  /* The thread is to be stopped.  */
  BTHR_STOP = (1 << 4)
};

In the scenario addressed by this patch, there were separate resume requests
for the same thread to record-btrace between which the direction got lost.


>If we do want to support mixed directions, then is the following snipped from
>fetch_inferior_event still useful for anything?
>
>    scoped_restore save_exec_dir
>      = make_scoped_restore (&execution_direction,
>			     target_execution_direction ());
>
>AFAICS, when handling an event we'll now use the thread's execution direction,
>so flipping
>the global like this here is basically dead code.

Hmmm, it probably is indeed redundant.


>> @@ -7404,6 +7416,11 @@ keep_going_stepped_thread (struct thread_info *tp)
>>
>>        tp->resumed = true;
>>        resume_ptid = internal_resume_ptid (tp->control.stepping_command);
>> +
>> +      scoped_restore save_exec_dir
>> +	= make_scoped_restore (&execution_direction,
>> +			       tp->control.execution_direction);
>
>Shouldn't this be done for all do_target_resume calls?  How about doing it from
>within do_target_resume instead?

Sounds good.


>> +    gdb_test_multiple "thread apply $thread info record" \
>> +	"thread $thread not replaying" {
>> +        -re "Replay in progress" {
>
>This inadvertently leaves the prompt in the expect buffer.  You can use -wrap to
>match it.

Thanks.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928