-
Notifications
You must be signed in to change notification settings - Fork 489
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
2 changed files
with
81 additions
and
4 deletions.
There are no files selected for viewing
74 changes: 74 additions & 0 deletions
74
msys2-runtime/0042-cygthread-suspend-thread-before-terminating.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| From e09c64ef65cf51011122e55b26abdbda9e70f7e6 Mon Sep 17 00:00:00 2001 | ||
| From: Jeremy Drake <[email protected]> | ||
| Date: Mon, 11 Nov 2024 20:09:49 -0800 | ||
| Subject: [PATCH 42/N] cygthread: suspend thread before terminating. | ||
|
|
||
| This addresses an extremely difficult to debug deadlock when running | ||
| under emulation on ARM64. | ||
|
|
||
| A relatively easy way to trigger this bug is to call `fork()`, then within the | ||
| child process immediately call another `fork()` and then `exit()` the | ||
| intermediate process. | ||
|
|
||
| It would seem that there is a "code emulation" lock on the wait thread at | ||
| this stage, and if the thread is terminated too early, that lock still exists | ||
| albeit without a thread, and nothing moves forward. | ||
|
|
||
| It seems that a `SuspendThread()` combined with a `GetThreadContext()` | ||
| (to force the thread to _actually_ be suspended, for more details see | ||
| https://devblogs.microsoft.com/oldnewthing/20150205-00/?p=44743) | ||
| makes sure the thread is "booted" from emulation before it is suspended. | ||
|
|
||
| Hopefully this means it won't be holding any locks or otherwise leave | ||
| emulation in a bad state when the thread is terminated. | ||
|
|
||
| Also, attempt to use `CancelSynchonousIo()` (as seen in `flock.cc`) to avoid | ||
| the need for `TerminateThread()` altogether. This doesn't always work, | ||
| however, so was not a complete fix for the deadlock issue. | ||
|
|
||
| Addresses: https://cygwin.com/pipermail/cygwin-developers/2024-May/012694.html | ||
| Signed-off-by: Jeremy Drake <[email protected]> | ||
| --- | ||
| winsup/cygwin/cygthread.cc | 14 ++++++++++++++ | ||
| winsup/cygwin/sigproc.cc | 3 ++- | ||
| 2 files changed, 16 insertions(+), 1 deletion(-) | ||
|
|
||
| diff --git a/winsup/cygwin/cygthread.cc b/winsup/cygwin/cygthread.cc | ||
| index 54918e7..4f16097 100644 | ||
| --- a/winsup/cygwin/cygthread.cc | ||
| +++ b/winsup/cygwin/cygthread.cc | ||
| @@ -302,6 +302,20 @@ cygthread::terminate_thread () | ||
| if (!inuse) | ||
| goto force_notterminated; | ||
|
|
||
| + if (_my_tls._ctinfo != this) | ||
| + { | ||
| + CONTEXT context; | ||
| + context.ContextFlags = CONTEXT_CONTROL; | ||
| + /* SuspendThread makes sure a thread is "booted" from emulation before | ||
| + it is suspended. As such, the emulator hopefully won't be in a bad | ||
| + state (aka, holding any locks) when the thread is terminated. */ | ||
| + SuspendThread (h); | ||
| + /* We need to call GetThreadContext, even though we don't care about the | ||
| + context, because SuspendThread is asynchronous and GetThreadContext | ||
| + will make sure the thread is *really* suspended before returning */ | ||
| + GetThreadContext (h, &context); | ||
| + } | ||
| + | ||
| TerminateThread (h, 0); | ||
| WaitForSingleObject (h, INFINITE); | ||
| CloseHandle (h); | ||
| diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc | ||
| index a89f09d..0089626 100644 | ||
| --- a/winsup/cygwin/sigproc.cc | ||
| +++ b/winsup/cygwin/sigproc.cc | ||
| @@ -410,7 +410,8 @@ proc_terminate () | ||
| if (!have_execed || !have_execed_cygwin) | ||
| chld_procs[i]->ppid = 1; | ||
| if (chld_procs[i].wait_thread) | ||
| - chld_procs[i].wait_thread->terminate_thread (); | ||
| + if (!CancelSynchronousIo (chld_procs[i].wait_thread->thread_handle ())) | ||
| + chld_procs[i].wait_thread->terminate_thread (); | ||
| /* Release memory associated with this process unless it is 'myself'. | ||
| 'myself' is only in the chld_procs table when we've execed. We | ||
| reach here when the next process has finished initializing but we |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters