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

[Bruno Larsen <blarsen@redhat.com>]

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>

-- 
Cheers,
Bruno