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

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

Split thread resuming block into separate function.
---
 gdb/infrun.c | 119 ++++++++++++++++++++++++++-------------------------
 1 file changed, 60 insertions(+), 59 deletions(-)
  

On 07/06/2023 09:09, Christina Schimpe via Gdb-patches wrote:
> From: Mihails Strasuns <mihails.strasuns@intel.com>
>
> Split thread resuming block into separate function.

Hi Christina, thanks for picking this one up. Unrelated to the changes, 
I think your email client got some sort of hiccup, since patch 0 isn't 
shown as related to this one. Not sure what you did different from your 
previous patches, since the other ones were fine, but just thought I 
would mention it :)

I also have one comment on the patch, inlined.

> ---
>   gdb/infrun.c | 119 ++++++++++++++++++++++++++-------------------------
>   1 file changed, 60 insertions(+), 59 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 58da1cef29e..571cf29ef32 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3268,6 +3268,63 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>       }
>   }
>   
> +/* Helper function for `proceed`, does bunch of checks to see
> +   if a thread can be resumed right now, switches to that thread
> +   and moves on to `keep_going_pass_signal`.  */
> +
> +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;
> +    }
> +
> +  /* 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))

I'm not sure this if statement is entirely correct. Let me explain how I 
understood it, and correct me if I am wrong anywhere, please.

That statement seems to be a mix between the one on line 3486 and the 
one on line 3505, first one being:

(tp->inf->thread_waiting_for_vfork_done != nullptr && tp != 
tp->inf_thread_waiting_for_vfork_done)

And the second is

(!tp->resumed () && !thread_is_in_step_over_chain (tp) && !(non_stop && 
tp->inf->thread_waiting_for_vfork_done != nullptr))

To save my sanity, I'll shorten them to a single letter. So my 
understanding is that that condition triggers on:

(A && B) || (C && D && !(E && A))

The new condition, on the other hand, is (A && (B || E)), which expands 
to (A && B) || (!(A + E)).  The first half is correct, but the second 
one is nowhere near the second part. Along with that, there are multiple 
early exits that I don't understand the code well enough to know if they 
could be triggered in the else call.

All this is to say, I think the final else if in the original function 
should not be changed, and this if should be simplified to the original 
condition.

If you would still like to avoid code repetition, I think the best is 
taking the lines that set a thread running into its own function, but 
that is up to you.

I hope this makes sense... and if I am mistaken, please explain it to me :-)
  

Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 07/06/2023 09:09, Christina Schimpe via Gdb-patches wrote:
>> From: Mihails Strasuns <mihails.strasuns@intel.com>
>>
>> Split thread resuming block into separate function.
>
> Hi Christina, thanks for picking this one up. Unrelated to the changes, 
> I think your email client got some sort of hiccup, since patch 0 isn't 
> shown as related to this one. Not sure what you did different from your 
> previous patches, since the other ones were fine, but just thought I 
> would mention it :)
>
> I also have one comment on the patch, inlined.
>
>> ---
>>   gdb/infrun.c | 119 ++++++++++++++++++++++++++-------------------------
>>   1 file changed, 60 insertions(+), 59 deletions(-)
>>
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 58da1cef29e..571cf29ef32 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -3268,6 +3268,63 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>>       }
>>   }
>>   
>> +/* Helper function for `proceed`, does bunch of checks to see
>> +   if a thread can be resumed right now, switches to that thread
>> +   and moves on to `keep_going_pass_signal`.  */
>> +
>> +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;
>> +    }
>> +
>> +  /* 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))
>
> I'm not sure this if statement is entirely correct. Let me explain how I 
> understood it, and correct me if I am wrong anywhere, please.
>
> That statement seems to be a mix between the one on line 3486 and the 
> one on line 3505, first one being:
>
> (tp->inf->thread_waiting_for_vfork_done != nullptr && tp != 
> tp->inf_thread_waiting_for_vfork_done)
>
> And the second is
>
> (!tp->resumed () && !thread_is_in_step_over_chain (tp) && !(non_stop && 
> tp->inf->thread_waiting_for_vfork_done != nullptr))
>
> To save my sanity, I'll shorten them to a single letter. So my 
> understanding is that that condition triggers on:
>
> (A && B) || (C && D && !(E && A))
>
> The new condition, on the other hand, is (A && (B || E)), which expands 
> to (A && B) || (!(A + E)).  The first half is correct, but the second 
> one is nowhere near the second part. Along with that, there are multiple 
> early exits that I don't understand the code well enough to know if they 
> could be triggered in the else call.
>
> All this is to say, I think the final else if in the original function 
> should not be changed, and this if should be simplified to the original 
> condition.

I disagree.

If you check the conditions for the early exits you'll notice that these
correspond (mostly) with the conditions that you are worried are
missing.

So the second 'else if' before this patch is:

    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))

Afterwards we call 'proceed_resume_thread_checked', but exit early if:

   cur_thr->resumed ()

or

   thread_is_in_step_over_chain (cur_thr)

So 'proceed_resume_thread_checked' will only do anything if both those
conditions are NOT true, which matches with our previous 'else if'
condition.

That just leaves the final part:

	     && !(non_stop
		  && cur_thr->inf->thread_waiting_for_vfork_done != nullptr))

this becomes another early exit with this condition:

  if (tp->inf->thread_waiting_for_vfork_done != nullptr
      && (tp != tp->inf->thread_waiting_for_vfork_done
	  || non_stop))

Previously the logic was: !(A && B)
Now the logic is: !(B && (C || A))
               => !((A || C) && B)

I've added the ! around the new logic because the condition as written
is for the early exit, so we only _do_ something when the early exit
condition is not true.

So, this leaves two questions:

  1. Is the addition of '|| C' (i.e. '|| tp !=
  tp->inf->thread_waiting_for_vfork_done') here important? And

  2. The new code has an extra early exit with the condition: 'if
  (!tp->inf->has_execution ())', is this important?

I don't know the answer to #1 for sure, but my guess is this is fine.
The logic in the comment explains it, we really shouldn't be trying to
resume some arbitrary thread in a program space that's had it's
breakpoints temporarily removed.  So if '|| tp !=
tp->inf->thread_waiting_for_vfork_done' does trigger then hopefully this
is a good thing.  My guess is this can't occur down this code path.

For #2 I don't see this as a problem.  This is just asking can this
thread actually be made to run at all.  If this isn't true then I don't
think anything good would happen from trying to actually set the thread
running.  Again, my guess would be that this can never be false along
this code path, but having the check should be harmless.

Hopefully I've not made a mistake in my analysis here...

Thanks,
Andrew

>
> If you would still like to avoid code repetition, I think the best is 
> taking the lines that set a thread running into its own function, but 
> that is up to you.
>
> I hope this makes sense... and if I am mistaken, please explain it to me :-)
>
> -- 
> Cheers,
> Bruno
>
>> +    {
>> +      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 +3513,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 ();
>>     }
  

Christina Schimpe via Gdb-patches <gdb-patches@sourceware.org> writes:

> From: Mihails Strasuns <mihails.strasuns@intel.com>
>
> Split thread resuming block into separate function.
> ---
>  gdb/infrun.c | 119 ++++++++++++++++++++++++++-------------------------
>  1 file changed, 60 insertions(+), 59 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 58da1cef29e..571cf29ef32 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3268,6 +3268,63 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>      }
>  }
>  
> +/* Helper function for `proceed`, does bunch of checks to see
> +   if a thread can be resumed right now, switches to that thread
> +   and moves on to `keep_going_pass_signal`.  */

