[PATCH v2 0/1] Refactor proceed function

[PATCH v2 0/1] Refactor proceed function

[Christina Schimpe]

Hi all,

this patch was originally written by Mihails Strasuns.  This version addresses
the feedback of Andrew to improve comments. I also added myself as co-author in
the commit message.

V1 of this patch can be found here:
https://sourceware.org/pipermail/gdb-patches/2023-June/200105.html

Regards,
Christina

Mihails Strasuns (1):
  gdb, infrun: refactor part of `proceed` into separate function

 gdb/infrun.c | 120 ++++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 59 deletions(-)

-- 
2.25.1

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

[PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function

[Christina Schimpe]

From: Mihails Strasuns <mihails.strasuns@intel.com>

Split thread resuming block into separate function.

Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
---
 gdb/infrun.c | 120 ++++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 59 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 58da1cef29e..ec41b658864 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target)
     }
 }
 
+  /* Helper function for `proceed`.  Check if thread TP is suitable for
+     resuming, and, if it is, switch to the thread and call
+     `keep_going_pass_signal`.  If TP is not suitable for resuming then
+     this function will just return without switching threads.  */
+
+static void
+proceed_resume_thread_checked (thread_info *tp)
+{
+  if (!tp->inf->has_execution ())
+    {
+      infrun_debug_printf ("[%s] target has no execution",
+			   tp->ptid.to_string ().c_str ());
+      return;
+    }
+
+  if (tp->resumed ())
+    {
+      infrun_debug_printf ("[%s] resumed",
+			   tp->ptid.to_string ().c_str ());
+      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
+      return;
+    }
+
+  if (thread_is_in_step_over_chain (tp))
+    {
+      infrun_debug_printf ("[%s] needs step-over",
+			   tp->ptid.to_string ().c_str ());
+      return;
+    }
+
+  /* There are further two scenarios for which we must not resume the thread:
+     - If a thread of that inferior is waiting for a vfork-done
+       (for a detached vfork child to exec or exit), breakpoints are
+       removed.  We must not resume any thread of that inferior, other
+       than the one waiting for the vfork-done.
+     - In non-stop, forbid resuming a thread if some other thread of
+       that inferior is waiting for a vfork-done event (this means
+       breakpoints are out for this inferior).  */
+  if (tp->inf->thread_waiting_for_vfork_done != nullptr
+      && (tp != tp->inf->thread_waiting_for_vfork_done
+	  || non_stop))
+    {
+      infrun_debug_printf ("[%s] another thread of this inferior is "
+			   "waiting for vfork-done",
+			   tp->ptid.to_string ().c_str ());
+      return;
+    }
+
+  infrun_debug_printf ("resuming %s",
+		       tp->ptid.to_string ().c_str ());
+
+  execution_control_state ecs (tp);
+  switch_to_thread (tp);
+  keep_going_pass_signal (&ecs);
+  if (!ecs.wait_some_more)
+    error (_("Command aborted."));
+}
+
 /* Basic routine for continuing the program in various fashions.
 
    ADDR is the address to resume at, or -1 for resume where stopped.
@@ -3456,67 +3514,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
 						       resume_ptid))
 	  {
 	    switch_to_thread_no_regs (tp);
-
-	    if (!tp->inf->has_execution ())
-	      {
-		infrun_debug_printf ("[%s] target has no execution",
-				     tp->ptid.to_string ().c_str ());
-		continue;
-	      }
-
-	    if (tp->resumed ())
-	      {
-		infrun_debug_printf ("[%s] resumed",
-				     tp->ptid.to_string ().c_str ());
-		gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
-		continue;
-	      }
-
-	    if (thread_is_in_step_over_chain (tp))
-	      {
-		infrun_debug_printf ("[%s] needs step-over",
-				     tp->ptid.to_string ().c_str ());
-		continue;
-	      }
-
-	    /* If a thread of that inferior is waiting for a vfork-done
-	       (for a detached vfork child to exec or exit), breakpoints are
-	       removed.  We must not resume any thread of that inferior, other
-	       than the one waiting for the vfork-done.  */
-	    if (tp->inf->thread_waiting_for_vfork_done != nullptr
-		&& tp != tp->inf->thread_waiting_for_vfork_done)
-	      {
-		infrun_debug_printf ("[%s] another thread of this inferior is "
-				     "waiting for vfork-done",
-				     tp->ptid.to_string ().c_str ());
-		continue;
-	      }
-
-	    infrun_debug_printf ("resuming %s",
-				 tp->ptid.to_string ().c_str ());
-
-	    execution_control_state ecs (tp);
-	    switch_to_thread (tp);
-	    keep_going_pass_signal (&ecs);
-	    if (!ecs.wait_some_more)
-	      error (_("Command aborted."));
+	    proceed_resume_thread_checked (tp);
 	  }
       }
-    else if (!cur_thr->resumed ()
-	     && !thread_is_in_step_over_chain (cur_thr)
-	     /* In non-stop, forbid resuming a thread if some other thread of
-		that inferior is waiting for a vfork-done event (this means
-		breakpoints are out for this inferior).  */
-	     && !(non_stop
-		  && cur_thr->inf->thread_waiting_for_vfork_done != nullptr))
-      {
-	/* The thread wasn't started, and isn't queued, run it now.  */
-	execution_control_state ecs (cur_thr);
-	switch_to_thread (cur_thr);
-	keep_going_pass_signal (&ecs);
-	if (!ecs.wait_some_more)
-	  error (_("Command aborted."));
-      }
+    else
+      proceed_resume_thread_checked (cur_thr);
 
     disable_commit_resumed.reset_and_commit ();
   }
