[PATCH] gdb: tweak scoped_disable_commit_resumed uses when resuming all threads in non-stop

[PATCH] gdb: tweak scoped_disable_commit_resumed uses when resuming all threads in non-stop

[Simon Marchi]

When doing "continue -a" in non-stop mode, each thread is individually
resumed while the commit resumed state is enabled.  This forces the
target to commit each resumption immediately, instead of being able to
batch things.

The reason is that there is no scoped_disable_commit_resumed around the
loop over threads in continue_1, when "non_stop && all_threads" is true.
Since the proceed function is called once for each thread, the
scoped_disable_commit_resumed in proceed therefore forces commit-resumed
between each thread resumption.  Add the necessary
scoped_disable_commit_resumed in continue_1 to avoid that.

I looked at the MI side of things, the function exec_continue, and found
that it was correct.  There is a similar iteration over threads, and
there is a scoped_disable_commit_resumed at the function scope.  This is
not wrong, but a bit more than we need.  The branches that just call
continue_1 do not need it, as continue_1 takes care of disabling commit
resumed.  So, move the scoped_disable_commit_resumed to the inner scope
where we iterate on threads and proceed them individually.

Here's an example debugging a multi-threaded program attached by
gdbserver (debug output trimmed for brevity):

    $ ./gdb -nx -q --data-directory=data-directory -ex "set non-stop" -ex "tar rem :1234"
    (gdb) set debug remote
    (gdb) set debug infrun
    (gdb) c -a
    Continuing.
    [infrun] proceed: enter
      [infrun] scoped_disable_commit_resumed: reason=proceeding
      [remote] Sending packet: $vCont;c:p14388.14388#90
      [infrun] reset: reason=proceeding
      [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
      [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
    [infrun] proceed: exit
    [infrun] proceed: enter
      [infrun] scoped_disable_commit_resumed: reason=proceeding
      [remote] Sending packet: $vCont;c:p14388.1438a#b9
      [infrun] reset: reason=proceeding
      [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
      [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
    [infrun] proceed: exit
    ... and so on for each thread ...

Notice how we send one vCont;c for each thread.  With the patch applied, we
send a single vCont;c at the end:

    [infrun] scoped_disable_commit_resumed: reason=continue all threads in non-stop
    [infrun] proceed: enter
      [infrun] scoped_disable_commit_resumed: reason=proceeding
      [infrun] reset: reason=proceeding
    [infrun] proceed: exit
    [infrun] clear_proceed_status_thread: Thread 85790.85792
    [infrun] proceed: enter
      [infrun] scoped_disable_commit_resumed: reason=proceeding
      [infrun] reset: reason=proceeding
    [infrun] proceed: exit
    ... proceeding threads individually ...
    [infrun] reset: reason=continue all threads in non-stop
    [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
    [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
    [remote] Sending packet: $vCont;c#a8

Change-Id: I331dd2473c5aa5114f89854196fed2a8fdd122bb
---
 gdb/infcmd.c     | 4 ++++
 gdb/mi/mi-main.c | 7 ++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 302db421a21..6d7b82b32fa 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -603,6 +603,8 @@ continue_1 (int all_threads)
       /* Backup current thread and selected frame and restore on scope
 	 exit.  */
       scoped_restore_current_thread restore_thread;
+      scoped_disable_commit_resumed disable_commit_resumed
+	("continue all threads in non-stop");
 
       iterate_over_threads (proceed_thread_callback, NULL);
 
@@ -623,6 +625,8 @@ continue_1 (int all_threads)
 	  */
 	  target_terminal::inferior ();
 	}
+
+      disable_commit_resumed.reset_and_commit ();
     }
   else
     {
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 44008d1c0a8..e28fae0cc6c 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -266,7 +266,6 @@ exec_continue (char **argv, int argc)
 {
   prepare_execution_command (current_inferior ()->top_target (), mi_async_p ());
 
-  scoped_disable_commit_resumed disable_commit_resumed ("mi continue");
 
   if (non_stop)
     {
@@ -279,6 +278,8 @@ exec_continue (char **argv, int argc)
       if (current_context->all || current_context->thread_group != -1)
 	{
 	  scoped_restore_current_thread restore_thread;
+	  scoped_disable_commit_resumed disable_commit_resumed
+	    ("MI continue all threads in non-stop");
 	  int pid = 0;
 
 	  if (!current_context->all)
@@ -288,7 +289,9 @@ exec_continue (char **argv, int argc)
 
 	      pid = inf->pid;
 	    }
+
 	  iterate_over_threads (proceed_thread_callback, &pid);
+	  disable_commit_resumed.reset_and_commit ();
 	}
       else
 	{
@@ -313,8 +316,6 @@ exec_continue (char **argv, int argc)
 	  continue_1 (1);
 	}
     }
-
-  disable_commit_resumed.reset_and_commit ();
 }
 
 static void
-- 
2.33.0

Re: [PATCH] gdb: tweak scoped_disable_commit_resumed uses when resuming all threads in non-stop

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-11-05 15:03:36 -0400]:

> When doing "continue -a" in non-stop mode, each thread is individually
> resumed while the commit resumed state is enabled.  This forces the
> target to commit each resumption immediately, instead of being able to
> batch things.
> 
> The reason is that there is no scoped_disable_commit_resumed around the
> loop over threads in continue_1, when "non_stop && all_threads" is true.
> Since the proceed function is called once for each thread, the
> scoped_disable_commit_resumed in proceed therefore forces commit-resumed
> between each thread resumption.  Add the necessary
> scoped_disable_commit_resumed in continue_1 to avoid that.
> 
> I looked at the MI side of things, the function exec_continue, and found
> that it was correct.  There is a similar iteration over threads, and
> there is a scoped_disable_commit_resumed at the function scope.  This is
> not wrong, but a bit more than we need.  The branches that just call
> continue_1 do not need it, as continue_1 takes care of disabling commit
> resumed.  So, move the scoped_disable_commit_resumed to the inner scope
> where we iterate on threads and proceed them individually.
> 
> Here's an example debugging a multi-threaded program attached by
> gdbserver (debug output trimmed for brevity):
> 
>     $ ./gdb -nx -q --data-directory=data-directory -ex "set non-stop" -ex "tar rem :1234"
>     (gdb) set debug remote
>     (gdb) set debug infrun
>     (gdb) c -a
>     Continuing.
>     [infrun] proceed: enter
>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>       [remote] Sending packet: $vCont;c:p14388.14388#90
>       [infrun] reset: reason=proceeding
>       [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
>       [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
>     [infrun] proceed: exit
>     [infrun] proceed: enter
>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>       [remote] Sending packet: $vCont;c:p14388.1438a#b9
>       [infrun] reset: reason=proceeding
>       [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
>       [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
>     [infrun] proceed: exit
>     ... and so on for each thread ...
> 
> Notice how we send one vCont;c for each thread.  With the patch applied, we
> send a single vCont;c at the end:
> 
>     [infrun] scoped_disable_commit_resumed: reason=continue all threads in non-stop
>     [infrun] proceed: enter
>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>       [infrun] reset: reason=proceeding
>     [infrun] proceed: exit
>     [infrun] clear_proceed_status_thread: Thread 85790.85792
>     [infrun] proceed: enter
>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>       [infrun] reset: reason=proceeding
>     [infrun] proceed: exit
>     ... proceeding threads individually ...
>     [infrun] reset: reason=continue all threads in non-stop
>     [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
>     [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
>     [remote] Sending packet: $vCont;c#a8

LGTM.

Thanks for the detailed explanation.

Andrew

> 
> Change-Id: I331dd2473c5aa5114f89854196fed2a8fdd122bb
> ---
>  gdb/infcmd.c     | 4 ++++
>  gdb/mi/mi-main.c | 7 ++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 302db421a21..6d7b82b32fa 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -603,6 +603,8 @@ continue_1 (int all_threads)
>        /* Backup current thread and selected frame and restore on scope
>  	 exit.  */
>        scoped_restore_current_thread restore_thread;
> +      scoped_disable_commit_resumed disable_commit_resumed
> +	("continue all threads in non-stop");
>  
>        iterate_over_threads (proceed_thread_callback, NULL);
>  
> @@ -623,6 +625,8 @@ continue_1 (int all_threads)
>  	  */
>  	  target_terminal::inferior ();
>  	}
> +
> +      disable_commit_resumed.reset_and_commit ();
>      }
>    else
>      {
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 44008d1c0a8..e28fae0cc6c 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -266,7 +266,6 @@ exec_continue (char **argv, int argc)
>  {
>    prepare_execution_command (current_inferior ()->top_target (), mi_async_p ());
>  
> -  scoped_disable_commit_resumed disable_commit_resumed ("mi continue");
>  
>    if (non_stop)
>      {
> @@ -279,6 +278,8 @@ exec_continue (char **argv, int argc)
>        if (current_context->all || current_context->thread_group != -1)
>  	{
>  	  scoped_restore_current_thread restore_thread;
> +	  scoped_disable_commit_resumed disable_commit_resumed
> +	    ("MI continue all threads in non-stop");
>  	  int pid = 0;
>  
>  	  if (!current_context->all)
> @@ -288,7 +289,9 @@ exec_continue (char **argv, int argc)
>  
>  	      pid = inf->pid;
>  	    }
> +
>  	  iterate_over_threads (proceed_thread_callback, &pid);
> +	  disable_commit_resumed.reset_and_commit ();
>  	}
>        else
>  	{
> @@ -313,8 +316,6 @@ exec_continue (char **argv, int argc)
>  	  continue_1 (1);
>  	}
>      }
> -
> -  disable_commit_resumed.reset_and_commit ();
>  }
>  
>  static void
> -- 
> 2.33.0
> 

Re: [PATCH] gdb: tweak scoped_disable_commit_resumed uses when resuming all threads in non-stop

[Simon Marchi]

On 2021-11-08 12:50 p.m., Andrew Burgess via Gdb-patches wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-11-05 15:03:36 -0400]:
> 
>> When doing "continue -a" in non-stop mode, each thread is individually
>> resumed while the commit resumed state is enabled.  This forces the
>> target to commit each resumption immediately, instead of being able to
>> batch things.
>>
>> The reason is that there is no scoped_disable_commit_resumed around the
>> loop over threads in continue_1, when "non_stop && all_threads" is true.
>> Since the proceed function is called once for each thread, the
>> scoped_disable_commit_resumed in proceed therefore forces commit-resumed
>> between each thread resumption.  Add the necessary
>> scoped_disable_commit_resumed in continue_1 to avoid that.
>>
>> I looked at the MI side of things, the function exec_continue, and found
>> that it was correct.  There is a similar iteration over threads, and
>> there is a scoped_disable_commit_resumed at the function scope.  This is
>> not wrong, but a bit more than we need.  The branches that just call
>> continue_1 do not need it, as continue_1 takes care of disabling commit
>> resumed.  So, move the scoped_disable_commit_resumed to the inner scope
>> where we iterate on threads and proceed them individually.
>>
>> Here's an example debugging a multi-threaded program attached by
>> gdbserver (debug output trimmed for brevity):
>>
>>     $ ./gdb -nx -q --data-directory=data-directory -ex "set non-stop" -ex "tar rem :1234"
>>     (gdb) set debug remote
>>     (gdb) set debug infrun
>>     (gdb) c -a
>>     Continuing.
>>     [infrun] proceed: enter
>>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>>       [remote] Sending packet: $vCont;c:p14388.14388#90
>>       [infrun] reset: reason=proceeding
>>       [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
>>       [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
>>     [infrun] proceed: exit
>>     [infrun] proceed: enter
>>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>>       [remote] Sending packet: $vCont;c:p14388.1438a#b9
>>       [infrun] reset: reason=proceeding
>>       [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
>>       [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
>>     [infrun] proceed: exit
>>     ... and so on for each thread ...
>>
>> Notice how we send one vCont;c for each thread.  With the patch applied, we
>> send a single vCont;c at the end:
>>
>>     [infrun] scoped_disable_commit_resumed: reason=continue all threads in non-stop
>>     [infrun] proceed: enter
>>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>>       [infrun] reset: reason=proceeding
>>     [infrun] proceed: exit
>>     [infrun] clear_proceed_status_thread: Thread 85790.85792
>>     [infrun] proceed: enter
>>       [infrun] scoped_disable_commit_resumed: reason=proceeding
>>       [infrun] reset: reason=proceeding
>>     [infrun] proceed: exit
>>     ... proceeding threads individually ...
>>     [infrun] reset: reason=continue all threads in non-stop
>>     [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
>>     [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
>>     [remote] Sending packet: $vCont;c#a8
> 
> LGTM.
> 
> Thanks for the detailed explanation.
> 
> Andrew

Thanks, pushed.

Simon