Re: [PATCH 5/6] gdb: add commands to control scheduler locking.

[Eli Zaretskii <eliz@gnu.org>]

> From: Natalia Saiapova <natalia.saiapova@intel.com>
> Cc: tankut.baris.aktemur@intel.com
> Date: Fri, 29 Dec 2023 10:42:01 +0000
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -18,6 +18,23 @@ disassemble
>  
>  * New commands
>  
> +set scheduler-locking replay run | replay step | run | step (on|off)
> +show scheduler-locking (replay run | replay step | run | step)
> +  Extend the scheduler locking settings with a set of set/show
> +  commands, which can be used individually to control the scheduler during
> +  various commands.
> +    'replay run' -- when on, the scheduler is locked during non-stepping
> +    commands in replay mode.
> +    'replay step' -- when on, the scheduler is locked during stepping
> +    commands in replay mode.
> +    'run' -- when on, the scheduler is locked during non-stepping commands
> +    in normal mode.
> +    'step' -- when on, the scheduler is locked during stepping commands
> +    in normal mode.
> +  The older scheduler locking settings can be used as shortcuts, their behavior
> +  is not changed.
> +  The output of "show scheduler-locking" has changed to support the new settings.
> +

I suggest to mention a couple of examples of non-stepping commands
where these settings are relevant.

> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -7110,22 +7110,56 @@ On some OSes, you can modify @value{GDBN}'s default behavior by
>  locking the OS scheduler to allow only a single thread to run.
>  
>  @table @code
> -@item set scheduler-locking @var{mode}
> -@cindex scheduler-locking
> +
> +@item set scheduler-locking @var{type} [@code{on}|@code{off}]
> +@cindex scheduler locking type
> +@cindex lock scheduler
> +Set the scheduler locking settings.  It applies to normal execution,
> +record mode, and replay mode.  The scheduler locking can be set separately
> +for stepping and non-stepping commands.

Similarly here.  In the manual, it is actually much more important,
since the manual is supposed to be the ultimate source of information
about GDB commands: after reading the relevant parts of the manual,
the user is supposed to understand the command enough to be able to
use it intelligently and correctly.

> +@table @code
> +@item replay run
> +When @code{on} the scheduler is locked for non-stepping commands during
                 ^
A comma is missing there.

> +replay mode.  For commands like @samp{continue}, @samp{until}, @samp{finish},
> +or expression evaluation only the current thread may run.

The last sentence should IMO be moved to where you say "non-stepping
commands" above, because it actually explains what are those commands.

Btw, I'm a bit worried about our use of "scheduler lock", with the
meaning of "prevent other threads from running".  We never say that
locking the scheduler means only the current thread is allowed to run,
we rely on the readers to figure that out on their own.  (Yes, I
understand that this was the problem with the original text as well.)

> +@item replay step
> +When @code{on} the scheduler is locked for stepping commands during replay
> +mode.  This mode optimizes for single-stepping; only the current thread is
> +resumed while you are stepping, so that the focus of debugging does not change
> +unexpectedly.
> +
> +@item run
> +When @code{on} the scheduler is locked for non-stepping commands during
> +normal execution and record modes.  For commands like @samp{continue},
> +@samp{until}, @samp{finish}, or expression evaluation only the current
> +thread may run.
> +
> +@item step
> +When @code{on} the scheduler is locked for stepping commands during
> +normal execution and record modes.  This mode optimizes for single-stepping;
> +only the current thread is resumed while you are stepping, so that the focus
> +of debugging does not change unexpectedly.
> +
> +@end table

The above description lost the information about the default setting.
To understand what is the default, the reader needs to read further,
where you describe the old settings ("shortcuts") and their
equivalence to the new one; this is sub-optimal.

> -Only the current thread may run when the inferior is resumed.  New
> +Only the current thread may run when the inferior is resumed.   New
                                                                ^^^
Redundant whitespace there.

Thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>