Re: [RFC] Allowing all threads of all|current process(es) to be resumed [new command + docs]

[Pedro Alves <pedro at codesourcery dot com>]

On Saturday 30 May 2009 14:10:23, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Sat, 30 May 2009 11:51:52 +0100
> > Cc: Marc Khouzam <marc.khouzam@ericsson.com>
> > 
> > Currently, with the generic framework, if GDB is attached to
> > multiple processes, issuing a "continue", "next", etc., makes GDB
> > resume all threads of all processes.  But, with the multi-forks
> > framework, GDB only debugs one of the forks at a given time, while
> > leaving the others stopped.  E.g.,
> > 
> >  (gdb) set detach-on-fork off
> >  (gdb) catch fork
> >  Catchpoint 1 (fork)
> >  (gdb) r
> >  Starting program: /home/pedro/gdb/sspaces/build/gdb/testsuite/gdb.base/foll-fork
> > 
> >  Catchpoint 1 (forked process 23902), 0x00007ffff789ac4b in fork () from /lib/libc.so.6
> >  (gdb)     
> > 
> > A 'continue' command here, will only resume the parent process, and leave
> > the child process stopped.
> > This means that moving multi-forks into multi-inferior framework changes
> > its behaviour.  If we make execution commands resume only
> > threads of the current inferior, then we're changing the current
> > default for targets making use of the current
> > multi-inferior framework...  So, we've got a conflict, and something's got
> > to give.  :-/  Both variants are reasonable and which one is right depends on
> > what you're debugging.
> 
> Can you give at least 2 different use-cases, each one favoring one of
> the possible behaviors?  I'm not sure I understand the nature of the
> conflict.

As I've explained above.  When debugging with a multi-process aware target,
and you're debugging multiple inferiors, "continue" resumes all threads
of all processes.  When debugging multi-forks on linux native, and
you're attached to multiple forks, "continue" resumes all threads
of *the current process* only.  The conflict is that both "multi-process"
implementations behave differently, and I'm getting rid of the multi-forks
support as-is.  But, I'd like to keep the option of having its behaviour.
So, I'm adding the command so that the user can select what behaviour is
wanted, either resume all processes, or current process only.  Use case
is "user wants to resume all processes" vs "user wants to resume only
the current process".

The multi-forks.exp test makes use of the "resume threads of the
current process only" variant.  It breaks badly if I allow all
threads of all processes to be resumed.  I've chosen the conservative
default of behaving like the multi-forks.exp test expects.

> A few comments about the patch for the manual:
> 
> > +@cindex resume multiple processes
> 
> This index entry is too general.  I suggest something more specific to
> the aspects you are describing.

Putting myself in a user's perspective, if I wanted to know how to
resume multiple process simultaneously, I'd go look for resume, multiple,
maybe I should add "simultaneously".  Did you have something
completely different in mind?

