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

["Schimpe, Christina" <christina.schimpe@intel.com>]

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