From patchwork Wed Dec 4 16:35:30 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 102389 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 BFEAE3858423 for ; Wed, 4 Dec 2024 16:39:41 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 2F64A3858D20 for ; Wed, 4 Dec 2024 16:39:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2F64A3858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2F64A3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1733330353; cv=none; b=xvXzmL/Wy0sgMWG2jTwQEKldrEGnJhV7YN2t9hKYkCQDeyB/EP8Le0lkjTLv0WUNbLsPcB774/Z7WBD8wKGZpfSZjPhJMna/QNyrqtUInDwY8S8pDNZFIHkSqZ5sdsHtd+Whr7L0ScAlwWjmEOX+9i300my59pH77r/HqqjCO1c= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1733330353; c=relaxed/simple; bh=ZBmCaGplbAxQ3wbqT08C0pZ6FVQuBpFYBF50JUCQzSM=; h=DKIM-Signature:DKIM-Signature:From:To:Subject:Date:Message-ID: MIME-Version; b=YYYC9iouWf1iwjwS2tFOqPBL7ZMo8lMgM4vzKJQW9ifzb1MBq3zNm6XIOlfu4EgYI3eYfWLJTtIt7x7gIc9vIsR2uLtkx7xx4pMax6SjKIxMbU4P4xXBC0LBvUFjnWpu9P8yj2sJQHmSbNpZ8Os99XO9W8aydqPXHG9JaKFBP1A= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1733330345; bh=ZBmCaGplbAxQ3wbqT08C0pZ6FVQuBpFYBF50JUCQzSM=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=KpiKsvrnAYRw9VuRC9KCtpYYTc6Q6NFKrGRzm0hIiz/VMmUAe38gsBEtnWmDWiH+2 jMs3PndWCVgJVPpKMGiNhb4+RbZVHEUrVNTbeAf4RhJiW9v78nCNeO9m9pFtqKx9XK sDEVs4SnJElRAGllQyJXxvI0+qktiMixTVnHDcDg= Received: by simark.ca (Postfix, from userid 112) id 216061E1A5; Wed, 4 Dec 2024 11:39:05 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-Spam-Level: X-Spam-Status: No, score=-161.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_PASS, SPF_PASS, TXREP, URIBL_BLOCKED, URIBL_SBL_A autolearn=ham autolearn_force=no version=3.4.6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1733330340; bh=ZBmCaGplbAxQ3wbqT08C0pZ6FVQuBpFYBF50JUCQzSM=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=lVMAMBYnRWgiroCbEz32yHaX5F9M2pNhN36/I08mcsThg2N+fi4Llo6ZCdvtcm4PG B6WjpzyPn9A9Ea0lcPmpOa6DrZabP9OUWDz/9mvY0YnaTrk/uokFiuV0qV40AObinS gFIo4/QjC5VXf94YJZdiPYtAsYw9WQ66UX1Xi29M= Received: from simark.localdomain (modemcable238.237-201-24.mc.videotron.ca [24.201.237.238]) (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 ESMTPSA id 1049D1E1A4; Wed, 4 Dec 2024 11:39:00 -0500 (EST) From: simark@simark.ca To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 3/3] gdbserver: win32 simplify process removal Date: Wed, 4 Dec 2024 11:35:30 -0500 Message-ID: <20241204163857.2297347-4-simark@simark.ca> X-Mailer: git-send-email 2.47.1 In-Reply-To: <20241204163857.2297347-1-simark@simark.ca> References: <20241204163857.2297347-1-simark@simark.ca> MIME-Version: 1.0 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 From: Simon Marchi In the spirit of encapsulation, I'm looking to remove the need for external code to access the "ptid -> thread" map of process_info, making it an internal implementation detail. The only remaining use is in function clear_inferiors, and it led me down this rabbit hole: - clear_inferiors is really only used by the Windows port and doesn't really make sense in the grand scheme of things, I think (when would you want to remove all threads of all processes, without removing those processes?) - ok, so let's remove clear_inferiors and inline the code where it's called, in function win32_clear_inferiors - the Windows port does not support multi-process, so it's not really necessary to loop over all processes like this: for_each_process ([] (process_info *process) { process->thread_list ().clear (); process->thread_map ().clear (); }); We could just do: current_process ()->thread_list ().clear (); current_process ()->thread_map ().clear (); (or pass down the process from the caller, but it's not important right now) - so, the code that we've inlined in win32_clear_inferiors does 3 things: - clear the process' thread list and map (which deletes the thread_info objects) - clear the dll list, which just basically frees some objects - switch to no current process / no current thread - let's now look at where this win32_clear_inferiors function is used: - in win32_process_target::kill, where the process is removed just after - in win32_process_target::detach, where the process is removed just after - in win32_process_target::wait, when handling a process exit. After this returns, we could be in handle_target_event (if async) or resume (if sync), both in `server.cc`. In both of these cases, target_mourn_inferior gets called, we end up in win32_process_target::mourn, which removes the process - in all 3 cases above, we end up removing the process, which takes care of the 3 actions listed above: - the thread list and map get cleared when the process gets destroyed - same with the dll list - remove_process switches to no current process / current thread if the process being removed is the current one - I conclude that it's probably unnecessary to do the cleanup in win32_clear_inferiors, because it's going to get done right after anyway. Therefore, this patch does: - remove clear_inferiors, remove the call in win32_clear_inferiors - remove clear_dlls, which is now unused - remove process_info::thread_map, which is now unused - rename win32_clear_inferiors to win32_clear_process, which seems more accurate win32_clear_inferiors also does: for_each_thread (delete_thread_info); which also makes sure to delete all threads, but it also deletes the Windows private data object (windows_thread_info), so I'll leave this one there for now. But if we could make the thread private data destruction automatic, on thread destruction, it could be removed, I think. There should be no user-visible change with this patch. Of course, operations don't happen in the same order as before, so there might be some important detail I'm missing. I'm only able to build-test this, if someone could give it a test run on Windows, it would be appreciated. Change-Id: I4a560affe763a2c965a97754cc02f3083dbe6fbf Reviewed-By: Hannes Domani --- gdbserver/dll.cc | 9 --------- gdbserver/dll.h | 1 - gdbserver/inferiors.cc | 15 --------------- gdbserver/inferiors.h | 6 ------ gdbserver/win32-low.cc | 11 +++++------ 5 files changed, 5 insertions(+), 37 deletions(-) diff --git a/gdbserver/dll.cc b/gdbserver/dll.cc index ff637a03fed1..f49aa560aec5 100644 --- a/gdbserver/dll.cc +++ b/gdbserver/dll.cc @@ -89,12 +89,3 @@ unloaded_dll (process_info *proc, const char *name, CORE_ADDR base_addr) proc->dlls_changed = true; } } - -void -clear_dlls (void) -{ - for_each_process ([] (process_info *proc) - { - proc->all_dlls.clear (); - }); -} diff --git a/gdbserver/dll.h b/gdbserver/dll.h index 30c6a2c65d06..e603df4be2b2 100644 --- a/gdbserver/dll.h +++ b/gdbserver/dll.h @@ -32,7 +32,6 @@ struct dll_info CORE_ADDR base_addr; }; -extern void clear_dlls (void); extern void loaded_dll (const char *name, CORE_ADDR base_addr); extern void loaded_dll (process_info *proc, const char *name, CORE_ADDR base_addr); diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc index 18d22cdae61d..6134d3be3fe8 100644 --- a/gdbserver/inferiors.cc +++ b/gdbserver/inferiors.cc @@ -130,21 +130,6 @@ set_thread_regcache_data (struct thread_info *thread, struct regcache *data) thread->regcache_data = data; } -void -clear_inferiors (void) -{ - for_each_process ([] (process_info *process) - { - process->thread_list ().clear (); - process->thread_map ().clear (); - }); - - clear_dlls (); - - switch_to_thread (nullptr); - current_process_ = nullptr; -} - struct process_info * add_process (int pid, int attached) { diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h index a197654e8556..3aafc693be4e 100644 --- a/gdbserver/inferiors.h +++ b/gdbserver/inferiors.h @@ -93,10 +93,6 @@ struct process_info : public intrusive_list_node owning_intrusive_list &thread_list () { return m_thread_list; } - /* Return a reference to the private thread map. */ - std::unordered_map &thread_map () - { return m_ptid_thread_map; } - /* Return the number of threads in this process. */ unsigned int thread_count () const { return m_ptid_thread_map.size (); } @@ -160,8 +156,6 @@ int have_attached_inferiors_p (void); /* Switch to a thread of PROC. */ void switch_to_process (process_info *proc); -void clear_inferiors (void); - void *thread_target_data (struct thread_info *); struct regcache *thread_regcache_data (struct thread_info *); void set_thread_regcache_data (struct thread_info *, struct regcache *); diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc index e36bcc4f44a9..9e4108e64a2b 100644 --- a/gdbserver/win32-low.cc +++ b/gdbserver/win32-low.cc @@ -676,7 +676,7 @@ gdbserver_windows_process::handle_output_debug_string } static void -win32_clear_inferiors (void) +win32_clear_process () { if (windows_process.open_process_used) { @@ -686,7 +686,6 @@ win32_clear_inferiors (void) for_each_thread (delete_thread_info); windows_process.siginfo_er.ExceptionCode = 0; - clear_inferiors (); } /* Implementation of target_ops::kill. */ @@ -709,9 +708,9 @@ win32_process_target::kill (process_info *process) windows_process.handle_output_debug_string (nullptr); } - win32_clear_inferiors (); - + win32_clear_process (); remove_process (process); + return 0; } @@ -730,7 +729,7 @@ win32_process_target::detach (process_info *process) return -1; DebugSetProcessKillOnExit (FALSE); - win32_clear_inferiors (); + win32_clear_process (); remove_process (process); return 0; @@ -1219,7 +1218,7 @@ win32_process_target::wait (ptid_t ptid, target_waitstatus *ourstatus, case TARGET_WAITKIND_EXITED: OUTMSG2 (("Child exited with retcode = %x\n", ourstatus->exit_status ())); - win32_clear_inferiors (); + win32_clear_process (); return ptid_t (windows_process.current_event.dwProcessId); case TARGET_WAITKIND_STOPPED: case TARGET_WAITKIND_SIGNALLED: