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

[Bruno Larsen]

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

-- 
Cheers,
Bruno

> Thanks,
> Andrew