-- 
2.25.1

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

Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function

[Andrew Burgess]

Christina Schimpe <christina.schimpe@intel.com> writes:

> From: Mihails Strasuns <mihails.strasuns@intel.com>
>
> Split thread resuming block into separate function.
>
> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
> ---
>  gdb/infrun.c | 120 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 61 insertions(+), 59 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 58da1cef29e..ec41b658864 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>      }
>  }
>  
> +  /* Helper function for `proceed`.  Check if thread TP is suitable for
> +     resuming, and, if it is, switch to the thread and call
> +     `keep_going_pass_signal`.  If TP is not suitable for resuming then
> +     this function will just return without switching threads.  */

The indentation here is wrong -- header comments should start in the
first column

> +
> +static void
> +proceed_resume_thread_checked (thread_info *tp)
> +{
> +  if (!tp->inf->has_execution ())
> +    {
> +      infrun_debug_printf ("[%s] target has no execution",
> +			   tp->ptid.to_string ().c_str ());
> +      return;
> +    }
> +
> +  if (tp->resumed ())
> +    {
> +      infrun_debug_printf ("[%s] resumed",
> +			   tp->ptid.to_string ().c_str ());
> +      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
> +      return;
> +    }
> +
> +  if (thread_is_in_step_over_chain (tp))
> +    {
> +      infrun_debug_printf ("[%s] needs step-over",
> +			   tp->ptid.to_string ().c_str ());
> +      return;
> +    }
> +
> +  /* There are further two scenarios for which we must not resume the thread:
> +     - If a thread of that inferior is waiting for a vfork-done
> +       (for a detached vfork child to exec or exit), breakpoints are
> +       removed.  We must not resume any thread of that inferior, other
> +       than the one waiting for the vfork-done.
> +     - In non-stop, forbid resuming a thread if some other thread of
> +       that inferior is waiting for a vfork-done event (this means
> +       breakpoints are out for this inferior).  */
> +  if (tp->inf->thread_waiting_for_vfork_done != nullptr
> +      && (tp != tp->inf->thread_waiting_for_vfork_done
> +	  || non_stop))
> +    {
> +      infrun_debug_printf ("[%s] another thread of this inferior is "
> +			   "waiting for vfork-done",
> +			   tp->ptid.to_string ().c_str ());
> +      return;
> +    }

I tried to like this merged logic block, but I just couldn't.  So in the
end I went back to the original commit:

  commit d8bbae6ea080249c05ca90b1f8640fde48a18301
  Date:   Fri Jan 14 15:40:59 2022 -0500
  
      gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)

And tried to understand what's going on.  The commit message on that
original commit is awesome, and well worth a read.

After looking at that for a while I think I have a replacement for this
block, which I think achieves the same thing, but is structured, and
commented slightly differently.

I've included an updated version of this patch below, I'd love to hear
what you think?

The patch below is only partially tested at this point.  I'm a little
short of time right now.  But it passes all the vfork related tests that
I've been looking at, so I'm reasonably sure it's OK, I just wanted to
get this out so you could take a look at it.

Thanks,
Andrew

---

commit 45460db250f116956f4d75376f6637d2e4e7632f
Author: Mihails Strasuns <mihails.strasuns@intel.com>
Date:   Mon Jun 12 09:49:27 2023 +0200

    gdb, infrun: refactor part of `proceed` into separate function
    
    Split the thread resuming code from proceed into new function
    proceed_resume_thread_checked.
    
    Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 58da1cef29e..7e6104ffab2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3268,6 +3268,90 @@ check_multi_target_resumption (process_stratum_target *resume_target)
     }
 }
 
