Proposal: scheduler-locking during inferior calls

Proposal: scheduler-locking during inferior calls

[Saiapova, Natalia]

Dear All,

In multithreaded program, when several threads are stopped at a breakpoint and
scheduler-locking mode is not 'on', there might be unexpected thread switches
during an inferior call evaluation. If such thread switching occurs,
the evaluation of the inferior call is abandoned:

~~~
(gdb) run
Thread 1 "a.out" hit Breakpoint 2, main._omp_fn.0(void) () at multi-omp.cpp:18
18                                         std::cout << omp_get_thread_num();
(gdb) call do_something()
[Switching to Thread 0x7ffff6327700 (LWP 32498)]

Thread 3 "a.out" hit Breakpoint 2, main._omp_fn.0(void) () at multi-omp.cpp:18
18                                         std::cout << omp_get_thread_num();
The program stopped in another thread while making a function call from GDB.
Evaluation of the expression containing the function
(do_something()) will be abandoned.
When the function is done executing, GDB will silently stop.
~~~

The similar problem happens with stepping commands, however, for these we have
'step' scheduler-locking mode. We could add a new scheduler locking mode that
will handle inferior calls. But it is highly likely, that this mode would
be used together with 'step'. If we blend step locking into the new mode,
one might also want to include the replay, and then the number of possible
combinations explodes. It does not seem to be flexible and scalable solution.
And if the new mode locks only inferior calls, it will not be user-friendly,
since then a user will be forced to switch modes during the debugging.

I would like to propose a change of existing scheduler-locking mechanism, which
will give a user fine-granular control over the scheduler lock. The user will
define his or her own mode based on options that he or she could set
independently:

~~~
(gdb) set scheduler-locking step on
(gdb) set scheduler-locking eval on
(gdb) set scheduler-locking replay on
(gdb) set scheduler-locking run off
(gdb) show scheduler-locking
Scheduler-locking mode:
step on
eval on
replay on
run off
~~~

Here 'eval' represents inferior calls, and 'run' represents 'continue' and
'finish' commands.

However, these will not be backward compatible with the current 'set/show
scheduler-locking' commands. As I am not sure, that pros of this change will
outweigh the backward incompatibility, I have added a command
'set scheduler-locking-eval' as a work-around. It toggles scheduler locking
for inferior calls and coexists with current scheduler-locking modes
(so it could be used together with or without step or replay modes).

I would appreciate other opinions about this problem and its possible solutions.

Here is an example diff for the set scheduler-locking-eval command.

~~~
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c2ce532..84fa225 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2214,6 +2214,12 @@ static const char *const scheduler_enums[] = {
   schedlock_replay,
   NULL
};
+
+static struct {
+  /* If true, scheduler locking is on during inferior calls.  */
+  int eval = 1;
+} scheduler_locking;
+
static const char *scheduler_mode = schedlock_replay;
static void
show_scheduler_mode (struct ui_file *file, int from_tty,
@@ -2264,6 +2270,7 @@ ptid_t
user_visible_resume_ptid (int step)
{
   ptid_t resume_ptid;
+  thread_info * tp = find_thread_ptid (current_inferior (), inferior_ptid);
   if (non_stop)
     {
@@ -2272,7 +2279,8 @@ user_visible_resume_ptid (int step)
       resume_ptid = inferior_ptid;
     }
   else if ((scheduler_mode == schedlock_on)
-          || (scheduler_mode == schedlock_step && step))
+          || (scheduler_mode == schedlock_step && step)
+          || (scheduler_locking.eval && tp->control.in_infcall))
     {
       /* User-settable 'scheduler' mode requires solo thread
         resume.  */
@@ -3021,8 +3029,7 @@ thread_still_needs_step_over (struct thread_info *tp)
   return what;
}
-/* Returns true if scheduler locking applies.  STEP indicates whether
-   we're about to do a step/next-like command to a thread.  */
+/* Returns true if scheduler locking applies to thread TP.  */
 static int
schedlock_applies (struct thread_info *tp)
@@ -3032,7 +3039,8 @@ schedlock_applies (struct thread_info *tp)
              && tp->control.stepping_command)
          || (scheduler_mode == schedlock_replay
              && target_record_will_replay (minus_one_ptid,
-                                           execution_direction)));
+                                           execution_direction))
+          || (scheduler_locking.eval && tp->control.in_infcall));
}
 extern void switch_to_inferior_no_thread (inferior *inf);
@@ -9487,6 +9495,13 @@ infrun_async_inferior_event_handler (gdb_client_data data)
   inferior_event_handler (INF_REG_EVENT, NULL);
}
+static void
+show_schedlock_eval (struct ui_file *file, int from_tty,
+                     struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("Scheduler locking during function evaluations is %s.\n"), value);
+}
+
void
_initialize_infrun (void)
{
@@ -9703,6 +9718,16 @@ replay == scheduler locked in replay mode and unlocked during normal execution."
                        show_scheduler_mode,
                        &setlist, &showlist);