Function comments are suppose to mention the function arguments.  So I'd
prefer something like:

  /* 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;
> +    }
> +
> +  /* 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).  */

It feels like there's duplication between the two parts of this comment
now.  I think you need to rewrite this as a single comment block that
explains the new merged logic here.

Thanks,
Andrew

> +
> +  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 +3513,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
  

On 07/06/2023 16:21, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> On 07/06/2023 09:09, Christina Schimpe via Gdb-patches wrote:
>>> From: Mihails Strasuns <mihails.strasuns@intel.com>
>>>
>>> Split thread resuming block into separate function.
>> Hi Christina, thanks for picking this one up. Unrelated to the changes,
>> I think your email client got some sort of hiccup, since patch 0 isn't
>> shown as related to this one. Not sure what you did different from your
>> previous patches, since the other ones were fine, but just thought I
>> would mention it :)
>>
>> I also have one comment on the patch, inlined.
>>
>>> ---
>>>    gdb/infrun.c | 119 ++++++++++++++++++++++++++-------------------------
>>>    1 file changed, 60 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>> index 58da1cef29e..571cf29ef32 100644
>>> --- a/gdb/infrun.c
>>> +++ b/gdb/infrun.c
>>> @@ -3268,6 +3268,63 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>>>        }
>>>    }
>>>    
>>> +/* Helper function for `proceed`, does bunch of checks to see
>>> +   if a thread can be resumed right now, switches to that thread
>>> +   and moves on to `keep_going_pass_signal`.  */
>>> +
>>> +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;
>>> +    }
>>> +
>>> +  /* 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))
>> I'm not sure this if statement is entirely correct. Let me explain how I
>> understood it, and correct me if I am wrong anywhere, please.
>>
>> That statement seems to be a mix between the one on line 3486 and the
>> one on line 3505, first one being:
>>
>> (tp->inf->thread_waiting_for_vfork_done != nullptr && tp !=
>> tp->inf_thread_waiting_for_vfork_done)
>>
>> And the second is
>>
>> (!tp->resumed () && !thread_is_in_step_over_chain (tp) && !(non_stop &&
>> tp->inf->thread_waiting_for_vfork_done != nullptr))
>>
>> To save my sanity, I'll shorten them to a single letter. So my
>> understanding is that that condition triggers on:
>>
>> (A && B) || (C && D && !(E && A))
>>
>> The new condition, on the other hand, is (A && (B || E)), which expands
>> to (A && B) || (!(A + E)).  The first half is correct, but the second
>> one is nowhere near the second part. Along with that, there are multiple
>> early exits that I don't understand the code well enough to know if they
>> could be triggered in the else call.
>>
>> All this is to say, I think the final else if in the original function
>> should not be changed, and this if should be simplified to the original
>> condition.
> I disagree.
>
> If you check the conditions for the early exits you'll notice that these
> correspond (mostly) with the conditions that you are worried are
> missing.
Ah yes, you're right. I guess I should have been more careful when 
reading the whole change.
>
> So the second 'else if' before this patch is:
>
>      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))
>
> Afterwards we call 'proceed_resume_thread_checked', but exit early if:
>
>     cur_thr->resumed ()
>
> or
>
>     thread_is_in_step_over_chain (cur_thr)
>
> So 'proceed_resume_thread_checked' will only do anything if both those
> conditions are NOT true, which matches with our previous 'else if'
> condition.
>
> That just leaves the final part:
>
> 	     && !(non_stop
> 		  && cur_thr->inf->thread_waiting_for_vfork_done != nullptr))
>
> this becomes another early exit with this condition:
>
>    if (tp->inf->thread_waiting_for_vfork_done != nullptr
>        && (tp != tp->inf->thread_waiting_for_vfork_done
> 	  || non_stop))
>
> Previously the logic was: !(A && B)
> Now the logic is: !(B && (C || A))
>                 => !((A || C) && B)
>
> I've added the ! around the new logic because the condition as written
> is for the early exit, so we only _do_ something when the early exit
> condition is not true.
Yeah, this checks out.
>
> So, this leaves two questions:
>
>    1. Is the addition of '|| C' (i.e. '|| tp !=
>    tp->inf->thread_waiting_for_vfork_done') here important? And
>
>    2. The new code has an extra early exit with the condition: 'if
>    (!tp->inf->has_execution ())', is this important?
>
> I don't know the answer to #1 for sure, but my guess is this is fine.
> The logic in the comment explains it, we really shouldn't be trying to
> resume some arbitrary thread in a program space that's had it's
> breakpoints temporarily removed.  So if '|| tp !=
> tp->inf->thread_waiting_for_vfork_done' does trigger then hopefully this
> is a good thing.  My guess is this can't occur down this code path.
After about 2 hours' worth of boolean logic, I think there might be 
something to look into here. Currently, we allow the inferior to proceed 
if (!non_stop && !target_is_non_stop_p () && tp != 
tp->inf->thread_waiting_for_vfork_done) whereas the new code doesn't 
allow for this case. I don't know enough of GDB and non_stop mode to 
know if this is a possible case, and if removing it is good or not, so 
I'll defer to you on this one.
>
> For #2 I don't see this as a problem.  This is just asking can this
> thread actually be made to run at all.  If this isn't true then I don't
> think anything good would happen from trying to actually set the thread
> running.  Again, my guess would be that this can never be false along
> this code path, but having the check should be harmless.

Agreed.

>
> Hopefully I've not made a mistake in my analysis here...

Quick glossary:

A => non_stop
B => target_is_non_stop_p ()
C => tp->inf->thread_waiting_for_vfork_done != nullptr
D => tp != tp->inf->thread_waiting_for_vfork_done
E => tp->resume ()
F => thread_is_in_step_over_chain
G => tp->inf->has_execution ()
H => (!A & B)
asterisk => AND operation
plus => OR operation

Current code has 2 possible flows. I'm calling flow 1 the one that goes 
through the "for" loop, and flow 2 the other one (its also in order in 
the file). The logic equations that resume in each flow are, respectively:

* (!A * B) * G * !E * !F * !(C * D) [1]
* !(!A * B) * !E * !F * !(A * C)    [2]// and you mentioned that adding 
& G to this equation is harmless, if not beneficial, so I'll add it from 
now on

The new version technically also has 2 control flows, but since it is H 
& (condition) | !H & condition, we can factor H out and get a simplified 
expression:

* G * !E * !F * !(C * (D + A))    [3]

Since both options have G & !E & !F, I'll ignore them, it won't change 
the fact that they're equal or not. Equation 3 can be rewritten as

* !C + (!D * !A)    [4]

Equation 4 is only useful at the end. Back to current code, we proceed 
when 1 or 2 is true, giving us:

(!A * B) * !(C * D) + !(!A * B) * !(A * C) =>
!A * B * (!C + !D) + (A + !B) * (!A + !C) =>
!A*B*!C + !A*B*!D + A*!C + !A*!B + !B*!C; using the rule that !B*!C = 
A*!B*!C + !A*!B*!C we have =>
!A*B*!C + !A*B*!D + !A*!B*!C + !A*!B + A*!C + A*!B*!C =>
!A*(B*!C + !B*!C + B*!D + !B) + A*(!C + !B*!C); using the rule that B*!C 
+ !B*!C = !C =>
!A*(!C + B*!D + !B) + A*!C =>
!A*(B*!D + !B) + !A*!C + A*!C =>
!A*(B*!D + !B*!D + !B*D) + !C =>
!A*(!D + !B*D) + !C =>
!C + !A*!D + !A*!B*D [5]

So, current code has one more term in the final boolean expression when 
compared to the new code. That term corresponds to continuing the 
inferior if !non_stop && !target_is_non_stop_p() && tp != 
tp->inf->thread_waiting_for_vfork_done
  

Hi Andrew, 

Thanks a lot for your feedback. 

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Wednesday, June 7, 2023 4:26 PM
> To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, infrun: refactor part of `proceed` into separate
> function
> 
> Christina Schimpe via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > From: Mihails Strasuns <mihails.strasuns@intel.com>
> >
> > Split thread resuming block into separate function.
> > ---
> >  gdb/infrun.c | 119
> > ++++++++++++++++++++++++++-------------------------
> >  1 file changed, 60 insertions(+), 59 deletions(-)
> >
> > diff --git a/gdb/infrun.c b/gdb/infrun.c index
> > 58da1cef29e..571cf29ef32 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -3268,6 +3268,63 @@ check_multi_target_resumption
> (process_stratum_target *resume_target)
> >      }
> >  }
> >
> > +/* Helper function for `proceed`, does bunch of checks to see
> > +   if a thread can be resumed right now, switches to that thread
> > +   and moves on to `keep_going_pass_signal`.  */
> 
> Function comments are suppose to mention the function arguments.  So I'd
> prefer something like:
> 
>   /* 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;
> > +    }
> > +
> > +  /* 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).  */
> 
> It feels like there's duplication between the two parts of this comment now.
> I think you need to rewrite this as a single comment block that explains the
> new merged logic here.
> 