+/* Helper function for `proceed`.  Check if thread TP is suitable for
+   resuming, and, if it is, switch to the thread and call
+   `keep_going_pass_signal`.  If TP is not suitable for resuming then this
+   function will just return without switching threads.  */
+
+static void
+proceed_resume_thread_checked (thread_info *tp)
+{
+  if (!tp->inf->has_execution ())
+    {
+      infrun_debug_printf ("[%s] target has no execution",
+			   tp->ptid.to_string ().c_str ());
+      return;
+    }
+
+  if (tp->resumed ())
+    {
+      infrun_debug_printf ("[%s] resumed",
+			   tp->ptid.to_string ().c_str ());
+      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
+      return;
+    }
+
+  if (thread_is_in_step_over_chain (tp))
+    {
+      infrun_debug_printf ("[%s] needs step-over",
+			   tp->ptid.to_string ().c_str ());
+      return;
+    }
+
+  /* When handling a vfork GDB removes all breakpoints from the program
+     space in which the vfork is being handled, as such we must take care
+     not to resume any thread other than the vfork parent -- resuming the
+     vfork parent allows GDB to receive and handle the 'vfork done'
+     event.  */
+  if (tp->inf->thread_waiting_for_vfork_done != nullptr)
+    {
+      if (target_is_non_stop_p ())
+	{
+	  /* For non-stop targets, regardless of whether GDB is using
+	     all-stop or non-stop mode, threads are controlled
+	     individually.
+
+	     When a thread is handling a vfork, breakpoints are removed
+	     from the inferior (well, program space in fact), so it is
+	     critical that we don't try to resume any thread other than the
+	     vfork parent.  */
+	  if (tp != tp->inf->thread_waiting_for_vfork_done)
+	    {
+	      infrun_debug_printf ("[%s] another thread of this inferior is "
+				   "waiting for vfork-done",
+				   tp->ptid.to_string ().c_str ());
+	      return;
+	    }
+
+	  /* In non-stop mode we will never end up trying to proceed the
+	     thread that is handling a vfork.  When we realise that a
+	     thread is handling a vfork we immediately stop all the other
+	     threads, and resume the vfork parent thread.  As far as the
+	     user is concerned, the vfork parent thread is running.  Any
+	     attempt by the user to interrupt the vfork parent will be
+	     deferred until the vfork has completed, at which point the
+	     parent will no longer be waiting for a vfork.  */
+	  gdb_assert (!non_stop);
+	}
+      else
+	{
+	  /* For all-stop targets, when we attempt to resume the inferior,
+	     we will in fact only resume the vfork parent thread, this is
+	     handled in internal_resume_ptid, as such, there is nothing
+	     more that needs doing here.  */
+	}
+    }
+
+  infrun_debug_printf ("resuming %s",
+		       tp->ptid.to_string ().c_str ());
+
+  execution_control_state ecs (tp);
+  switch_to_thread (tp);
+  keep_going_pass_signal (&ecs);
+  if (!ecs.wait_some_more)
+    error (_("Command aborted."));
+}
+
 /* Basic routine for continuing the program in various fashions.
 
    ADDR is the address to resume at, or -1 for resume where stopped.
@@ -3456,67 +3540,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
 						       resume_ptid))
 	  {
 	    switch_to_thread_no_regs (tp);
-
-	    if (!tp->inf->has_execution ())
-	      {
-		infrun_debug_printf ("[%s] target has no execution",
-				     tp->ptid.to_string ().c_str ());
-		continue;
-	      }
-
-	    if (tp->resumed ())
-	      {
-		infrun_debug_printf ("[%s] resumed",
-				     tp->ptid.to_string ().c_str ());
-		gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
-		continue;
-	      }
-
-	    if (thread_is_in_step_over_chain (tp))
-	      {
-		infrun_debug_printf ("[%s] needs step-over",
-				     tp->ptid.to_string ().c_str ());
-		continue;
-	      }
-
-	    /* If a thread of that inferior is waiting for a vfork-done
-	       (for a detached vfork child to exec or exit), breakpoints are
-	       removed.  We must not resume any thread of that inferior, other
-	       than the one waiting for the vfork-done.  */
-	    if (tp->inf->thread_waiting_for_vfork_done != nullptr
-		&& tp != tp->inf->thread_waiting_for_vfork_done)
-	      {
-		infrun_debug_printf ("[%s] another thread of this inferior is "
-				     "waiting for vfork-done",
-				     tp->ptid.to_string ().c_str ());
-		continue;
-	      }
-
-	    infrun_debug_printf ("resuming %s",
-				 tp->ptid.to_string ().c_str ());
-
-	    execution_control_state ecs (tp);
-	    switch_to_thread (tp);
-	    keep_going_pass_signal (&ecs);
-	    if (!ecs.wait_some_more)
-	      error (_("Command aborted."));
+	    proceed_resume_thread_checked (tp);
 	  }
       }
