From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id EE5A5385700D for ; Thu, 8 Jun 2023 18:41:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EE5A5385700D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1686249716; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=yC1GY8HuTSZYhKsHLFGP3pjKmCciYcCS5moM7yPg1aE=; b=S41Jv1IENwe8vp/ZaygxH3NV+JwpOg8qUfJP4as/kq2r7RSOeYEEdebGquSTdYxOEQZUjq Wp290e9vIaW7paXDmYQicBM9SUPjsH4qqkXV93Mt9UyhLYcLJBayvo8H0Q61XJk7rlxI3e iSSdSX+gYDVqsmQoeO15L7WYxDBY6Vg= Received: from mail-lf1-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-507-eSfdAFjXPPGtuxLCuAs_rA-1; Thu, 08 Jun 2023 14:41:55 -0400 X-MC-Unique: eSfdAFjXPPGtuxLCuAs_rA-1 Received: by mail-lf1-f72.google.com with SMTP id 2adb3069b0e04-4f62d8e527bso534389e87.1 for ; Thu, 08 Jun 2023 11:41:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686249713; x=1688841713; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=7LaMkbtKE2eJ698L7n/9nmB3ZDE8rHkGxOGSLnKDvi4=; b=k/AkNic37EGc1uQ8WrJ/ElWnk1JLh7jxrUsGz/4pLFNlQCMtIdTUyM61Lzu9iDorS4 qa0IpGdPwrnm846gZ952eZ0eiN8QJkq+TxhE1oH2pE4Bg06nuM27DgaPrFYAv4R2+1dw Z4WkLU/j8dvW9VLFsx1A6Pq3FxlA0LPRtWzFygFSLH4CK8GIdfziHN+nYp959KDftpL2 Jhdh4RK4M/HsgTWHlGCPMm6IN91TfYVcrA/9GOQUoPwtBCQBkl9+3v0pCw8nq43tVLE4 3fez04jxZjDtDIR6FHQqDpV3MFMpvR/O93vn5ST3wU3gRlQlg5I24hxEQmCuNSL1ivUr HatA== X-Gm-Message-State: AC+VfDxjNduylL6oFDGpr1HvFBzpL/mcxHtv+9HduDiL2mv6qAWxPi8L 0jy4B9s2yej4J5s0k1qgxjaIifeeZ/jXGmD0mpbLUzYAkojo/W6xUguFkz+3atof606J5QDDQiz asCt/qWAcvZ904JAcyifMWNS/eJBIzw== X-Received: by 2002:a05:6512:1c5:b0:4d8:8ad1:a05f with SMTP id f5-20020a05651201c500b004d88ad1a05fmr7756lfp.48.1686249712988; Thu, 08 Jun 2023 11:41:52 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4wpWOKRjymkR7d8dn965FDzeTZZqD7G60X+YQySbn4byxDl659ouIBfxnDjxQzv77OJrLuSw== X-Received: by 2002:a05:6512:1c5:b0:4d8:8ad1:a05f with SMTP id f5-20020a05651201c500b004d88ad1a05fmr7750lfp.48.1686249712515; Thu, 08 Jun 2023 11:41:52 -0700 (PDT) Received: from localhost (92.40.218.122.threembb.co.uk. [92.40.218.122]) by smtp.gmail.com with ESMTPSA id d6-20020adffd86000000b0030ae87bd3e3sm2303399wrr.18.2023.06.08.11.41.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Jun 2023 11:41:52 -0700 (PDT) From: Andrew Burgess To: Bruno Larsen , Christina Schimpe , gdb-patches@sourceware.org Subject: Re: [PATCH 1/1] gdb, infrun: refactor part of `proceed` into separate function In-Reply-To: <3bb3148b-b461-19cf-6c13-30fb3c6096d2@redhat.com> References: <20230607070959.3558904-2-christina.schimpe@intel.com> <536483a5-f3f0-aa09-95ab-df0cadb74eb7@redhat.com> <87r0qn1hfe.fsf@redhat.com> <3bb3148b-b461-19cf-6c13-30fb3c6096d2@redhat.com> Date: Thu, 08 Jun 2023 19:41:50 +0100 Message-ID: <871qilztht.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Bruno Larsen writes: > On 07/06/2023 16:21, Andrew Burgess wrote: >> Bruno Larsen via Gdb-patches writes: >> >>> On 07/06/2023 09:09, Christina Schimpe via Gdb-patches wrote: >>>> From: Mihails Strasuns >>>> >>>> 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) >>>> } >>>> } >>>> =20 >>>> +/* 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", >>>> +=09=09=09 tp->ptid.to_string ().c_str ()); >>>> + return; >>>> + } >>>> + >>>> + if (tp->resumed ()) >>>> + { >>>> + infrun_debug_printf ("[%s] resumed", >>>> +=09=09=09 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", >>>> +=09=09=09 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 !=3D nullptr >>>> + && (tp !=3D tp->inf->thread_waiting_for_vfork_done >>>> +=09 || 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 !=3D nullptr && tp !=3D >>> 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 !=3D 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)).=C2=A0 The first half is correct, but the sec= ond >>> one is nowhere near the second part. Along with that, there are multipl= e >>> early exits that I don't understand the code well enough to know if the= y >>> 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=20 > reading the whole change. >> >> So the second 'else if' before this patch is: >> >> else if (!cur_thr->resumed () >> =09 && !thread_is_in_step_over_chain (cur_thr) >> =09 /* In non-stop, forbid resuming a thread if some other thread of >> =09=09that inferior is waiting for a vfork-done event (this means >> =09=09breakpoints are out for this inferior). */ >> =09 && !(non_stop >> =09=09 && cur_thr->inf->thread_waiting_for_vfork_done !=3D 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: >> >> =09 && !(non_stop >> =09=09 && cur_thr->inf->thread_waiting_for_vfork_done !=3D nullptr)) >> >> this becomes another early exit with this condition: >> >> if (tp->inf->thread_waiting_for_vfork_done !=3D nullptr >> && (tp !=3D tp->inf->thread_waiting_for_vfork_done >> =09 || non_stop)) >> >> Previously the logic was: !(A && B) >> Now the logic is: !(B && (C || A)) >> =3D> !((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 !=3D >> 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 !=3D >> 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=20 > something to look into here. Currently, we allow the inferior to proceed= =20 > if (!non_stop && !target_is_non_stop_p () && tp !=3D=20 > tp->inf->thread_waiting_for_vfork_done) whereas the new code doesn't=20 > allow for this case. I don't know enough of GDB and non_stop mode to=20 > know if this is a possible case, and if removing it is good or not, so=20 > 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 =3D> non_stop > B =3D> target_is_non_stop_p () > C =3D> tp->inf->thread_waiting_for_vfork_done !=3D nullptr > D =3D> tp !=3D tp->inf->thread_waiting_for_vfork_done > E =3D> tp->resume () > F =3D> thread_is_in_step_over_chain > G =3D> tp->inf->has_execution () > H =3D> (!A & B) > asterisk =3D> AND operation > plus =3D> OR operation > > Current code has 2 possible flows. I'm calling flow 1 the one that goes= =20 > through the "for" loop, and flow 2 the other one (its also in order in=20 > 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)=C2=A0=C2=A0=C2=A0 [2]// and you mentione= d that adding=20 > & G to this equation is harmless, if not beneficial, so I'll add it from= =20 > now on > > The new version technically also has 2 control flows, but since it is H= =20 > & (condition) | !H & condition, we can factor H out and get a simplified= =20 > expression: > > * G * !E * !F * !(C * (D + A))=C2=A0=C2=A0=C2=A0 [3] > > Since both options have G & !E & !F, I'll ignore them, it won't change=20 > the fact that they're equal or not. Equation 3 can be rewritten as > > * !C + (!D * !A)=C2=A0=C2=A0=C2=A0 [4] > > Equation 4 is only useful at the end. Back to current code, we proceed=20 > when 1 or 2 is true, giving us: > > (!A * B) * !(C * D) + !(!A * B) * !(A * C) =3D> > !A * B * (!C + !D) + (A + !B) * (!A + !C) =3D> > !A*B*!C + !A*B*!D + A*!C + !A*!B + !B*!C; using the rule that !B*!C =3D= =20 > A*!B*!C + !A*!B*!C we have =3D> > !A*B*!C + !A*B*!D + !A*!B*!C + !A*!B + A*!C + A*!B*!C =3D> > !A*(B*!C + !B*!C + B*!D + !B) + A*(!C + !B*!C); using the rule that B*!C= =20 > + !B*!C =3D !C =3D> > !A*(!C + B*!D + !B) + A*!C =3D> > !A*(B*!D + !B) + !A*!C + A*!C =3D> > !A*(B*!D + !B*!D + !B*D) + !C =3D> > !A*(!D + !B*D) + !C =3D> > !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= =20 > compared to the new code. That term corresponds to continuing the=20 > inferior if !non_stop && !target_is_non_stop_p() && tp !=3D=20 > tp->inf->thread_waiting_for_vfork_done > > --=20 > Cheers, > Bruno > >> Thanks, >> Andrew