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 9372F3858C54 for ; Wed, 7 Jun 2023 17:24:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9372F3858C54 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=1686158674; 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=8XKuwN4fpvnmBEtAWkF/djBJrh7i5d80RR/rfa/aCeA=; b=aCQlQIcBaKcQgyfI3zHWnxObjVQ8UR16QBFqx8kpQFdpNIBidS0ybOnr2tsp6+9ZSwynVa ofi/+iOWoLD4tbqRzqPXwryTyqu6eT6Bo6KS+81cd3Ol2T6wS2GQRk19hLymtgvIQrHumb 9nQPP5jzTY/2OgcFG9HV7kcmFJ7CUnA= Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-365-8qzBCUO_Ohy96m3mPmHnww-1; Wed, 07 Jun 2023 13:24:32 -0400 X-MC-Unique: 8qzBCUO_Ohy96m3mPmHnww-1 Received: by mail-qk1-f197.google.com with SMTP id af79cd13be357-75ec7dedd93so79454185a.0 for ; Wed, 07 Jun 2023 10:24:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686158672; x=1688750672; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=8XKuwN4fpvnmBEtAWkF/djBJrh7i5d80RR/rfa/aCeA=; b=SA31Zdv/zis0dUOqmFo6v2+RuSh+CmV9yvOX9tPNBkx6HcY1VwLtTJ20xGq/uxPt71 QqLYGxaV6XyzXMuo0LP19+uV24K3GN7SDHzLKkO5F5u0Z7DtqqaGsl4/rYLGsQGjXwHM XZIRMt3whW7K1oqp84tPFMPDFI7FvU73OglelUKc98ThzSMKdAzE/1YiWOpU0cBi5YU4 scW/cpUqzwpHHbDTc+JCsAAOXcOiNVMvrLD/HlqWBNZ0nQ1d5Rx/Vg0PUtiZANzbBmsm Xe9+e+P45IEZ207FEihwQH04dmmSZpWe3mki/cT8ZppphNO4g4gFk1xIqBKfT92YHFyl CUqg== X-Gm-Message-State: AC+VfDwre2gyau3k0af8V/zTha26a0Dcz0smry6m0fs2C+tvWtqAUVbm Zw3m0XwO+uJIcPh5KFfy5EbE503YcTLfNdKnrE9aMOIdIeUAErajD0y3m7qqgP/yWU8BkTPgwpa 6UonCLnX627m12hFBFJ+EPg== X-Received: by 2002:a37:bce:0:b0:75b:23a1:d848 with SMTP id 197-20020a370bce000000b0075b23a1d848mr2558149qkl.10.1686158672088; Wed, 07 Jun 2023 10:24:32 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5mHtaAY+s7etz2fCzwhfEPAdP+oyS1cNgt8KGQI8OzbyOgbj/ZupGVWkMD15kflsSh6u9wqw== X-Received: by 2002:a37:bce:0:b0:75b:23a1:d848 with SMTP id 197-20020a370bce000000b0075b23a1d848mr2558128qkl.10.1686158671671; Wed, 07 Jun 2023 10:24:31 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-225-44.bb.vodafone.cz. [94.112.225.44]) by smtp.gmail.com with ESMTPSA id a20-20020a0ce354000000b006286334f999sm59629qvm.78.2023.06.07.10.24.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 07 Jun 2023 10:24:31 -0700 (PDT) Message-ID: <3bb3148b-b461-19cf-6c13-30fb3c6096d2@redhat.com> Date: Wed, 7 Jun 2023 19:24:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH 1/1] gdb, infrun: refactor part of `proceed` into separate function To: Andrew Burgess , Christina Schimpe , gdb-patches@sourceware.org References: <20230607070959.3558904-2-christina.schimpe@intel.com> <536483a5-f3f0-aa09-95ab-df0cadb74eb7@redhat.com> <87r0qn1hfe.fsf@redhat.com> From: Bruno Larsen In-Reply-To: <87r0qn1hfe.fsf@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,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: 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) >>> } >>> } >>> >>> +/* 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