-    else if (!cur_thr->resumed ()
-	     && !thread_is_in_step_over_chain (cur_thr)
-	     /* In non-stop, forbid resuming a thread if some other thread of
-		that inferior is waiting for a vfork-done event (this means
-		breakpoints are out for this inferior).  */
-	     && !(non_stop
-		  && cur_thr->inf->thread_waiting_for_vfork_done != nullptr))
-      {
-	/* The thread wasn't started, and isn't queued, run it now.  */
-	execution_control_state ecs (cur_thr);
-	switch_to_thread (cur_thr);
-	keep_going_pass_signal (&ecs);
-	if (!ecs.wait_some_more)
-	  error (_("Command aborted."));
-      }
+    else
+      proceed_resume_thread_checked (cur_thr);
 
     disable_commit_resumed.reset_and_commit ();
   }

Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> Christina Schimpe <christina.schimpe@intel.com> writes:
>
>> From: Mihails Strasuns <mihails.strasuns@intel.com>
>>
>> Split thread resuming block into separate function.
>>
>> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
>> ---
>>  gdb/infrun.c | 120 ++++++++++++++++++++++++++-------------------------
>>  1 file changed, 61 insertions(+), 59 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 58da1cef29e..ec41b658864 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>>      }
>>  }
>>  
>> +  /* Helper function for `proceed`.  Check if thread TP is suitable for
>> +     resuming, and, if it is, switch to the thread and call
>> +     `keep_going_pass_signal`.  If TP is not suitable for resuming then
>> +     this function will just return without switching threads.  */
>
> The indentation here is wrong -- header comments should start in the
> first column
>
>> +
>> +static void
>> +proceed_resume_thread_checked (thread_info *tp)
>> +{
>> +  if (!tp->inf->has_execution ())
>> +    {
>> +      infrun_debug_printf ("[%s] target has no execution",
>> +			   tp->ptid.to_string ().c_str ());
>> +      return;
>> +    }
>> +
>> +  if (tp->resumed ())
>> +    {
>> +      infrun_debug_printf ("[%s] resumed",
>> +			   tp->ptid.to_string ().c_str ());
>> +      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
>> +      return;
>> +    }
>> +
>> +  if (thread_is_in_step_over_chain (tp))
>> +    {
>> +      infrun_debug_printf ("[%s] needs step-over",
>> +			   tp->ptid.to_string ().c_str ());
>> +      return;
>> +    }
>> +
>> +  /* There are further two scenarios for which we must not resume the thread:
>> +     - If a thread of that inferior is waiting for a vfork-done
>> +       (for a detached vfork child to exec or exit), breakpoints are
>> +       removed.  We must not resume any thread of that inferior, other
>> +       than the one waiting for the vfork-done.
>> +     - In non-stop, forbid resuming a thread if some other thread of
>> +       that inferior is waiting for a vfork-done event (this means
>> +       breakpoints are out for this inferior).  */
>> +  if (tp->inf->thread_waiting_for_vfork_done != nullptr
>> +      && (tp != tp->inf->thread_waiting_for_vfork_done
>> +	  || non_stop))
>> +    {
>> +      infrun_debug_printf ("[%s] another thread of this inferior is "
>> +			   "waiting for vfork-done",
>> +			   tp->ptid.to_string ().c_str ());
>> +      return;
>> +    }
>
> I tried to like this merged logic block, but I just couldn't.  So in the
> end I went back to the original commit:
>
>   commit d8bbae6ea080249c05ca90b1f8640fde48a18301
>   Date:   Fri Jan 14 15:40:59 2022 -0500
>   
>       gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)
>
> And tried to understand what's going on.  The commit message on that
> original commit is awesome, and well worth a read.
>
> After looking at that for a while I think I have a replacement for this
> block, which I think achieves the same thing, but is structured, and
> commented slightly differently.
>
> I've included an updated version of this patch below, I'd love to hear
> what you think?
>
> The patch below is only partially tested at this point.  I'm a little
> short of time right now.  But it passes all the vfork related tests that
> I've been looking at, so I'm reasonably sure it's OK, I just wanted to
> get this out so you could take a look at it.
>
> Thanks,
> Andrew
>
> ---
>
> commit 45460db250f116956f4d75376f6637d2e4e7632f
> Author: Mihails Strasuns <mihails.strasuns@intel.com>
> Date:   Mon Jun 12 09:49:27 2023 +0200
>
>     gdb, infrun: refactor part of `proceed` into separate function
>     
>     Split the thread resuming code from proceed into new function
>     proceed_resume_thread_checked.
>     
>     Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 58da1cef29e..7e6104ffab2 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3268,6 +3268,90 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>      }
>  }
>  
> +/* Helper function for `proceed`.  Check if thread TP is suitable for
> +   resuming, and, if it is, switch to the thread and call
> +   `keep_going_pass_signal`.  If TP is not suitable for resuming then this
> +   function will just return without switching threads.  */
> +
> +static void
> +proceed_resume_thread_checked (thread_info *tp)
> +{
> +  if (!tp->inf->has_execution ())
> +    {
> +      infrun_debug_printf ("[%s] target has no execution",
> +			   tp->ptid.to_string ().c_str ());
> +      return;
> +    }
> +
> +  if (tp->resumed ())
> +    {
> +      infrun_debug_printf ("[%s] resumed",
> +			   tp->ptid.to_string ().c_str ());
> +      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
> +      return;
> +    }
> +
> +  if (thread_is_in_step_over_chain (tp))
> +    {
> +      infrun_debug_printf ("[%s] needs step-over",
> +			   tp->ptid.to_string ().c_str ());
> +      return;
> +    }
> +
> +  /* When handling a vfork GDB removes all breakpoints from the program
> +     space in which the vfork is being handled, as such we must take care
> +     not to resume any thread other than the vfork parent -- resuming the
> +     vfork parent allows GDB to receive and handle the 'vfork done'
> +     event.  */
> +  if (tp->inf->thread_waiting_for_vfork_done != nullptr)
> +    {
> +      if (target_is_non_stop_p ())
> +	{
> +	  /* For non-stop targets, regardless of whether GDB is using
> +	     all-stop or non-stop mode, threads are controlled
> +	     individually.
> +
> +	     When a thread is handling a vfork, breakpoints are removed
> +	     from the inferior (well, program space in fact), so it is
> +	     critical that we don't try to resume any thread other than the
> +	     vfork parent.  */
> +	  if (tp != tp->inf->thread_waiting_for_vfork_done)
> +	    {
> +	      infrun_debug_printf ("[%s] another thread of this inferior is "
> +				   "waiting for vfork-done",
> +				   tp->ptid.to_string ().c_str ());
> +	      return;
> +	    }

This actually fixes a bug that exists currently in GDB.  I'm working on
a patch which includes a test that exposes the bug.  I plan to get the
bug fix merged before we merge this refactoring commit.

> +
> +	  /* In non-stop mode we will never end up trying to proceed the
> +	     thread that is handling a vfork.  When we realise that a
> +	     thread is handling a vfork we immediately stop all the other
> +	     threads, and resume the vfork parent thread.  As far as the
> +	     user is concerned, the vfork parent thread is running.  Any
> +	     attempt by the user to interrupt the vfork parent will be
> +	     deferred until the vfork has completed, at which point the
> +	     parent will no longer be waiting for a vfork.  */
> +	  gdb_assert (!non_stop);

And this assertion is not correct.  Again, the new test I'll add will
trigger this assert.

We can get this refactor merged once my bug fix has landed.  I'll follow
up once I have something posted.

Thanks,
Andrew

> +	}
> +      else
> +	{
> +	  /* For all-stop targets, when we attempt to resume the inferior,
> +	     we will in fact only resume the vfork parent thread, this is
> +	     handled in internal_resume_ptid, as such, there is nothing
> +	     more that needs doing here.  */
> +	}
> +    }
> +
> +  infrun_debug_printf ("resuming %s",
> +		       tp->ptid.to_string ().c_str ());
> +
> +  execution_control_state ecs (tp);
> +  switch_to_thread (tp);
> +  keep_going_pass_signal (&ecs);
> +  if (!ecs.wait_some_more)
> +    error (_("Command aborted."));
> +}
> +
>  /* Basic routine for continuing the program in various fashions.
>  
>     ADDR is the address to resume at, or -1 for resume where stopped.
> @@ -3456,67 +3540,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
>  						       resume_ptid))
>  	  {
>  	    switch_to_thread_no_regs (tp);
> -
> -	    if (!tp->inf->has_execution ())
> -	      {
> -		infrun_debug_printf ("[%s] target has no execution",
> -				     tp->ptid.to_string ().c_str ());
> -		continue;
> -	      }
> -
> -	    if (tp->resumed ())
> -	      {
> -		infrun_debug_printf ("[%s] resumed",
> -				     tp->ptid.to_string ().c_str ());
> -		gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
> -		continue;
> -	      }
> -
> -	    if (thread_is_in_step_over_chain (tp))
> -	      {
> -		infrun_debug_printf ("[%s] needs step-over",
> -				     tp->ptid.to_string ().c_str ());
> -		continue;
> -	      }
> -
> -	    /* If a thread of that inferior is waiting for a vfork-done
> -	       (for a detached vfork child to exec or exit), breakpoints are
> -	       removed.  We must not resume any thread of that inferior, other
> -	       than the one waiting for the vfork-done.  */
> -	    if (tp->inf->thread_waiting_for_vfork_done != nullptr
> -		&& tp != tp->inf->thread_waiting_for_vfork_done)
> -	      {
> -		infrun_debug_printf ("[%s] another thread of this inferior is "
> -				     "waiting for vfork-done",
> -				     tp->ptid.to_string ().c_str ());
> -		continue;
> -	      }
> -
> -	    infrun_debug_printf ("resuming %s",
> -				 tp->ptid.to_string ().c_str ());
> -
> -	    execution_control_state ecs (tp);
> -	    switch_to_thread (tp);
> -	    keep_going_pass_signal (&ecs);
> -	    if (!ecs.wait_some_more)
> -	      error (_("Command aborted."));
> +	    proceed_resume_thread_checked (tp);
>  	  }
>        }
> -    else if (!cur_thr->resumed ()
> -	     && !thread_is_in_step_over_chain (cur_thr)
> -	     /* In non-stop, forbid resuming a thread if some other thread of
> -		that inferior is waiting for a vfork-done event (this means
> -		breakpoints are out for this inferior).  */
> -	     && !(non_stop
> -		  && cur_thr->inf->thread_waiting_for_vfork_done != nullptr))
> -      {
> -	/* The thread wasn't started, and isn't queued, run it now.  */
> -	execution_control_state ecs (cur_thr);
> -	switch_to_thread (cur_thr);
> -	keep_going_pass_signal (&ecs);
> -	if (!ecs.wait_some_more)
> -	  error (_("Command aborted."));
> -      }
> +    else
> +      proceed_resume_thread_checked (cur_thr);
>  
>      disable_commit_resumed.reset_and_commit ();
>    }

Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> Andrew Burgess <aburgess@redhat.com> writes:
>
>> Christina Schimpe <christina.schimpe@intel.com> writes:
>>
>>> From: Mihails Strasuns <mihails.strasuns@intel.com>
>>>
>>> Split thread resuming block into separate function.
>>>
>>> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
>>> ---
>>>  gdb/infrun.c | 120 ++++++++++++++++++++++++++-------------------------
>>>  1 file changed, 61 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>> index 58da1cef29e..ec41b658864 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>>>      }
>>>  }
>>>  
>>> +  /* Helper function for `proceed`.  Check if thread TP is suitable for
>>> +     resuming, and, if it is, switch to the thread and call
>>> +     `keep_going_pass_signal`.  If TP is not suitable for resuming then
>>> +     this function will just return without switching threads.  */
>>
>> The indentation here is wrong -- header comments should start in the
>> first column
>>
>>> +
>>> +static void
>>> +proceed_resume_thread_checked (thread_info *tp)
>>> +{
>>> +  if (!tp->inf->has_execution ())
>>> +    {
>>> +      infrun_debug_printf ("[%s] target has no execution",
>>> +			   tp->ptid.to_string ().c_str ());
>>> +      return;
>>> +    }
>>> +
>>> +  if (tp->resumed ())
>>> +    {
>>> +      infrun_debug_printf ("[%s] resumed",
>>> +			   tp->ptid.to_string ().c_str ());
>>> +      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
>>> +      return;
>>> +    }
>>> +
>>> +  if (thread_is_in_step_over_chain (tp))
>>> +    {
>>> +      infrun_debug_printf ("[%s] needs step-over",
>>> +			   tp->ptid.to_string ().c_str ());
>>> +      return;
>>> +    }
>>> +
>>> +  /* There are further two scenarios for which we must not resume the thread:
>>> +     - If a thread of that inferior is waiting for a vfork-done
>>> +       (for a detached vfork child to exec or exit), breakpoints are
>>> +       removed.  We must not resume any thread of that inferior, other
>>> +       than the one waiting for the vfork-done.
>>> +     - In non-stop, forbid resuming a thread if some other thread of
>>> +       that inferior is waiting for a vfork-done event (this means
>>> +       breakpoints are out for this inferior).  */
>>> +  if (tp->inf->thread_waiting_for_vfork_done != nullptr
>>> +      && (tp != tp->inf->thread_waiting_for_vfork_done
>>> +	  || non_stop))
>>> +    {
>>> +      infrun_debug_printf ("[%s] another thread of this inferior is "
>>> +			   "waiting for vfork-done",
>>> +			   tp->ptid.to_string ().c_str ());
>>> +      return;
>>> +    }
>>
>> I tried to like this merged logic block, but I just couldn't.  So in the
>> end I went back to the original commit:
>>
>>   commit d8bbae6ea080249c05ca90b1f8640fde48a18301
>>   Date:   Fri Jan 14 15:40:59 2022 -0500
>>   
>>       gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)
>>
>> And tried to understand what's going on.  The commit message on that
>> original commit is awesome, and well worth a read.
>>
>> After looking at that for a while I think I have a replacement for this
>> block, which I think achieves the same thing, but is structured, and
>> commented slightly differently.
>>
>> I've included an updated version of this patch below, I'd love to hear
>> what you think?
>>
>> The patch below is only partially tested at this point.  I'm a little
>> short of time right now.  But it passes all the vfork related tests that
>> I've been looking at, so I'm reasonably sure it's OK, I just wanted to
>> get this out so you could take a look at it.
>>
>> Thanks,
>> Andrew
>>
>> ---
>>
>> commit 45460db250f116956f4d75376f6637d2e4e7632f
>> Author: Mihails Strasuns <mihails.strasuns@intel.com>
>> Date:   Mon Jun 12 09:49:27 2023 +0200
>>
>>     gdb, infrun: refactor part of `proceed` into separate function
>>     
>>     Split the thread resuming code from proceed into new function
>>     proceed_resume_thread_checked.
>>     
>>     Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 58da1cef29e..7e6104ffab2 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -3268,6 +3268,90 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>>      }
>>  }
>>  
>> +/* Helper function for `proceed`.  Check if thread TP is suitable for
>> +   resuming, and, if it is, switch to the thread and call
>> +   `keep_going_pass_signal`.  If TP is not suitable for resuming then this
>> +   function will just return without switching threads.  */
>> +
>> +static void
>> +proceed_resume_thread_checked (thread_info *tp)
>> +{
>> +  if (!tp->inf->has_execution ())
>> +    {
>> +      infrun_debug_printf ("[%s] target has no execution",
>> +			   tp->ptid.to_string ().c_str ());
>> +      return;
>> +    }
>> +
>> +  if (tp->resumed ())
>> +    {
>> +      infrun_debug_printf ("[%s] resumed",
>> +			   tp->ptid.to_string ().c_str ());
>> +      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
>> +      return;
>> +    }
>> +
>> +  if (thread_is_in_step_over_chain (tp))
>> +    {
>> +      infrun_debug_printf ("[%s] needs step-over",
>> +			   tp->ptid.to_string ().c_str ());
>> +      return;
>> +    }
>> +
>> +  /* When handling a vfork GDB removes all breakpoints from the program
>> +     space in which the vfork is being handled, as such we must take care
>> +     not to resume any thread other than the vfork parent -- resuming the
>> +     vfork parent allows GDB to receive and handle the 'vfork done'
>> +     event.  */
>> +  if (tp->inf->thread_waiting_for_vfork_done != nullptr)
>> +    {
>> +      if (target_is_non_stop_p ())
>> +	{
>> +	  /* For non-stop targets, regardless of whether GDB is using
>> +	     all-stop or non-stop mode, threads are controlled
>> +	     individually.
>> +
>> +	     When a thread is handling a vfork, breakpoints are removed
>> +	     from the inferior (well, program space in fact), so it is
>> +	     critical that we don't try to resume any thread other than the
>> +	     vfork parent.  */
>> +	  if (tp != tp->inf->thread_waiting_for_vfork_done)
>> +	    {
>> +	      infrun_debug_printf ("[%s] another thread of this inferior is "
>> +				   "waiting for vfork-done",
>> +				   tp->ptid.to_string ().c_str ());
>> +	      return;
>> +	    }
>
> This actually fixes a bug that exists currently in GDB.  I'm working on
> a patch which includes a test that exposes the bug.  I plan to get the
> bug fix merged before we merge this refactoring commit.
>
>> +
>> +	  /* In non-stop mode we will never end up trying to proceed the
>> +	     thread that is handling a vfork.  When we realise that a
>> +	     thread is handling a vfork we immediately stop all the other
>> +	     threads, and resume the vfork parent thread.  As far as the
>> +	     user is concerned, the vfork parent thread is running.  Any
>> +	     attempt by the user to interrupt the vfork parent will be
>> +	     deferred until the vfork has completed, at which point the
>> +	     parent will no longer be waiting for a vfork.  */
>> +	  gdb_assert (!non_stop);
>
> And this assertion is not correct.  Again, the new test I'll add will
> trigger this assert.
>
> We can get this refactor merged once my bug fix has landed.  I'll follow
> up once I have something posted.

I finally got my vfork patches on the mailing list.  In the end it felt
more natural for this refactor to live part way through that series, so
that's what I've done.  You can see the series here:

https://inbox.sourceware.org/gdb-patches/cover.1687438786.git.aburgess@redhat.com/

Thanks,
Andrew

RE: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function

[Schimpe, Christina]

Hi Andrew, 

So sorry for my late response. Unfortunately, I could not have a look at it earlier and it also took me some time.

>> I tried to like this merged logic block, but I just couldn't.  So in the
>> end I went back to the original commit:
>>
>>   commit d8bbae6ea080249c05ca90b1f8640fde48a18301
>>   Date:   Fri Jan 14 15:40:59 2022 -0500
>>   
>>       gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)
>>
>> And tried to understand what's going on.  The commit message on that
>> original commit is awesome, and well worth a read.

I totally agree. I tested the latest version of your patch and it lgtm.

Thanks,

Christina

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Thursday, June 22, 2023 4:23 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Cc: blarsen@redhat.com; Simon Marchi <simark@simark.ca>
> Subject: Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into
> separate function
> 
> Andrew Burgess <aburgess@redhat.com> writes:
> 
> > Andrew Burgess <aburgess@redhat.com> writes:
> >
> >> Christina Schimpe <christina.schimpe@intel.com> writes:
> >>
> >>> From: Mihails Strasuns <mihails.strasuns@intel.com>
> >>>
> >>> Split thread resuming block into separate function.
> >>>
> >>> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
> >>> ---
> >>>  gdb/infrun.c | 120
> >>> ++++++++++++++++++++++++++-------------------------
> >>>  1 file changed, 61 insertions(+), 59 deletions(-)
> >>>
> >>> diff --git a/gdb/infrun.c b/gdb/infrun.c index
> >>> 58da1cef29e..ec41b658864 100644
> >>> --- a/gdb/infrun.c
> >>> +++ b/gdb/infrun.c
> >>> @@ -3268,6 +3268,64 @@ check_multi_target_resumption
> (process_stratum_target *resume_target)
> >>>      }
> >>>  }
> >>>
> >>> +  /* Helper function for `proceed`.  Check if thread TP is suitable for
> >>> +     resuming, and, if it is, switch to the thread and call
> >>> +     `keep_going_pass_signal`.  If TP is not suitable for resuming then
> >>> +     this function will just return without switching threads.  */
> >>
> >> The indentation here is wrong -- header comments should start in the
> >> first column
> >>
> >>> +
> >>> +static void
> >>> +proceed_resume_thread_checked (thread_info *tp) {
> >>> +  if (!tp->inf->has_execution ())
> >>> +    {
> >>> +      infrun_debug_printf ("[%s] target has no execution",
> >>> +			   tp->ptid.to_string ().c_str ());
> >>> +      return;
> >>> +    }
> >>> +
> >>> +  if (tp->resumed ())
> >>> +    {
> >>> +      infrun_debug_printf ("[%s] resumed",
> >>> +			   tp->ptid.to_string ().c_str ());
> >>> +      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
> >>> +      return;
> >>> +    }
> >>> +
> >>> +  if (thread_is_in_step_over_chain (tp))
> >>> +    {
> >>> +      infrun_debug_printf ("[%s] needs step-over",
> >>> +			   tp->ptid.to_string ().c_str ());
> >>> +      return;
> >>> +    }
> >>> +
> >>> +  /* There are further two scenarios for which we must not resume the
> thread:
> >>> +     - If a thread of that inferior is waiting for a vfork-done
> >>> +       (for a detached vfork child to exec or exit), breakpoints are
> >>> +       removed.  We must not resume any thread of that inferior, other
> >>> +       than the one waiting for the vfork-done.
> >>> +     - In non-stop, forbid resuming a thread if some other thread of
> >>> +       that inferior is waiting for a vfork-done event (this means
> >>> +       breakpoints are out for this inferior).  */
> >>> +  if (tp->inf->thread_waiting_for_vfork_done != nullptr
> >>> +      && (tp != tp->inf->thread_waiting_for_vfork_done
> >>> +	  || non_stop))
> >>> +    {
> >>> +      infrun_debug_printf ("[%s] another thread of this inferior is "
> >>> +			   "waiting for vfork-done",
> >>> +			   tp->ptid.to_string ().c_str ());
> >>> +      return;
> >>> +    }
> >>
> >> I tried to like this merged logic block, but I just couldn't.  So in
> >> the end I went back to the original commit:
> >>
> >>   commit d8bbae6ea080249c05ca90b1f8640fde48a18301
> >>   Date:   Fri Jan 14 15:40:59 2022 -0500
> >>
> >>       gdb: fix handling of vfork by multi-threaded program
> >> (follow-fork-mode=parent, detach-on-fork=on)
> >>
> >> And tried to understand what's going on.  The commit message on that
> >> original commit is awesome, and well worth a read.
> >>
> >> After looking at that for a while I think I have a replacement for
> >> this block, which I think achieves the same thing, but is structured,
> >> and commented slightly differently.
> >>
> >> I've included an updated version of this patch below, I'd love to
> >> hear what you think?
> >>
> >> The patch below is only partially tested at this point.  I'm a little
> >> short of time right now.  But it passes all the vfork related tests
> >> that I've been looking at, so I'm reasonably sure it's OK, I just
> >> wanted to get this out so you could take a look at it.
> >>
> >> Thanks,
> >> Andrew
> >>
> >> ---
> >>
> >> commit 45460db250f116956f4d75376f6637d2e4e7632f
> >> Author: Mihails Strasuns <mihails.strasuns@intel.com>
> >> Date:   Mon Jun 12 09:49:27 2023 +0200
> >>
> >>     gdb, infrun: refactor part of `proceed` into separate function
> >>
> >>     Split the thread resuming code from proceed into new function
> >>     proceed_resume_thread_checked.
> >>
> >>     Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com>
> >>
> >> diff --git a/gdb/infrun.c b/gdb/infrun.c index
> >> 58da1cef29e..7e6104ffab2 100644
> >> --- a/gdb/infrun.c
> >> +++ b/gdb/infrun.c
> >> @@ -3268,6 +3268,90 @@ check_multi_target_resumption
> (process_stratum_target *resume_target)
> >>      }
> >>  }
> >>
> >> +/* Helper function for `proceed`.  Check if thread TP is suitable for
> >> +   resuming, and, if it is, switch to the thread and call
> >> +   `keep_going_pass_signal`.  If TP is not suitable for resuming then this
> >> +   function will just return without switching threads.  */
> >> +
> >> +static void
> >> +proceed_resume_thread_checked (thread_info *tp) {
> >> +  if (!tp->inf->has_execution ())
> >> +    {
> >> +      infrun_debug_printf ("[%s] target has no execution",
> >> +			   tp->ptid.to_string ().c_str ());
> >> +      return;
> >> +    }
> >> +
> >> +  if (tp->resumed ())
> >> +    {
> >> +      infrun_debug_printf ("[%s] resumed",
> >> +			   tp->ptid.to_string ().c_str ());
> >> +      gdb_assert (tp->executing () || tp->has_pending_waitstatus ());
> >> +      return;
> >> +    }
> >> +
> >> +  if (thread_is_in_step_over_chain (tp))
> >> +    {
> >> +      infrun_debug_printf ("[%s] needs step-over",
> >> +			   tp->ptid.to_string ().c_str ());
> >> +      return;
> >> +    }
> >> +
> >> +  /* When handling a vfork GDB removes all breakpoints from the program
> >> +     space in which the vfork is being handled, as such we must take care
> >> +     not to resume any thread other than the vfork parent -- resuming the
> >> +     vfork parent allows GDB to receive and handle the 'vfork done'
> >> +     event.  */
> >> +  if (tp->inf->thread_waiting_for_vfork_done != nullptr)
> >> +    {
> >> +      if (target_is_non_stop_p ())
> >> +	{
> >> +	  /* For non-stop targets, regardless of whether GDB is using
> >> +	     all-stop or non-stop mode, threads are controlled
> >> +	     individually.
> >> +
> >> +	     When a thread is handling a vfork, breakpoints are removed
> >> +	     from the inferior (well, program space in fact), so it is
> >> +	     critical that we don't try to resume any thread other than the
> >> +	     vfork parent.  */
> >> +	  if (tp != tp->inf->thread_waiting_for_vfork_done)
> >> +	    {
> >> +	      infrun_debug_printf ("[%s] another thread of this inferior is "
> >> +				   "waiting for vfork-done",
> >> +				   tp->ptid.to_string ().c_str ());
> >> +	      return;
> >> +	    }
> >
> > This actually fixes a bug that exists currently in GDB.  I'm working
> > on a patch which includes a test that exposes the bug.  I plan to get
> > the bug fix merged before we merge this refactoring commit.
> >
> >> +
> >> +	  /* In non-stop mode we will never end up trying to proceed the
> >> +	     thread that is handling a vfork.  When we realise that a
> >> +	     thread is handling a vfork we immediately stop all the other
> >> +	     threads, and resume the vfork parent thread.  As far as the
> >> +	     user is concerned, the vfork parent thread is running.  Any
> >> +	     attempt by the user to interrupt the vfork parent will be
> >> +	     deferred until the vfork has completed, at which point the
> >> +	     parent will no longer be waiting for a vfork.  */
> >> +	  gdb_assert (!non_stop);
> >
> > And this assertion is not correct.  Again, the new test I'll add will
> > trigger this assert.
> >
> > We can get this refactor merged once my bug fix has landed.  I'll
> > follow up once I have something posted.
> 
> I finally got my vfork patches on the mailing list.  In the end it felt more natural
> for this refactor to live part way through that series, so that's what I've done.
> You can see the series here:
> 
> https://inbox.sourceware.org/gdb-
> patches/cover.1687438786.git.aburgess@redhat.com/
> 
> Thanks,
> Andrew

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