> > +By default, @value{GDBN} allows only threads of the current inferior
> > +to run in response to execution commands such as @code{continue}.
> 
> We don't introduce the notion of "execution commands" until the node
> that's after this one.  So if you want to use it here, I would suggest
> to add the definition of this term in the parent node ("Thread
> Stops"), and while at that, give a more-or-less comprehensive list of
> the commands that are considered to be "execution commands".

I do mention "such as @code{continue}".  I'm now including a couple more examples.
I can't see where "execution commands" are defined anywhere in other contexts
either, so this hasn't been a problem so far.  I think that such a change
should be done independently of this one, perhaps then adding cross references
everywhere appropriate.

gdb.texinfo:to run in response to execution commands such as @code{continue}.
gdb.texinfo:The final set of threads that are allowed to run by execution commands
gdb.texinfo:execution commands such as @code{continue} and @code{step} apply by default
gdb.texinfo:In non-stop mode, all execution commands apply only to the current thread
gdb.texinfo:You can use @value{GDBN}'s background execution commands
gdb.texinfo:The MI execution commands (@pxref{GDB/MI Program Execution}) are
gdb.texinfo:Other execution commands do not currently support the @code{-a} option.
gdb.texinfo:@value{GDBN}'s execution commands have two variants:  the normal
gdb.texinfo:background execution commands.  You can use these commands to
gdb.texinfo:message if you attempt to use the background execution commands.
gdb.texinfo:just @code{c&}.  The execution commands that accept background execution
gdb.texinfo:the restriction that you cannot issue another execution command until the
gdb.texinfo:@value{GDBN} will perform all execution commands in reverse, until the
gdb.texinfo:@value{GDBN} will perform all execution commands in the normal fashion.
gdb.texinfo:replay it later with both forward and reverse execution commands.
gdb.texinfo:associated with the stop message is the one for the execution command
gdb.texinfo:asynchronous just like other execution commands.  That is, first the

> 
> > +@cindex resume multiple processes
> 
> This second instance of the same index item is redundant.
> 
> > +Sets the mode for scheduling multiple processes.  When set, all
>    ^^^^
> We use "Set", not "Sets", throughout the manual.

Ok.

> 
> Also, I suggest to mention threads:
> 
>   Set the mode for scheduling threads of multiple processes.
> 
> On second thought, "scheduling" is not the best word here, as GDB is
> not a scheduler.  How about "continuing the execution"?

as in:

 Set the mode for continuing the execution of multiple processes.

? I think that "continuing" may trigger confusion by making it
look like only the "continue" command is affected.  See new patch below.

> 
> > +threads of all processes are allowed to run.  When not set (which is
> > +the default), only the threads of the current process are schedulable.
> 
> I think we say "when OFF" and "when ON", rather than "when set" and
> "when not set".

Ah, I see that, in the manual.  There are a few examples of boolean settings
in GDB's online help that use "When set", and I happened to copy from
one of those ("set step-mode", which is right below the new setting).
I've used ON/OFF everywhere now.

> > +@item show schedule-multiple
> > +Display the current mode for scheduling multiple processes.
> 
> Same comment about "threads" and "scheduling".
> 
> > +The final set of threads that are allowed to run by execution commands
> > +is defined by the union of the restrictions implied by both the
> > +@code{schedule-multiple} and @code{scheduler-locking} modes.  E.g., if
> > +the @code{schedule-multiple} mode is set to @code{on}, but the
> > +scheduler locking mode is also set to @code{on}, then only the current
> > +thread is made schedulable.
> 
> It might be easier and simpler to tell that scheduler-locking takes
> precedence.

It does not really take scrict precedence, due to the "step" scheduler-locking
variant.  I think it would be odd because I'd have to explain that
modes other than "scheduler-locking off" takes some sort of precedence.
Here's the set of modes this allows.

- continue all threads of all processes

   schedule-multiple on + scheduler-locking off

- continue all threads of the current process (default)

   schedule-multiple off + scheduler-locking off

- continue all threads of all processes, unless step is issued,
  in that case, step only the current thread.

   schedule-multiple on + scheduler-locking step

- continue all threads of the current process, unless step is
  issued, in that case, step only the current thread.

   schedule-multiple off + scheduler-locking step

- step only the current thread.

   schedule-multiple X + scheduler-locking on

IMO, these are intuitive given the nature and description
of both commands.

I thought of implementing this new feature new modes of
scheduler-locking instead, something like so:

set scheduler-locking off

 (all threads of all processes can run)

set scheduler-locking all-not-current-process

 (all threads of the current process can run, others remain locked)

set scheduler-locking all-not-current-process-step

 (all threads not of the current process remain locked,
  except when stepping, in which case, lock all except
  the current thread)

set scheduler-locking step

 (no thread is locked (all threads of all processes can run),
  except when stepping, in that case, lock all except
  the current thread)

set scheduler-locking on

 (all threads but the current remain locked)

But I looked to me that this is harder to understand.  The
nature of the "step" variant seems to want to be a different
setting from "resume all processes or just current process".

But I'm certainly open for discussion, hence the RFC.

It's certainly possible that experience shows that these modes
should be refined a bit differently.  In the mean time, this allows
me to go on with adding support for linux multi-process/multi-exec.

> 
> > +When set, all threads of all processes are affected by execution\n\
> > +commands (such as e.g., 'continue').  When not set (which is the\n\
> 
> You do not need both "such as" and "e.g.", one or the other is enough.

Ok.

> Also, the same comments as above about "ON/OFF" vs "set/not set" apply
> here.

Ok.

> We will also need an entry in NEWS for this new option.

Yep.  I was holding for the RFC to settle, but I've done
this now.

New patch below.  Thanks.

-- 
Pedro Alves

2009-05-30  Pedro Alves  <pedro@codesourcery.com>

	* gdb.texinfo (All-Stop): Document new 'set schedule-multiple'
	command.

2009-05-30  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (sched_multi): New global.
	(resume): If sched_multi is set, resume only threads of the
	current inferior.
	(prepare_to_proceed): Don't switch over to wait_ptid if we're
	resuming a different inferior, and sched_multi is off.
	(show_schedule_multiple): New.
	(_initialize_infrun): Register new "set schedule-multiple" command.
	* inferior.h (sched_multi): Declare.
	* NEWS: Mention new "schedule-multiple" setting.

---
 gdb/NEWS            |    5 ++++
 gdb/doc/gdb.texinfo |   30 ++++++++++++++++++++++++
 gdb/inferior.h      |    2 +
 gdb/infrun.c        |   63 +++++++++++++++++++++++++++++++++++++++++++---------
 4 files changed, 90 insertions(+), 10 deletions(-)

Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2009-05-30 10:28:15.000000000 +0100
+++ src/gdb/doc/gdb.texinfo	2009-05-30 16:34:46.000000000 +0100
@@ -4650,6 +4650,36 @@ the current thread away from the thread 
 Display the current scheduler locking mode.
 @end table
 
+@cindex resume threads of multiple processes simultaneously
+By default, @value{GDBN} allows only threads of the current inferior
+to run in response to execution commands such as @code{continue},
+@code{next} or @code{step}.  E.g., if @value{GDBN} is attached to two
+inferiors, each with two threads, the @code{continue} command resumes
+only the two threads of the current inferior.  You can modify
+@value{GDBN}'s default behavior by allowing all threads of all
+inferiors to run instead.
+
+@table @code
+@kindex set schedule-multiple
+@item set schedule-multiple
+Set the mode for allowing threads of multiple processes to be
+scheduled when an execution command is issued.  When @code{on}, all
+threads of all processes are allowed to run.  When @code{off}, only
+the threads of the current process are schedulable.  The default is
+@code{off}.
+
+@item show schedule-multiple
+Display the current mode for allowing threads of multiple processes to
+run.
+@end table
+
+The final set of threads that are allowed to run by execution commands
+is defined by the union of the restrictions implied by both the
+@code{schedule-multiple} and @code{scheduler-locking} modes.  E.g., if
+the @code{schedule-multiple} mode is set to @code{on}, but the
+scheduler locking mode is also set to @code{on}, then only the current
+thread is made schedulable.
+
 @node Non-Stop Mode
 @subsection Non-Stop Mode
 
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2009-05-30 01:36:41.000000000 +0100
+++ src/gdb/infrun.c	2009-05-30 16:19:21.000000000 +0100
@@ -1091,6 +1091,11 @@ set_schedlock_func (char *args, int from
     }
 }
 
+/* True if execution commands resume all threads of all processes by
+   default; otherwise, resume only threads of the current inferior
+   process.  */
+int sched_multi = 0;
+
 /* Try to setup for software single stepping over the specified location.
    Return 1 if target_resume() should use hardware single step.
 
@@ -1201,13 +1206,25 @@ a command like `return' or `jump' to con
     {
       ptid_t resume_ptid;
 
-      resume_ptid = RESUME_ALL;	/* Default */
-
       /* If STEP is set, it's a request to use hardware stepping
 	 facilities.  But in that case, we should never
 	 use singlestep breakpoint.  */
       gdb_assert (!(singlestep_breakpoints_inserted_p && step));
 
+      /* Decide the set of threads to ask the target to resume.  Start
+	 by assuming everything will be resumed, than narrow the set
+	 by applying increasingly restricting conditions.  */
+
+      /* By default, resume all threads of all processes.  */
+      resume_ptid = RESUME_ALL;
+
+      /* Maybe resume only all threads of the current process.  */
+      if (!sched_multi && target_supports_multi_process ())
+	{
+	  resume_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
+	}
+
+      /* Maybe resume a single thread after all.  */
       if (singlestep_breakpoints_inserted_p
 	  && stepping_past_singlestep_breakpoint)
 	{
@@ -1224,9 +1241,8 @@ a command like `return' or `jump' to con
 	     to support, and has no value.  */
 	  resume_ptid = inferior_ptid;
 	}
-
-      if ((step || singlestep_breakpoints_inserted_p)
-	  && tp->trap_expected)
+      else if ((step || singlestep_breakpoints_inserted_p)
+	       && tp->trap_expected)
 	{
 	  /* We're allowing a thread to run past a breakpoint it has
 	     hit, by single-stepping the thread with the breakpoint
@@ -1240,8 +1256,7 @@ a command like `return' or `jump' to con
 	     breakpoint, not just the one at PC.  */
 	  resume_ptid = inferior_ptid;
 	}
-
-      if (non_stop)
+      else if (non_stop)
 	{
 	  /* With non-stop mode on, threads are always handled
 	     individually.  */
@@ -1394,11 +1409,19 @@ prepare_to_proceed (int step)
 		       || (scheduler_mode == schedlock_step
 			   && step));
 
+  /* Don't switch over to WAIT_PTID if scheduler locking is on.  */
+  if (schedlock_enabled)
+    return 0;
+
+  /* Don't switch over if we're about to resume some other process
+     other than WAIT_PTID's, and schedule-multiple is off.  */
+  if (!sched_multi
+      && ptid_get_pid (wait_ptid) != ptid_get_pid (inferior_ptid))
+    return 0;
+
   /* Switched over from WAIT_PID.  */
   if (!ptid_equal (wait_ptid, minus_one_ptid)
-      && !ptid_equal (inferior_ptid, wait_ptid)
-      /* Don't single step WAIT_PID if scheduler locking is on.  */
-      && !schedlock_enabled)
+      && !ptid_equal (inferior_ptid, wait_ptid))
     {
       struct regcache *regcache = get_thread_regcache (wait_ptid);
 
@@ -5573,6 +5596,14 @@ show_non_stop (struct ui_file *file, int
 		    value);
 }
 