+  add_setshow_boolean_cmd ("scheduler-locking-eval", class_run,
+                         &scheduler_locking.eval, _("\
+Set mode for locking scheduler during function evaluation."), _("\
+Show mode for locking scheduler during function evaluation."), _("\
+off    == no locking (threads may preempt during inferior calls)\n\
+on     == no thread except the current thread may run during inferior calls."),
+                         NULL,
+                         show_schedlock_eval,
+                         &setlist, &showlist);
+
   add_setshow_boolean_cmd ("schedule-multiple", class_run, &sched_multi, _("\
Set mode for resuming threads of all processes."), _("\
Show mode for resuming threads of all processes."), _("\
~~~

--Natalia
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: Proposal: scheduler-locking during inferior calls

[Saiapova, Natalia]

> -----Original Message-----
> 
> Dear All,
> 
> In multithreaded program, when several threads are stopped at a breakpoint and
> scheduler-locking mode is not 'on', there might be unexpected thread switches
> during an inferior call evaluation. If such thread switching occurs,
> the evaluation of the inferior call is abandoned:
> 
> ~~~
> (gdb) run
> Thread 1 "a.out" hit Breakpoint 2, main._omp_fn.0(void) () at multi-omp.cpp:18
> 18                                         std::cout << omp_get_thread_num();
> (gdb) call do_something()
> [Switching to Thread 0x7ffff6327700 (LWP 32498)]
> 
> Thread 3 "a.out" hit Breakpoint 2, main._omp_fn.0(void) () at multi-omp.cpp:18
> 18                                         std::cout << omp_get_thread_num();
> The program stopped in another thread while making a function call from GDB.
> Evaluation of the expression containing the function
> (do_something()) will be abandoned.
> When the function is done executing, GDB will silently stop.
> ~~~
> 
> The similar problem happens with stepping commands, however, for these we have
> 'step' scheduler-locking mode. We could add a new scheduler locking mode that
> will handle inferior calls. But it is highly likely, that this mode would
> be used together with 'step'. If we blend step locking into the new mode,
> one might also want to include the replay, and then the number of possible
> combinations explodes. It does not seem to be flexible and scalable solution.
> And if the new mode locks only inferior calls, it will not be user-friendly,
> since then a user will be forced to switch modes during the debugging.
> 
> I would like to propose a change of existing scheduler-locking mechanism, which
> will give a user fine-granular control over the scheduler lock. The user will
> define his or her own mode based on options that he or she could set
> independently:
> 
> ~~~
> (gdb) set scheduler-locking step on
> (gdb) set scheduler-locking eval on
> (gdb) set scheduler-locking replay on
> (gdb) set scheduler-locking run off
> (gdb) show scheduler-locking
> Scheduler-locking mode:
> step on
> eval on
> replay on
> run off
> ~~~
> 
> Here 'eval' represents inferior calls, and 'run' represents 'continue' and
> 'finish' commands.
> 
> However, these will not be backward compatible with the current 'set/show
> scheduler-locking' commands. As I am not sure, that pros of this change will
> outweigh the backward incompatibility, I have added a command
> 'set scheduler-locking-eval' as a work-around. It toggles scheduler locking
> for inferior calls and coexists with current scheduler-locking modes
> (so it could be used together with or without step or replay modes).
> 
> I would appreciate other opinions about this problem and its possible solutions.

Currently, if we want to execute an inferior call in multithreaded program, 
we have to switch scheduler-locking to 'on' before the call, and then, switch it
back to 'step' or 'replay' after the call.

I could think about three options how we could improve scheduler locking for this case:
add a new mode, add a new set/show command (example from my first email), or
modify the existing locking mechanism (the proposed solution).

The easiest solution would be a creation of a new mode (that was actually 
my first attempt).  The mode was called 'step+' and covered all three options:
'step', 'replay', and 'inferior calls'.  But I believe, that was not 
very user-friendly.  This mode did not fit well to the current list: it not 
only added a new locking option, but also included already existing ones.

The second option is only a temporary workaround for the problem (I attached the example
to my first email).

The third option is not backward compatible, however, I believe, it would make
the scheduler locking UI more transparent to a user.  As far as I could see, 
current scheduler-locking modes do not contradict with each other, so if the 
interface allowed it, they could be mixed.  From the code perspective, 
the proposed change should not be big and will involve only infrun.c:

1. Instead of storing the current mode in 'scheduler_mode' variable, we will store 
a structure of all options
~~~
static struct { 
  int replay = 1;
  int step = 0;
  int eval = 0;
  int run = 0;
} scheduler_locking;
~~~
2. Modify 'schedlock_applies' function;
3. Modify user_visible_resume_ptid (possibly, also use 'schedlock_applies' there;
4. Modify 'clear_proceed_status' to use 'scheduler_locking.replay'.

If the proposed change sounds sensible I could create a patch.

--Natalia
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928