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 4B4993858C54 for ; Wed, 7 Jun 2023 09:26:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4B4993858C54 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=1686129999; 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=KSIZTcIj1mxP3dhhiBqZGmGJgEDgAh4Dw5CmsYvabRs=; b=S3n2H/FrTZxqboLneyCldzp0vq+njUPSEpXqwLSu98dO6ppoxvmEawRbuy5+7/jPt5LOFD wUERTv0qDT1C7zIGww2B4gfGaxuXOF8UsUvxEdL28gSIXLcZi/p2WYcjCefxIv4sUabPUY fre0t1eCW6ihvIBj/tkEubPplSQPFpo= Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-159-4O7l8qOwNOSTLg3srkIoAA-1; Wed, 07 Jun 2023 05:26:38 -0400 X-MC-Unique: 4O7l8qOwNOSTLg3srkIoAA-1 Received: by mail-qk1-f200.google.com with SMTP id af79cd13be357-75ebb50993fso448162385a.2 for ; Wed, 07 Jun 2023 02:26:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686129998; x=1688721998; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=KSIZTcIj1mxP3dhhiBqZGmGJgEDgAh4Dw5CmsYvabRs=; b=Id9yb4ELkE1+shPOFFllQE2IheW9qJF2LjLLIkmT3IJaxevrytvCGqEaG9Wu+Ow53Y 0HmnSzgcE7K9EHZb4MDNGJErv37v8oDfRJrMFAaMk4htCyuGq0Jz92ltT/hentQkaXDU tD1Byv9t7mk3g8NGrOcavwySK7qHYRxuyy1pbQjlJyEenuvkrv4L35Wl0nEB04tHbQXp 2R0Qo+q+nkAXFbXiFLmI9+13gWZPMaiLmdUnUD6SwqdLSZ2pleIHJsYoQy4GGaVDqstb uMvlz2hTriAY43xTl9OH0Y3nTm/Rrqx8el78FWlDXfLexX7zquI8Xf02OwbOaZjX69Ij JVmA== X-Gm-Message-State: AC+VfDxmSM05QBB1Fk+PrECNjhChaVqSUyGE9usgl7rre6YU/IKi9OjJ SIlZz9g9FtvRqiDeACfK8F/ZQVIhhGtNVwGDlcMNPQT/1GyAWKH0/gShhHX0AjlHhIWecPiH2jt UisFBZ0bUNtRCv2vyVpWC671ReKlIeQ== X-Received: by 2002:a05:620a:199d:b0:75b:23a1:3607 with SMTP id bm29-20020a05620a199d00b0075b23a13607mr1394146qkb.24.1686129997873; Wed, 07 Jun 2023 02:26:37 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ48MXoiJgJ1UcYWlJThZF7L2YK4bm+D6hUulF22wgVh9/O8tCW9ZP9wVvx3nCnHazJIOgd81g== X-Received: by 2002:a05:620a:199d:b0:75b:23a1:3607 with SMTP id bm29-20020a05620a199d00b0075b23a13607mr1394133qkb.24.1686129997490; Wed, 07 Jun 2023 02:26:37 -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 h6-20020a05620a10a600b0075edc88457asm384905qkk.135.2023.06.07.02.26.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 07 Jun 2023 02:26:36 -0700 (PDT) Message-ID: <536483a5-f3f0-aa09-95ab-df0cadb74eb7@redhat.com> Date: Wed, 7 Jun 2023 11:26:34 +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: Christina Schimpe , gdb-patches@sourceware.org References: <20230607070959.3558904-2-christina.schimpe@intel.com> From: Bruno Larsen In-Reply-To: <20230607070959.3558904-2-christina.schimpe@intel.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 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. If you would still like to avoid code repetition, I think the best is taking the lines that set a thread running into its own function, but that is up to you. I hope this makes sense... and if I am mistaken, please explain it to me :-) -- Cheers, Bruno > + { > + infrun_debug_printf ("[%s] another thread of this inferior is " > + "waiting for vfork-done", > + tp->ptid.to_string ().c_str ()); > + return; > + } > + > + infrun_debug_printf ("resuming %s", > + tp->ptid.to_string ().c_str ()); > + > + execution_control_state ecs (tp); > + switch_to_thread (tp); > + keep_going_pass_signal (&ecs); > + if (!ecs.wait_some_more) > + error (_("Command aborted.")); > +} > + > /* Basic routine for continuing the program in various fashions. > > ADDR is the address to resume at, or -1 for resume where stopped. > @@ -3456,67 +3513,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) > resume_ptid)) > { > switch_to_thread_no_regs (tp); > - > - if (!tp->inf->has_execution ()) > - { > - infrun_debug_printf ("[%s] target has no execution", > - tp->ptid.to_string ().c_str ()); > - continue; > - } > - > - if (tp->resumed ()) > - { > - infrun_debug_printf ("[%s] resumed", > - tp->ptid.to_string ().c_str ()); > - gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); > - continue; > - } > - > - if (thread_is_in_step_over_chain (tp)) > - { > - infrun_debug_printf ("[%s] needs step-over", > - tp->ptid.to_string ().c_str ()); > - continue; > - } > - > - /* 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. */ > - if (tp->inf->thread_waiting_for_vfork_done != nullptr > - && tp != tp->inf->thread_waiting_for_vfork_done) > - { > - infrun_debug_printf ("[%s] another thread of this inferior is " > - "waiting for vfork-done", > - tp->ptid.to_string ().c_str ()); > - continue; > - } > - > - infrun_debug_printf ("resuming %s", > - tp->ptid.to_string ().c_str ()); > - > - execution_control_state ecs (tp); > - switch_to_thread (tp); > - keep_going_pass_signal (&ecs); > - if (!ecs.wait_some_more) > - error (_("Command aborted.")); > + proceed_resume_thread_checked (tp); > } > } > - 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)) > - { > - /* The thread wasn't started, and isn't queued, run it now. */ > - execution_control_state ecs (cur_thr); > - switch_to_thread (cur_thr); > - keep_going_pass_signal (&ecs); > - if (!ecs.wait_some_more) > - error (_("Command aborted.")); > - } > + else > + proceed_resume_thread_checked (cur_thr); > > disable_commit_resumed.reset_and_commit (); > }