I agree with all your comments and will post a v2 asap.

Christina
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
  

Hi Bruno and Andrew, 

Thanks a lot for your investigations!

> > So, this leaves two questions:
> >
> >    1. Is the addition of '|| C' (i.e. '|| tp !=
> >    tp->inf->thread_waiting_for_vfork_done') here important? And
> >
> >    2. The new code has an extra early exit with the condition: 'if
> >    (!tp->inf->has_execution ())', is this important?
> >
> > I don't know the answer to #1 for sure, but my guess is this is fine.
> > The logic in the comment explains it, we really shouldn't be trying to
> > resume some arbitrary thread in a program space that's had it's
> > breakpoints temporarily removed.  So if '|| tp !=
> > tp->inf->thread_waiting_for_vfork_done' does trigger then hopefully
> > tp->inf->this
> > is a good thing.  My guess is this can't occur down this code path.
> After about 2 hours' worth of boolean logic, I think there might be something
> to look into here. Currently, we allow the inferior to proceed if (!non_stop
> && !target_is_non_stop_p () && tp !=
> tp->inf->thread_waiting_for_vfork_done) whereas the new code doesn't
> allow for this case. I don't know enough of GDB and non_stop mode to know
> if this is a possible case, and if removing it is good or not, so I'll defer to you
> on this one.
> >
> > For #2 I don't see this as a problem.  This is just asking can this
> > thread actually be made to run at all.  If this isn't true then I
> > don't think anything good would happen from trying to actually set the
> > thread running.  Again, my guess would be that this can never be false
> > along this code path, but having the check should be harmless.
> 
> Agreed.
> 
> >
> > Hopefully I've not made a mistake in my analysis here...
> 
> Quick glossary:
> 
> A => non_stop
> B => target_is_non_stop_p ()
> C => tp->inf->thread_waiting_for_vfork_done != nullptr D => tp != tp->inf-
> >thread_waiting_for_vfork_done
> E => tp->resume ()
> F => thread_is_in_step_over_chain
> G => tp->inf->has_execution ()
> H => (!A & B)
> asterisk => AND operation
> plus => OR operation
> 
> Current code has 2 possible flows. I'm calling flow 1 the one that goes
> through the "for" loop, and flow 2 the other one (its also in order in the file).
> The logic equations that resume in each flow are, respectively:
> 
> * (!A * B) * G * !E * !F * !(C * D) [1]
> * !(!A * B) * !E * !F * !(A * C)    [2]// and you mentioned that adding & G to
> this equation is harmless, if not beneficial, so I'll add it from now on
> 
> The new version technically also has 2 control flows, but since it is H &
> (condition) | !H & condition, we can factor H out and get a simplified
> expression:
> 
> * G * !E * !F * !(C * (D + A))    [3]
> 
> Since both options have G & !E & !F, I'll ignore them, it won't change the fact
> that they're equal or not. Equation 3 can be rewritten as
> 
> * !C + (!D * !A)    [4]
> 
> Equation 4 is only useful at the end. Back to current code, we proceed when
> 1 or 2 is true, giving us:
> 
> (!A * B) * !(C * D) + !(!A * B) * !(A * C) => !A * B * (!C + !D) + (A + !B) * (!A +
> !C) => !A*B*!C + !A*B*!D + A*!C + !A*!B + !B*!C; using the rule that !B*!C =
> A*!B*!C + !A*!B*!C we have => !A*B*!C + !A*B*!D + !A*!B*!C + !A*!B +
> A*!C + A*!B*!C => !A*(B*!C + !B*!C + B*!D + !B) + A*(!C + !B*!C); using the
> rule that B*!C
> + !B*!C = !C =>
> !A*(!C + B*!D + !B) + A*!C =>
> !A*(B*!D + !B) + !A*!C + A*!C =>
> !A*(B*!D + !B*!D + !B*D) + !C =>
> !A*(!D + !B*D) + !C =>
> !C + !A*!D + !A*!B*D [5]
> 
> So, current code has one more term in the final boolean expression when
> compared to the new code. That term corresponds to continuing the inferior
> if !non_stop && !target_is_non_stop_p() && tp !=
> tp->inf->thread_waiting_for_vfork_done

I looked at both flows and got to the same conclusion as Andrew.

For the first flow:
The current code allows to resume in case 
(!non_stop && target_is_non_stop_p ())  (1) 
&&( tp->inf->has_execution () && ! tp->resumed () && ! thread_is_in_step_over_chain (tp) (2) 
&& ! (tp->inf->thread_waiting_for_vfork_done != nullptr && tp != tp->inf->thread_waiting_for_vfork_done) (3)

The new code has a similar logic but line (3) looks a bit different as we have the "|| non_stop" here.
(!non_stop && target_is_non_stop_p ()) (1)  
&& tp->inf->has_execution () && !(tp->resumed ()) && ! thread_is_in_step_over_chain (tp) (2)
&& !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && (tp != tp->inf->thread_waiting_for_vfork_done || non_stop)) (3)

If we ever enter this branch non_stop must be set to false else the condition (1) wouldn't be met. 
&& !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && (tp != tp->inf->thread_waiting_for_vfork_done || false)) (3)
This makes line  (3) only depend on tp != tp->inf->thread_waiting_for_vfork_done 
=> && !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && tp != tp->inf->thread_waiting_for_vfork_done)

