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

[Andrew Burgess <aburgess@redhat.com>]

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