+static void
+show_schedule_multiple (struct ui_file *file, int from_tty,
+			struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("\
+Allowing threads of multiple processes be scheduled simultaneously is %s.\n"),
+		    value);
+}
 
 void
 _initialize_infrun (void)
@@ -5752,6 +5783,18 @@ step == scheduler locked during every si
 			show_scheduler_mode,
 			&setlist, &showlist);
 
+  add_setshow_boolean_cmd ("schedule-multiple", class_run, &sched_multi, _("\
+Set mode for allowing threads of multiple processes to be scheduled."), _("\
+Show mode for allowing threads of multiple processes to be scheduled."), _("\
+When on, execution commands (such as 'continue') resume all threads of\n\
+all processes.  When off (which is the default), execution commands\n\
+only resume the threads of the current process.  The set of threads\n\
+that are resumed is further refined by the scheduler-locking mode (see\n\
+help set scheduler-locking)."),
+			   NULL,
+			   show_schedule_multiple,
+			   &setlist, &showlist);
+
   add_setshow_boolean_cmd ("step-mode", class_run, &step_stop_if_no_debug, _("\
 Set mode of the step operation."), _("\
 Show mode of the step operation."), _("\
Index: src/gdb/inferior.h
===================================================================
--- src.orig/gdb/inferior.h	2009-05-30 01:36:12.000000000 +0100
+++ src/gdb/inferior.h	2009-05-30 12:49:53.000000000 +0100
@@ -135,6 +135,8 @@ extern void clear_proceed_status (void);
 
 extern void proceed (CORE_ADDR, enum target_signal, int);
 
+extern int sched_multi;
+
 /* When set, stop the 'step' command if we enter a function which has
    no line number information.  The normal behavior is that we step
    over such function.  */
Index: src/gdb/NEWS
===================================================================
--- src.orig/gdb/NEWS	2009-05-30 16:22:56.000000000 +0100
+++ src/gdb/NEWS	2009-05-30 16:35:58.000000000 +0100
@@ -305,6 +305,11 @@ show libthread-db-search-path
   Control list of directories which GDB will search for appropriate
   libthread_db.
 
+set schedule-multiple (on|off)
+show schedule-multiple
+  Allow GDB to resume all threads of all processes or only threads of
+  the current process.
+
 * New native configurations
 
 x86/x86_64 Darwin		i[34567]86-*-darwin*