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 1BD5C3854E4B for ; Fri, 16 Jun 2023 09:50:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1BD5C3854E4B 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=1686909045; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Z3CyNNZTzPo3NyOUEXCLrbtgDRFEGMefYqkSIheFcvM=; b=GkWxtQdG45NQldWM2ymoaqpM1MLSWv5VzfqXzyoto33krRC929c1G6bdIEGt2or1sBHdF3 BAp2w5v7whJ/3+Ck/igDrOlV9k8VQ3JPRLC6wyU4xSZAXrA1lT04hud/NQ8MQefirCy5cY aSrLqs8PZ+coRm7hR+Q53CC7nxuqa6Y= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-27-XqNMXPvLOu6I4CIrMth-EQ-1; Fri, 16 Jun 2023 05:50:41 -0400 X-MC-Unique: XqNMXPvLOu6I4CIrMth-EQ-1 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-31114af5e45so197395f8f.1 for ; Fri, 16 Jun 2023 02:50:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686909041; x=1689501041; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Z3CyNNZTzPo3NyOUEXCLrbtgDRFEGMefYqkSIheFcvM=; b=EPH2dsAvQi32PYtitB1SN8bcS+mbbG3noU9SkULr8c3RB8KcrvgO52nqsIuuyIwB8y bVLWLeSt8+l1GCaWYFl0MfIuDCcPNUBFEB/raXK0p3ggxtRtPTZhtXFd86mo+NJTQlhN zx/7r1wWrDMfiiO0D67vPJd+XJJ+N5EAt7Vk7x4PqOBChl58r8u6sXbGfKC81/i1irQH 4G5Ei9Q/yRH/dqmNCDbV8C7yBixs6Y3T5aqB8CSVj/XfXYJckPHbMWVre8d/16+4F6Ha NixqiJRqqsJX7EpsmmXy+2PCLF52bZloiNDN7wS850WYOxda9dMZE6y4rTr7EqCxU0sL Xn1A== X-Gm-Message-State: AC+VfDzhcuTTFAjmccroauike1mPqFB9EneJr8YmGncBsiAcMlSkiK9q e4dKwBcu8LBexfLwxGlx0GUZTwTHJsDSzAitprfR8uZgGEShu5Qx41DkGf+35CDxJOytcD7uaUc 7m9LsqcURNlzxqWYal7Zf8g== X-Received: by 2002:a5d:480c:0:b0:30f:bafb:246c with SMTP id l12-20020a5d480c000000b0030fbafb246cmr971695wrq.61.1686909040722; Fri, 16 Jun 2023 02:50:40 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6ggXXZezpU3dmaofjdak5al2BHh3NJoQUJFMWglbkAOVg1E3G867nqYN9VZL5s77HMOoc90g== X-Received: by 2002:a5d:480c:0:b0:30f:bafb:246c with SMTP id l12-20020a5d480c000000b0030fbafb246cmr971672wrq.61.1686909040225; Fri, 16 Jun 2023 02:50:40 -0700 (PDT) Received: from localhost (2.72.115.87.dyn.plus.net. [87.115.72.2]) by smtp.gmail.com with ESMTPSA id e5-20020adff345000000b0030630120e56sm23342915wrp.57.2023.06.16.02.50.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 16 Jun 2023 02:50:39 -0700 (PDT) From: Andrew Burgess To: Christina Schimpe , gdb-patches@sourceware.org Cc: blarsen@redhat.com, Simon Marchi Subject: Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function In-Reply-To: <87pm5yweda.fsf@redhat.com> References: <20230612074927.424961-1-christina.schimpe@intel.com> <20230612074927.424961-2-christina.schimpe@intel.com> <87pm5yweda.fsf@redhat.com> Date: Fri, 16 Jun 2023 10:50:38 +0100 Message-ID: <87edmbwxap.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: Andrew Burgess writes: > Christina Schimpe writes: > >> From: Mihails Strasuns >> >> Split thread resuming block into separate function. >> >> Co-Authored-By: Christina Schimpe >> --- >> gdb/infrun.c | 120 ++++++++++++++++++++++++++------------------------- >> 1 file changed, 61 insertions(+), 59 deletions(-) >> >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index 58da1cef29e..ec41b658864 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target) >> } >> } >> >> + /* Helper function for `proceed`. Check if thread TP is suitable for >> + resuming, and, if it is, switch to the thread and call >> + `keep_going_pass_signal`. If TP is not suitable for resuming then >> + this function will just return without switching threads. */ > > The indentation here is wrong -- header comments should start in the > first column > >> + >> +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; >> + } >> + >> + /* There are further two scenarios for which we must not resume the thread: >> + - 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)) >> + { >> + infrun_debug_printf ("[%s] another thread of this inferior is " >> + "waiting for vfork-done", >> + tp->ptid.to_string ().c_str ()); >> + return; >> + } > > I tried to like this merged logic block, but I just couldn't. So in the > end I went back to the original commit: > > commit d8bbae6ea080249c05ca90b1f8640fde48a18301 > Date: Fri Jan 14 15:40:59 2022 -0500 > > gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) > > And tried to understand what's going on. The commit message on that > original commit is awesome, and well worth a read. > > After looking at that for a while I think I have a replacement for this > block, which I think achieves the same thing, but is structured, and > commented slightly differently. > > I've included an updated version of this patch below, I'd love to hear > what you think? > > The patch below is only partially tested at this point. I'm a little > short of time right now. But it passes all the vfork related tests that > I've been looking at, so I'm reasonably sure it's OK, I just wanted to > get this out so you could take a look at it. > > Thanks, > Andrew > > --- > > commit 45460db250f116956f4d75376f6637d2e4e7632f > Author: Mihails Strasuns > Date: Mon Jun 12 09:49:27 2023 +0200 > > gdb, infrun: refactor part of `proceed` into separate function > > Split the thread resuming code from proceed into new function > proceed_resume_thread_checked. > > Co-Authored-By: Christina Schimpe > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 58da1cef29e..7e6104ffab2 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -3268,6 +3268,90 @@ check_multi_target_resumption (process_stratum_target *resume_target) > } > } > > +/* Helper function for `proceed`. Check if thread TP is suitable for > + resuming, and, if it is, switch to the thread and call > + `keep_going_pass_signal`. If TP is not suitable for resuming then this > + function will just return without switching threads. */ > + > +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; > + } > + > + /* When handling a vfork GDB removes all breakpoints from the program > + space in which the vfork is being handled, as such we must take care > + not to resume any thread other than the vfork parent -- resuming the > + vfork parent allows GDB to receive and handle the 'vfork done' > + event. */ > + if (tp->inf->thread_waiting_for_vfork_done != nullptr) > + { > + if (target_is_non_stop_p ()) > + { > + /* For non-stop targets, regardless of whether GDB is using > + all-stop or non-stop mode, threads are controlled > + individually. > + > + When a thread is handling a vfork, breakpoints are removed > + from the inferior (well, program space in fact), so it is > + critical that we don't try to resume any thread other than the > + vfork parent. */ > + if (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 ()); > + return; > + } This actually fixes a bug that exists currently in GDB. I'm working on a patch which includes a test that exposes the bug. I plan to get the bug fix merged before we merge this refactoring commit. > + > + /* In non-stop mode we will never end up trying to proceed the > + thread that is handling a vfork. When we realise that a > + thread is handling a vfork we immediately stop all the other > + threads, and resume the vfork parent thread. As far as the > + user is concerned, the vfork parent thread is running. Any > + attempt by the user to interrupt the vfork parent will be > + deferred until the vfork has completed, at which point the > + parent will no longer be waiting for a vfork. */ > + gdb_assert (!non_stop); And this assertion is not correct. Again, the new test I'll add will trigger this assert. We can get this refactor merged once my bug fix has landed. I'll follow up once I have something posted. Thanks, Andrew > + } > + else > + { > + /* For all-stop targets, when we attempt to resume the inferior, > + we will in fact only resume the vfork parent thread, this is > + handled in internal_resume_ptid, as such, there is nothing > + more that needs doing here. */ > + } > + } > + > + 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 +3540,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 (); > }