Which is exactly the same as the current code for line (3).

For the second flow:

We currently resume in case 
!(non_stop && target_is_non_stop_p ()) (1)
&& !tp->resumed () && ! thread_is_in_step_over_chain (tp) (2)
&& !(non_stop && cur_thr->inf->thread_waiting_for_vfork_done != nullptr) (3)

With the patch the second path looks like:
!(non_stop && target_is_non_stop_p ()) (1)
&& tp->inf->has_execution () &&  !tp->resumed () && ! thread_is_in_step_over_chain (tp) (2)
&& !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && (tp != tp->inf->thread_waiting_for_vfork_done || non_stop)) (3)

Line (2) is different as before, but as Bruno already agreed with Andrew adding the check "tp->inf->has_execution ()" should not harm.
This leaves one change in line (3), as Andrew already mentioned, we now have the additional option for early exit in case 
"tp != tp->inf->thread_waiting_for_vfork_done" is true.
I am also not sure if this is fine or not, but if I got it correctly this should be the only thing which is different for code path 2
(except the new execution check in line 2 for which we already agreed it is fine).

So looking at Bruno's concern 
> So, current code has one more term in the final boolean expression when
> compared to the new code. That term corresponds to continuing the inferior
> if !non_stop && !target_is_non_stop_p() && tp !=
> tp->inf->thread_waiting_for_vfork_done

