From patchwork Thu Apr 11 05:26:03 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Metzger, Markus T" X-Patchwork-Id: 88330 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0056D3849AC2 for ; Thu, 11 Apr 2024 05:27:33 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) by sourceware.org (Postfix) with ESMTPS id 644FE384AB42 for ; Thu, 11 Apr 2024 05:26:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 644FE384AB42 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=intel.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 644FE384AB42 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=198.175.65.18 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712813185; cv=none; b=DP6xUVGPLhajhSQteDr45WKaAeruwc1Wl7YTw7Qpwv/IBoivQWpu6ag+HTQeXIs5N85GqRRIGXfdJJ9qKctvlqs/rt95VC11lYDcmHwpu78P/IedSPiWkU5xvfsgWBhzG3QlwncTka3kH5HsMrTNMLGimMekwAd1BVMOCvjShEU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712813185; c=relaxed/simple; bh=BS4QG/VeP4C7CDcKg7hdyxddL6s+MvaV+wkOQ7axffQ=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=ibQnfvqUkGxofowaNwWY+cmQiePWe9LuCLrLeT22oujzTXMneTm5UYhfQLcetXMN8p2paIisspBPrKgaSGF1VvZsA2eTC41r6zkWcbJVUOiNvoFQdmhM+dtDd1Edgk6MYq/mFoxIJ9sfYRyj+F9ciOeuqd+qqibt7PV1APvOqYo= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1712813182; x=1744349182; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=BS4QG/VeP4C7CDcKg7hdyxddL6s+MvaV+wkOQ7axffQ=; b=TrfV2WIS0GRkKQafmbq0QNGeO7xvqqFLWzvFzr3JCcwD5AoMx9QFIp0o XsdzrwTJwfYm43sw0qA1hQ2FlspX3TuaX77jqhnIG6wlwhg6TbPp8pPht qRJwKZ0jDtOUwet4eGs93QFApYnWzVhlNxo0WWziVZN7KLamEc/1VW4N3 FYx5Xfa54aMGK6Frj3gSuvMbxsYCYjl4zmF3J1byTDG4DcP0Mfe7YGqcj Hk32GvpUrQvAf1w/O8WBNv2nZsXIUHS7P5rFNHjYDrE0bcv+02Lhm17FA dNCvnSoGJPOvnc9a6JSESQdjfq4fO75o6byqWi2E828xCwoy0WUa7hC7z A==; X-CSE-ConnectionGUID: ZmHB7mM8THCnxwXFm5iDpA== X-CSE-MsgGUID: 1pz+36q7TBiX6gs3tJokNQ== X-IronPort-AV: E=McAfee;i="6600,9927,11039"; a="8368012" X-IronPort-AV: E=Sophos;i="6.07,192,1708416000"; d="scan'208";a="8368012" Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Apr 2024 22:26:22 -0700 X-CSE-ConnectionGUID: HEIrFpzRQAWkdFYr1Ark0g== X-CSE-MsgGUID: Q51EXFSQSuqlp0nFe8om2w== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,192,1708416000"; d="scan'208";a="20764558" Received: from gkldtt-dev-004.igk.intel.com (HELO localhost) ([10.123.221.202]) by fmviesa010-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Apr 2024 22:26:21 -0700 From: Markus Metzger To: gdb-patches@sourceware.org Cc: pedro@palves.net Subject: [PATCH v2 5/6] gdb, infrun: fix silent inferior switch in do_target_wait() Date: Thu, 11 Apr 2024 05:26:03 +0000 Message-Id: <20240411052604.87893-6-markus.t.metzger@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20240411052604.87893-1-markus.t.metzger@intel.com> References: <20240411052604.87893-1-markus.t.metzger@intel.com> MIME-Version: 1.0 X-Spam-Status: No, score=-11.4 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_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org In do_target_wait(), we iterate over inferiors and call do_target_wait_1(), which eventually calls target_wait() per inferior. Each time, we wait for minus_one_ptid. In some cases, e.g. gdb.threads/detach-step-over.exp, we ask to wait for one inferior, and get an event from a different inferior back without noticing the inferior switch. Wait for a single inferior, instead. Since we iterate over all inferiors, we still cover everything. This exposes another bug with STOP_QUIETLY_NO_SIGSTOP handling. After attaching, we interrupt all threads in the new inferior, then call do_target_wait() to receive the stopped events. This randomly selects an inferior to start waiting for and iterates over all inferiors starting from there. The initial stop event for the main thread is already queued up, so we wouldn't actually wait() if we had started with the new inferior. Or if we had waited for minus_one_ptid, which would then have silently switched inferiors. Since we no longer allow that, we may actually wait() for the new inferior and find other events to report, out of which we randomly select one. If we selected an event for another thread, e.g. one that had been interrupted as part of non-stop attach, STOP_QUIETLY_NO_SIGSTOP would be applied to that thread (unnecessarily), leaving the main thread with a SIGSTOP event but last_resume_kind = 0 (resume_continue). When the main thread is later selected, SIGSTOP is reported to the user. Normally, linux-nat's wait() turns the SIGSTOP it uses for interrupting threads into GDB_SIGNAL_0. This is based on last_resume_kind, which is set to 2 (resume_stop) when sending SIGSTOP to interrupt a thread. We do this for all threads of the new inferior when interrupting them as part of non-stop attach. Except for the main thread, which we expect to be reported before the first wait(). Set last_resume_kind to resume_stop for the main thread after attaching. --- gdb/infrun.c | 46 +++++++++++++++++++++++++++++++++++++++++++--- gdb/linux-nat.c | 17 ++++++++++++----- gdb/remote.c | 22 +++++++++++++++++----- 3 files changed, 72 insertions(+), 13 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index a5030b16376..9ca0571065c 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4198,7 +4198,23 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs, auto do_wait = [&] (inferior *inf) { - ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options); + ptid_t ptid { inf->pid }; + + /* Make sure we're not widening WAIT_PTID. */ + if (!ptid.matches (wait_ptid) + /* Targets that cannot async will be asked for a blocking wait. + + Blocking wait does not work inferior-by-inferior if the target + provides more than one inferior. Fall back to waiting for + WAIT_PTID in that case. */ + || !target_can_async_p () || ((options & TARGET_WNOHANG) == 0) + /* FIXME: I don't see why we should have inferiors with zero pid, + which indicates that the respective ptid is not a process. + They do exist, though, and we cannot wait for them. */ + || !ptid.is_pid ()) + ptid = wait_ptid; + + ecs->ptid = do_target_wait_1 (inf, ptid, &ecs->ws, options); ecs->target = inf->process_target (); return (ecs->ws.kind () != TARGET_WAITKIND_IGNORE); }; @@ -4208,6 +4224,12 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs, reported the stop to the user, polling for events. */ scoped_restore_current_thread restore_thread; + /* The first TARGET_WAITKIND_NO_RESUMED execution state. + + If we do not find a more interesting event, we will report that. */ + execution_control_state no_resumed {}; + no_resumed.ptid = null_ptid; + intrusive_list_iterator start = inferior_list.iterator_to (*selected); @@ -4218,7 +4240,13 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs, inferior *inf = &*it; if (inferior_matches (inf) && do_wait (inf)) - return true; + { + if (ecs->ws.kind () != TARGET_WAITKIND_NO_RESUMED) + return true; + + if (no_resumed.ptid == null_ptid) + no_resumed = *ecs; + } } for (intrusive_list_iterator it = inferior_list.begin (); @@ -4228,7 +4256,19 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs, inferior *inf = &*it; if (inferior_matches (inf) && do_wait (inf)) - return true; + { + if (ecs->ws.kind () != TARGET_WAITKIND_NO_RESUMED) + return true; + + if (no_resumed.ptid == null_ptid) + no_resumed = *ecs; + } + } + + if (no_resumed.ptid != null_ptid) + { + *ecs = no_resumed; + return true; } ecs->ws.set_ignore (); diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 2602e1f240d..06b39d67a72 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -1154,6 +1154,7 @@ linux_nat_target::attach (const char *args, int from_tty) /* Add the initial process as the first LWP to the list. */ lp = add_initial_lwp (ptid); + lp->last_resume_kind = resume_stop; status = linux_nat_post_attach_wait (lp->ptid, &lp->signalled); if (!WIFSTOPPED (status)) @@ -3329,12 +3330,18 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus, moment at which we know its PID. */ if (ptid.is_pid () && find_lwp_pid (ptid) == nullptr) { - ptid_t lwp_ptid (ptid.pid (), ptid.pid ()); + /* Unless we already did and this is simply a request to wait for a + particular inferior. */ + inferior *inf = find_inferior_ptid (linux_target, ptid); + if (inf && inf->find_thread (ptid)) + { + ptid_t lwp_ptid (ptid.pid (), ptid.pid ()); - /* Upgrade the main thread's ptid. */ - thread_change_ptid (linux_target, ptid, lwp_ptid); - lp = add_initial_lwp (lwp_ptid); - lp->resumed = 1; + /* Upgrade the main thread's ptid. */ + thread_change_ptid (linux_target, ptid, lwp_ptid); + lp = add_initial_lwp (lwp_ptid); + lp->resumed = 1; + } } /* Make sure SIGCHLD is blocked until the sigsuspend below. */ diff --git a/gdb/remote.c b/gdb/remote.c index a09ba4d715d..49abd4e4376 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7825,12 +7825,24 @@ remote_target::remote_notif_remove_queued_reply (ptid_t ptid) { remote_state *rs = get_remote_state (); + auto pred = [=] (const stop_reply_up &event) + { + /* A null ptid should only happen if we have a single process. It + wouldn't match the process ptid, though, so let's check this case + separately. */ + if ((event->ptid == null_ptid) && ptid.is_pid ()) + return true; + + /* A minus one ptid should only happen for events that match + everything. It wouldn't match a process or thread ptid, though, so + let's check this case separately. */ + if (event->ptid == minus_one_ptid) + return true; + + return event->ptid.matches (ptid); + }; auto iter = std::find_if (rs->stop_reply_queue.begin (), - rs->stop_reply_queue.end (), - [=] (const stop_reply_up &event) - { - return event->ptid.matches (ptid); - }); + rs->stop_reply_queue.end (), pred); stop_reply_up result; if (iter != rs->stop_reply_queue.end ()) {