From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id kMlSNbeqBWbKwhkAWB0awg (envelope-from ) for ; Thu, 28 Mar 2024 13:36:55 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=L00i6iLL; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id D68A21E0C0; Thu, 28 Mar 2024 13:36:55 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id B9ACA1E030 for ; Thu, 28 Mar 2024 13:36:53 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 4FD643858403 for ; Thu, 28 Mar 2024 17:36:53 +0000 (GMT) Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.9]) by sourceware.org (Postfix) with ESMTPS id 8826A3858C36 for ; Thu, 28 Mar 2024 17:36:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8826A3858C36 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 8826A3858C36 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=198.175.65.9 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711647375; cv=none; b=rUmMdxb8G7oVEhWGyHJ701CoZCeknDAIEPuEtPV97tCQcXkwgMdRspVozjqQhtm66lldaTV+yThRNpms2cr6urxtwsLFgmkMALipvBIAqlPLumd7oYU3hZxlF/uw/GiVYoui2qK984Qk4vaMtQa/2DBVG/g/jzf9RUoNPvpD8eI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711647375; c=relaxed/simple; bh=fIflWrETZ8gEIJ7cV2NjFs0bd5vJq7hLvEs4piQjJVs=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=lgZO507RwPCoYt1HDIG/0srVOz0vvCrkgUrdO7DpmW290rvR/8AekpxqTTgehKwiI6Q5vHFzWPz+sBk+g+WxiquUdyHvBaA1Tld0ndssIp3kiL+bgfXvjkabC+KS3xUb2VloiFJEtwkXfgC40COB5OcmGbB1DvDy0+dDrdjYE8o= 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=1711647372; x=1743183372; h=from:to:subject:date:message-id:in-reply-to:references: mime-version:content-transfer-encoding; bh=fIflWrETZ8gEIJ7cV2NjFs0bd5vJq7hLvEs4piQjJVs=; b=L00i6iLL9nGDJBhxmToZk/oZLoemZzzAim0630gD78PKlMv9ORB36t1e WC2dsJbJll9MMaYv/02y6cCKdvDUxHyxB5GgXn3g+KZT14Q3/iPwelTZC yPgUUYw2r7WpcVGI+rY3KUnVUjyDrln/LUkOnyA2E6mw46I+5LYCrhcod OdoJg4xn0Wd62IN6mXzicusQweAYIvpjjkIr7/1BdSIun2R44reXaVO4j ijvXTsyxp93uIPF+bxAkXaZsOWp3njJ4PG95Ice5URYI+74NFQlPE+z1A uJdE/+8fdWtiQQU/wRN/x1OzSpQFQoTnTdsjj1XdvLp07BHLO0ZopZDSS w==; X-CSE-ConnectionGUID: 9RyGRUw6S02blVOUF20shg== X-CSE-MsgGUID: uXtF0O5DQseYRxLhfhurIw== X-IronPort-AV: E=McAfee;i="6600,9927,11027"; a="29302314" X-IronPort-AV: E=Sophos;i="6.07,162,1708416000"; d="scan'208";a="29302314" Received: from orviesa008.jf.intel.com ([10.64.159.148]) by orvoesa101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Mar 2024 10:36:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,162,1708416000"; d="scan'208";a="17354572" Received: from gkldtt-dev-004.igk.intel.com (HELO localhost) ([10.123.221.202]) by orviesa008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Mar 2024 10:36:11 -0700 From: Tankut Baris Aktemur To: gdb-patches@sourceware.org Subject: [PATCH 3/3] gdb: avoid redundant calls to target_stop during attach Date: Thu, 28 Mar 2024 18:35:37 +0100 Message-Id: <20240328173537.2679010-3-tankut.baris.aktemur@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20240328173537.2679010-1-tankut.baris.aktemur@intel.com> References: <20240328173537.2679010-1-tankut.baris.aktemur@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.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_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, WEIRD_PORT 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+public-inbox=simark.ca@sourceware.org When attaching to a non-stop target (e.g. the native Linux target in the default mode or the remote target after "maint set target-non-stop on" or "set non-stop on"), GDB sends a stop request to the target at (from infcmd.c:attach_command) if (target_is_non_stop_p ()) { /* If we find that the current thread isn't stopped, explicitly do so now, because we're going to install breakpoints and poke at memory. */ if (async_exec) /* The user requested an `attach&'; stop just one thread. */ target_stop (inferior_ptid); else /* The user requested an `attach', so stop all threads of this inferior. */ target_stop (ptid_t (inferior_ptid.pid ())); } Suppose we are in all-stop-on-top-of-non-stop mode with multiple threads. After sending the stop requests to the threads and completing the attach command, but before giving the prompt to the user, GDB waits for the target in 'wait_sync_command_done' and receives a stop event. After handling the event, GDB attempts to stop all threads at (top-gdb) bt #0 stop_all_threads (reason=0x555557023ef0 "presenting stop to user in all-stop", inf=0x0) at src/gdb/infrun.c:5594 #1 0x00005555561c0cb5 in stop_all_threads_if_all_stop_mode () at src/gdb/infrun.c:4326 #2 0x00005555561c19a9 in fetch_inferior_event () at src/gdb/infrun.c:4676 #3 0x00005555561993b3 in inferior_event_handler (event_type=INF_REG_EVENT) at src/gdb/inf-loop.c:42 #4 0x000055555621b0e3 in handle_target_event (error=0, client_data=0x0) at src/gdb/linux-nat.c:4316 #5 0x0000555556f37bf6 in handle_file_event (file_ptr=0x5555587344f0, ready_mask=1) at src/gdbsupport/event-loop.cc:573 #6 0x0000555556f381f0 in gdb_wait_for_event (block=0) at src/gdbsupport/event-loop.cc:694 #7 0x0000555556f36ea7 in gdb_do_one_event (mstimeout=-1) at src/gdbsupport/event-loop.cc:217 #8 0x000055555662ce24 in wait_sync_command_done () at src/gdb/top.c:427 #9 0x000055555662ced0 in maybe_wait_sync_command_done (was_sync=0) at src/gdb/top.c:444 #10 0x000055555662d591 in execute_command (p=0x7fffffffe2ba "2", from_tty=1) at src/gdb/top.c:577 #11 0x000055555626fe2d in catch_command_errors (command=0x55555662ceed , arg=0x7fffffffe2ad "attach 2892262", from_tty=1, do_bp_actions=true) at src/gdb/main.c:513 #12 0x000055555627007c in execute_cmdargs (cmdarg_vec=0x7fffffffdad0, file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0x7fffffffda1c) at src/gdb/main.c:612 #13 0x000055555627163b in captured_main_1 (context=0x7fffffffdd20) at src/gdb/main.c:1293 #14 0x0000555556271871 in captured_main (data=0x7fffffffdd20) at src/gdb/main.c:1314 #15 0x0000555556271920 in gdb_main (args=0x7fffffffdd20) at src/gdb/main.c:1343 #16 0x0000555555cf8d0b in main (argc=9, argv=0x7fffffffde58) at src/gdb/gdb.c:39 In stop_all_threads, the thread states are [infrun] infrun_debug_show_threads: enter [infrun] infrun_debug_show_threads: non-exited threads: [infrun] infrun_debug_show_threads: thread 2892262.2892262.0, executing = 0, resumed = 0, stop_requested = 0, state = STOPPED [infrun] infrun_debug_show_threads: thread 2892262.2892271.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING [infrun] infrun_debug_show_threads: thread 2892262.2892272.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING [infrun] infrun_debug_show_threads: thread 2892262.2892273.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING [infrun] infrun_debug_show_threads: thread 2892262.2892274.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING [infrun] infrun_debug_show_threads: exit Note that the first thread appears as 'executing = 0, state = STOPPED', because it is the thread for which the stop event was received first. Also note that for all of the threads we have 'stop_requested = 0' even though we had sent stop requests already in 'attach_command'. This causes GDB to call 'target_stop' again for each thread (except the first eventing one, for which 'executing = 0'). The goal of this patch is to avoid the unnecessary calls to 'target_stop', but first a note about this motivation: The Linux native target keeps track of whether a stop request was already sent to a thread, and if so, does not send a request again. The remote target peeks the pending stop events and if there is already an event queued for the thread, skips sending a vCont packet. That is, calling 'target_stop' is harmless. From that perspective, it may seem like this patch is implementing a very minor optimization. However, the actual motivation of the patch is to improve the internal state book-keeping in GDB and to make the execution flow easier to follow for the developer. To avoid the redundant calls to 'target_stop', the solution we take is to set the 'stop_requested' field of threads in 'attach_command' after stopping the target. We use the existing 'stop_current_target_threads_ns' function for this purpose. When we use this approach, in the same 'stop_all_threads' above, this time we get [infrun] infrun_debug_show_threads: enter [infrun] infrun_debug_show_threads: non-exited threads: [infrun] infrun_debug_show_threads: thread 2892262.2892262.0, executing = 0, resumed = 0, stop_requested = 1, state = STOPPED [infrun] infrun_debug_show_threads: thread 2892262.2892271.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING [infrun] infrun_debug_show_threads: thread 2892262.2892272.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING [infrun] infrun_debug_show_threads: thread 2892262.2892273.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING [infrun] infrun_debug_show_threads: thread 2892262.2892274.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING [infrun] infrun_debug_show_threads: exit The 'stop_requested = 1' values help GDB avoid skip the 'target_stop' calls. GDB goes on to fetch the pending stop events and finishes 'stop_all_threads' successfully. Above, there is one glitch, though, at [infrun] infrun_debug_show_threads: thread 2892262.2892262.0, executing = 0, resumed = 0, stop_requested = 1, state = STOPPED The 'stop_requested' field remained set. This causes a failure in 'gdb.threads/attach-non-stop.exp' where a thread that is expected to become running remains stopped, because of the following check in 'proceed_after_attach': if (!thread->executing () && !thread->stop_requested && thread->stop_signal () == GDB_SIGNAL_0) { switch_to_thread (thread); clear_proceed_status (0); proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); } I think the solution is to clear the 'stop_requested' field after we process the stop event. I first thought of doing this inside 'mark_non_executing_threads', but this does not work because the 'stop_requested' field is still used in the execution flow thereafter. Then I settled at 'fetch_inferior_event' in the block where we are sure that we should stop. Continuing the investigation, let us now focus on the synchronous attach command (i.e. "attach " without any "&") when the non-stop mode is on. In 'attach_command', GDB binds a continuation to the inferior: /* Wait for stop. */ inferior->add_continuation ([=] () { attach_post_wait (from_tty, mode); }); The 'mode' parameter is ATTACH_POST_WAIT_STOP in the sync mode. In 'attach_post_wait', we have else if (mode == ATTACH_POST_WAIT_STOP) { /* The user requested a plain `attach', so be sure to leave the inferior stopped. */ /* At least the current thread is already stopped. */ /* In all-stop, by definition, all threads have to be already stopped at this point. In non-stop, however, although the selected thread is stopped, others may still be executing. Be sure to explicitly stop all threads of the process. This should have no effect on already stopped threads. */ if (non_stop) target_stop (ptid_t (inferior->pid)); ... If 'non_stop' is true, then 'target_is_non_stop_p ()' must have been true as well, and because 'async_exec' is false, we must have done 'target_stop (ptid_t (inferior_ptid.pid ()))' in 'attach_command' already. This means, the call to 'target_stop' in the code above is unnecessary and can be removed. We make this simplification in the patch. Regression-tested on X86-64 Linux using the unix, native-gdbserver, and native-extended-gdbserver board files. --- gdb/infcmd.c | 31 ++++++++++++++++--------------- gdb/infrun.c | 1 + 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 68ecdb9feba..247db761a87 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -63,6 +63,8 @@ static void until_next_command (int); static void step_1 (int, int, const char *); +static void stop_current_target_threads_ns (ptid_t); + #define ERROR_NO_INFERIOR \ if (!target_has_execution ()) error (_("The program is not being run.")); @@ -2568,14 +2570,12 @@ attach_post_wait (int from_tty, enum attach_post_wait_mode mode) /* At least the current thread is already stopped. */ - /* In all-stop, by definition, all threads have to be already - stopped at this point. In non-stop, however, although the - selected thread is stopped, others may still be executing. - Be sure to explicitly stop all threads of the process. This - should have no effect on already stopped threads. */ - if (non_stop) - target_stop (ptid_t (inferior->pid)); - else if (target_is_non_stop_p ()) + /* In all-stop targets, by definition, all threads have to be + already stopped at this point. In non-stop targets, however, + be sure to explicitly stop all threads of the process if we + are in all-stop mode. This should have no effect on already + stopped threads. */ + if (!non_stop && target_is_non_stop_p ()) { struct thread_info *lowest = inferior_thread (); @@ -2686,13 +2686,14 @@ attach_command (const char *args, int from_tty) do so now, because we're going to install breakpoints and poke at memory. */ - if (async_exec) - /* The user requested an `attach&'; stop just one thread. */ - target_stop (inferior_ptid); - else - /* The user requested an `attach', so stop all threads of this - inferior. */ - target_stop (ptid_t (inferior_ptid.pid ())); + /* If the user requested an `attach&', stop just one thread. + If the user requested an `attach', stop all threads of this + inferior. */ + ptid_t stop_ptid = (async_exec + ? inferior_ptid + : ptid_t (inferior_ptid.pid ())); + + stop_current_target_threads_ns (stop_ptid); } /* Check for exec file mismatch, and let the user solve it. */ diff --git a/gdb/infrun.c b/gdb/infrun.c index c0ebc95a061..91b0bdea46f 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4684,6 +4684,7 @@ fetch_inferior_event () bool should_notify_stop = true; bool proceeded = false; + set_stop_requested (ecs.target, ecs.ptid, false); stop_all_threads_if_all_stop_mode (); clean_up_just_stopped_threads_fsms (&ecs); -- 2.34.1 Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928