Do I understand correctly that you have another concern in addition to the option for early exit in case  "tp != tp->inf->thread_waiting_for_vfork_done" mentioned above?

Thanks again for looking into this.

Christina



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
  

Bruno Larsen <blarsen@redhat.com> writes:

> On 07/06/2023 16:21, Andrew Burgess wrote:
>> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>>> On 07/06/2023 09:09, Christina Schimpe via Gdb-patches wrote:
>>>> From: Mihails Strasuns <mihails.strasuns@intel.com>
>>>>
>>>> Split thread resuming block into separate function.
>>> Hi Christina, thanks for picking this one up. Unrelated to the changes,
>>> I think your email client got some sort of hiccup, since patch 0 isn't
>>> shown as related to this one. Not sure what you did different from your
>>> previous patches, since the other ones were fine, but just thought I
>>> would mention it :)
>>>
>>> I also have one comment on the patch, inlined.
>>>
>>>> ---
>>>>    gdb/infrun.c | 119 ++++++++++++++++++++++++++-------------------------
>>>>    1 file changed, 60 insertions(+), 59 deletions(-)
>>>>
>>>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>>>> index 58da1cef29e..571cf29ef32 100644
>>>> --- a/gdb/infrun.c
>>>> +++ b/gdb/infrun.c
>>>> @@ -3268,6 +3268,63 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>>>>        }
>>>>    }
>>>>    
>>>> +/* Helper function for `proceed`, does bunch of checks to see
>>>> +   if a thread can be resumed right now, switches to that thread
>>>> +   and moves on to `keep_going_pass_signal`.  */
>>>> +
>>>> +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;
>>>> +    }
>>>> +
>>>> +  /* 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))
>>> I'm not sure this if statement is entirely correct. Let me explain how I
>>> understood it, and correct me if I am wrong anywhere, please.
>>>
>>> That statement seems to be a mix between the one on line 3486 and the
>>> one on line 3505, first one being:
>>>
>>> (tp->inf->thread_waiting_for_vfork_done != nullptr && tp !=
>>> tp->inf_thread_waiting_for_vfork_done)
>>>
>>> And the second is
>>>
>>> (!tp->resumed () && !thread_is_in_step_over_chain (tp) && !(non_stop &&
>>> tp->inf->thread_waiting_for_vfork_done != nullptr))
>>>
>>> To save my sanity, I'll shorten them to a single letter. So my
>>> understanding is that that condition triggers on:
>>>
>>> (A && B) || (C && D && !(E && A))
>>>
>>> The new condition, on the other hand, is (A && (B || E)), which expands
>>> to (A && B) || (!(A + E)).  The first half is correct, but the second
>>> one is nowhere near the second part. Along with that, there are multiple
>>> early exits that I don't understand the code well enough to know if they
>>> could be triggered in the else call.
>>>
>>> All this is to say, I think the final else if in the original function
>>> should not be changed, and this if should be simplified to the original
>>> condition.
>> I disagree.
>>
>> If you check the conditions for the early exits you'll notice that these
>> correspond (mostly) with the conditions that you are worried are
>> missing.
> Ah yes, you're right. I guess I should have been more careful when 
> reading the whole change.
>>
>> So the second 'else if' before this patch is:
>>
>>      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))
>>
>> Afterwards we call 'proceed_resume_thread_checked', but exit early if:
>>
>>     cur_thr->resumed ()
>>
>> or
>>
>>     thread_is_in_step_over_chain (cur_thr)
>>
>> So 'proceed_resume_thread_checked' will only do anything if both those
>> conditions are NOT true, which matches with our previous 'else if'
>> condition.
>>
>> That just leaves the final part:
>>
>> 	     && !(non_stop
>> 		  && cur_thr->inf->thread_waiting_for_vfork_done != nullptr))
>>
>> this becomes another early exit with this condition:
>>
>>    if (tp->inf->thread_waiting_for_vfork_done != nullptr
>>        && (tp != tp->inf->thread_waiting_for_vfork_done
>> 	  || non_stop))
>>
>> Previously the logic was: !(A && B)
>> Now the logic is: !(B && (C || A))
>>                 => !((A || C) && B)
>>
>> I've added the ! around the new logic because the condition as written
>> is for the early exit, so we only _do_ something when the early exit
>> condition is not true.
> Yeah, this checks out.
>>
>> So, this leaves two questions:
>>
>>    1. Is the addition of '|| C' (i.e. '|| tp !=
>>    tp->inf->thread_waiting_for_vfork_done') here important? And
>>
>>    2. The new code has an extra early exit with the condition: 'if
>>    (!tp->inf->has_execution ())', is this important?
>>
>> I don't know the answer to #1 for sure, but my guess is this is fine.
>> The logic in the comment explains it, we really shouldn't be trying to
>> resume some arbitrary thread in a program space that's had it's
>> breakpoints temporarily removed.  So if '|| tp !=
>> tp->inf->thread_waiting_for_vfork_done' does trigger then hopefully this
>> is a good thing.  My guess is this can't occur down this code path.
> After about 2 hours' worth of boolean logic, I think there might be 
> something to look into here. Currently, we allow the inferior to proceed 
> if (!non_stop && !target_is_non_stop_p () && tp != 
> tp->inf->thread_waiting_for_vfork_done) whereas the new code doesn't 
> allow for this case. I don't know enough of GDB and non_stop mode to 
> know if this is a possible case, and if removing it is good or not, so 
> I'll defer to you on this one.

I'm slightly confused here: your conclusion is exactly what I pointed
out as item #1 in my list of questions.  So I agree with you 100% that
this is indeed a new condition.

As I said, I don't know for certain if this change is a problem or not,
but I don't think it should be.  The comment in the code already says:

  "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."

So if feels like in the current code, if we could resume some other
thread then this would be a problem.

So my (lets be honest) guess is that either it's impossible to try and
start some other thread in this condition, or if we could, then this
change will be fixing a latent bug.

Thanks,
Andrew

>>
>> For #2 I don't see this as a problem.  This is just asking can this
>> thread actually be made to run at all.  If this isn't true then I don't
>> think anything good would happen from trying to actually set the thread
>> running.  Again, my guess would be that this can never be false along
>> this code path, but having the check should be harmless.
>
> Agreed.
>
>>
>> Hopefully I've not made a mistake in my analysis here...
>
> Quick glossary:
>
> A => non_stop
> B => target_is_non_stop_p ()
> C => tp->inf->thread_waiting_for_vfork_done != nullptr
> D => tp != tp->inf->thread_waiting_for_vfork_done
> E => tp->resume ()
> F => thread_is_in_step_over_chain
> G => tp->inf->has_execution ()
> H => (!A & B)
> asterisk => AND operation
> plus => OR operation
>
> Current code has 2 possible flows. I'm calling flow 1 the one that goes 
> through the "for" loop, and flow 2 the other one (its also in order in 
> the file). The logic equations that resume in each flow are, respectively:
>
> * (!A * B) * G * !E * !F * !(C * D) [1]
> * !(!A * B) * !E * !F * !(A * C)    [2]// and you mentioned that adding 
> & G to this equation is harmless, if not beneficial, so I'll add it from 
> now on
>
> The new version technically also has 2 control flows, but since it is H 
> & (condition) | !H & condition, we can factor H out and get a simplified 
> expression:
>
> * G * !E * !F * !(C * (D + A))    [3]
>
> Since both options have G & !E & !F, I'll ignore them, it won't change 
> the fact that they're equal or not. Equation 3 can be rewritten as
>
> * !C + (!D * !A)    [4]
>
> Equation 4 is only useful at the end. Back to current code, we proceed 
> when 1 or 2 is true, giving us:
>
> (!A * B) * !(C * D) + !(!A * B) * !(A * C) =>
> !A * B * (!C + !D) + (A + !B) * (!A + !C) =>
> !A*B*!C + !A*B*!D + A*!C + !A*!B + !B*!C; using the rule that !B*!C = 
> A*!B*!C + !A*!B*!C we have =>
> !A*B*!C + !A*B*!D + !A*!B*!C + !A*!B + A*!C + A*!B*!C =>
> !A*(B*!C + !B*!C + B*!D + !B) + A*(!C + !B*!C); using the rule that B*!C 
> + !B*!C = !C =>
> !A*(!C + B*!D + !B) + A*!C =>
> !A*(B*!D + !B) + !A*!C + A*!C =>
> !A*(B*!D + !B*!D + !B*D) + !C =>
> !A*(!D + !B*D) + !C =>
> !C + !A*!D + !A*!B*D [5]

I'm having flash-backs to a university logic course :-/

Thanks,
Andrew


>
> So, current code has one more term in the final boolean expression when 
> compared to the new code. That term corresponds to continuing the 
> inferior if !non_stop && !target_is_non_stop_p() && tp != 
> tp->inf->thread_waiting_for_vfork_done
>
> -- 
> Cheers,
> Bruno
>
>> Thanks,
>> Andrew
  

On 08/06/2023 17:32, Schimpe, Christina wrote:
> Hi Bruno and Andrew,
>
> Thanks a lot for your investigations!
>
>>> So, this leaves two questions:
>>>
>>>     1. Is the addition of '|| C' (i.e. '|| tp !=
>>>     tp->inf->thread_waiting_for_vfork_done') here important? And
>>>
>>>     2. The new code has an extra early exit with the condition: 'if
>>>     (!tp->inf->has_execution ())', is this important?
>>>
>>> I don't know the answer to #1 for sure, but my guess is this is fine.
>>> The logic in the comment explains it, we really shouldn't be trying to
>>> resume some arbitrary thread in a program space that's had it's
>>> breakpoints temporarily removed.  So if '|| tp !=
>>> tp->inf->thread_waiting_for_vfork_done' does trigger then hopefully
>>> tp->inf->this
>>> is a good thing.  My guess is this can't occur down this code path.
>> After about 2 hours' worth of boolean logic, I think there might be something
>> to look into here. Currently, we allow the inferior to proceed if (!non_stop
>> && !target_is_non_stop_p () && tp !=
>> tp->inf->thread_waiting_for_vfork_done) whereas the new code doesn't
>> allow for this case. I don't know enough of GDB and non_stop mode to know
>> if this is a possible case, and if removing it is good or not, so I'll defer to you
>> on this one.
>>> For #2 I don't see this as a problem.  This is just asking can this
>>> thread actually be made to run at all.  If this isn't true then I
>>> don't think anything good would happen from trying to actually set the
>>> thread running.  Again, my guess would be that this can never be false
>>> along this code path, but having the check should be harmless.
>> Agreed.
>>
>>> Hopefully I've not made a mistake in my analysis here...
>> Quick glossary:
>>
>> A => non_stop
>> B => target_is_non_stop_p ()
>> C => tp->inf->thread_waiting_for_vfork_done != nullptr D => tp != tp->inf-
>>> thread_waiting_for_vfork_done
>> E => tp->resume ()
>> F => thread_is_in_step_over_chain
>> G => tp->inf->has_execution ()
>> H => (!A & B)
>> asterisk => AND operation
>> plus => OR operation
>>
>> Current code has 2 possible flows. I'm calling flow 1 the one that goes
>> through the "for" loop, and flow 2 the other one (its also in order in the file).
>> The logic equations that resume in each flow are, respectively:
>>
>> * (!A * B) * G * !E * !F * !(C * D) [1]
>> * !(!A * B) * !E * !F * !(A * C)    [2]// and you mentioned that adding & G to
>> this equation is harmless, if not beneficial, so I'll add it from now on
>>
>> The new version technically also has 2 control flows, but since it is H &
>> (condition) | !H & condition, we can factor H out and get a simplified
>> expression:
>>
>> * G * !E * !F * !(C * (D + A))    [3]
>>
>> Since both options have G & !E & !F, I'll ignore them, it won't change the fact
>> that they're equal or not. Equation 3 can be rewritten as
>>
>> * !C + (!D * !A)    [4]
>>
>> Equation 4 is only useful at the end. Back to current code, we proceed when
>> 1 or 2 is true, giving us:
>>
>> (!A * B) * !(C * D) + !(!A * B) * !(A * C) => !A * B * (!C + !D) + (A + !B) * (!A +
>> !C) => !A*B*!C + !A*B*!D + A*!C + !A*!B + !B*!C; using the rule that !B*!C =
>> A*!B*!C + !A*!B*!C we have => !A*B*!C + !A*B*!D + !A*!B*!C + !A*!B +
>> A*!C + A*!B*!C => !A*(B*!C + !B*!C + B*!D + !B) + A*(!C + !B*!C); using the
>> rule that B*!C
>> + !B*!C = !C =>
>> !A*(!C + B*!D + !B) + A*!C =>
>> !A*(B*!D + !B) + !A*!C + A*!C =>
>> !A*(B*!D + !B*!D + !B*D) + !C =>
>> !A*(!D + !B*D) + !C =>
>> !C + !A*!D + !A*!B*D [5]
>>
>> So, current code has one more term in the final boolean expression when
>> compared to the new code. That term corresponds to continuing the inferior
>> if !non_stop && !target_is_non_stop_p() && tp !=
>> tp->inf->thread_waiting_for_vfork_done
> I looked at both flows and got to the same conclusion as Andrew.
>
> For the first flow:
> The current code allows to resume in case
> (!non_stop && target_is_non_stop_p ())  (1)
> &&( tp->inf->has_execution () && ! tp->resumed () && ! thread_is_in_step_over_chain (tp) (2)
> && ! (tp->inf->thread_waiting_for_vfork_done != nullptr && tp != tp->inf->thread_waiting_for_vfork_done) (3)
>
> The new code has a similar logic but line (3) looks a bit different as we have the "|| non_stop" here.
> (!non_stop && target_is_non_stop_p ()) (1)
> && tp->inf->has_execution () && !(tp->resumed ()) && ! thread_is_in_step_over_chain (tp) (2)
> && !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && (tp != tp->inf->thread_waiting_for_vfork_done || non_stop)) (3)
>
> If we ever enter this branch non_stop must be set to false else the condition (1) wouldn't be met.
> && !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && (tp != tp->inf->thread_waiting_for_vfork_done || false)) (3)
> This makes line  (3) only depend on tp != tp->inf->thread_waiting_for_vfork_done
> => && !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && tp != tp->inf->thread_waiting_for_vfork_done)
>
> Which is exactly the same as the current code for line (3).
>
> For the second flow:
>
> We currently resume in case
> !(non_stop && target_is_non_stop_p ()) (1)
> && !tp->resumed () && ! thread_is_in_step_over_chain (tp) (2)
> && !(non_stop && cur_thr->inf->thread_waiting_for_vfork_done != nullptr) (3)
>
> With the patch the second path looks like:
> !(non_stop && target_is_non_stop_p ()) (1)
> && tp->inf->has_execution () &&  !tp->resumed () && ! thread_is_in_step_over_chain (tp) (2)
> && !  (tp->inf->thread_waiting_for_vfork_done != nullptr  && (tp != tp->inf->thread_waiting_for_vfork_done || non_stop)) (3)
>
> Line (2) is different as before, but as Bruno already agreed with Andrew adding the check "tp->inf->has_execution ()" should not harm.
> This leaves one change in line (3), as Andrew already mentioned, we now have the additional option for early exit in case
> "tp != tp->inf->thread_waiting_for_vfork_done" is true.
> I am also not sure if this is fine or not, but if I got it correctly this should be the only thing which is different for code path 2
> (except the new execution check in line 2 for which we already agreed it is fine).
>
> So looking at Bruno's concern
>> So, current code has one more term in the final boolean expression when
>> compared to the new code. That term corresponds to continuing the inferior
>> if !non_stop && !target_is_non_stop_p() && tp !=
>> tp->inf->thread_waiting_for_vfork_done
> Do I understand correctly that you have another concern in addition to the option for early exit in case  "tp != tp->inf->thread_waiting_for_vfork_done" mentioned above?
>
> Thanks again for looking into this.
>
That was actually my only concern. Since we all agree on the analysis 
and everyone guesses that this is ok, I'm cool with the change:
Reviewed-By: Bruno Larsen <blarsen@redhat.com>