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.133.124]) by sourceware.org (Postfix) with ESMTPS id 7DC4B3858D35 for ; Thu, 22 Jun 2023 14:23:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7DC4B3858D35 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=1687443806; 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=Udgyi7tAHm8LBGFnJEBV+0ZgyX4W+mUtHevd75BtSBI=; b=VfWBSfD/FEcMmsbI3LRVpxI/aj+Hqv9J1JPKeAEFdxtXTOc9tFDH6iz9rClRndCAZqSsSQ KUOkHG3Xmcwk0BsHAxaRrFOyDWC9MmwkDHY+0rrGvPmGdtB5fCtrAhjp/nOyP9ibfrlX4i 4fcdTiOPvx6czHGckWC60tB3hnJ4Xa8= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-570-FIt2XMjoPwy41XZrjNa4mQ-1; Thu, 22 Jun 2023 10:23:23 -0400 X-MC-Unique: FIt2XMjoPwy41XZrjNa4mQ-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-3fa06fc72f2so7145305e9.0 for ; Thu, 22 Jun 2023 07:23:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687443800; x=1690035800; 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=Udgyi7tAHm8LBGFnJEBV+0ZgyX4W+mUtHevd75BtSBI=; b=gh5j6d9hPFFSsWPoAVOHQTgLMkJCCfVFKd/l3+bkiqao8tXKj1OUovRBk9Qjr9OlFk Vzh0w4+05vfJVIOG/GQ9grOywbuIr6yiECbnQWTLg7pgzukJv3W1MrHXndi2iyGVB33x scSOOrVv4+BMuVK43KBV0o2nlAJV0cZUmMSC4E5G/799QL0na6MKOjruZh0XpbRv5CDg E94/7v8qcBxE45SKYarWXKDmgTm0QTbOROFY9nxTfrHOK7T8X0rTL9v7g/ZwfW2q3x9u Ihkd6/ldh52d3/SeGwltXRqhFLpQUFlvFymRoMyW26lRYZxR6/naYPncpTlJlGS5aXUl m9XA== X-Gm-Message-State: AC+VfDwgCguxRXzIqQpplDvDGggIP5m12M8wJLvSkFcfHr/a3yCPkPQr n9rFKAkanLg40NQ2HJhK+bjY/MXnhuKiX8FDWO5W9nGztWp1/aMi2wRWiM6bYfG2acMzyC9hGT5 aOR9gIQWaCxZ7loTkDPCzmQ== X-Received: by 2002:a7b:cb56:0:b0:3f7:e48b:974d with SMTP id v22-20020a7bcb56000000b003f7e48b974dmr18626783wmj.27.1687443800125; Thu, 22 Jun 2023 07:23:20 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6hT5as4P5OUM9hw0C4wVsNnHiH+yE57QlslcV7QmmaAVmGqFocMKdzedgy3hy8yLiQfp+xMg== X-Received: by 2002:a7b:cb56:0:b0:3f7:e48b:974d with SMTP id v22-20020a7bcb56000000b003f7e48b974dmr18626757wmj.27.1687443799729; Thu, 22 Jun 2023 07:23:19 -0700 (PDT) Received: from localhost (2.72.115.87.dyn.plus.net. [87.115.72.2]) by smtp.gmail.com with ESMTPSA id g3-20020adff3c3000000b0030e52d4c1bcsm7198659wrp.71.2023.06.22.07.23.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Jun 2023 07:23:19 -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: <87edmbwxap.fsf@redhat.com> References: <20230612074927.424961-1-christina.schimpe@intel.com> <20230612074927.424961-2-christina.schimpe@intel.com> <87pm5yweda.fsf@redhat.com> <87edmbwxap.fsf@redhat.com> Date: Thu, 22 Jun 2023 15:23:18 +0100 Message-ID: <87y1kbshih.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.9 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,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,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: > 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. I finally got my vfork patches on the mailing list. In the end it felt more natural for this refactor to live part way through that series, so that's what I've done. You can see the series here: https://inbox.sourceware.org/gdb-patches/cover.1687438786.git.aburgess@redhat.com/ Thanks, Andrew