dscho hooked me up with a workflow to run Git for Windows' test suite on
an msys2-runtime pull request's CI artifacts. With this patch and the
three you pushed (reverting the v2 patch we had applied already, and
applying the two from the cygwin-3_5-branch), the test suite no longer
hangs.
On Mon, 20 Jan 2025, Takashi Yano wrote:
> It seems that current _cygtls::handle_SIGCONT() code sometimes falls
> into a deadlock due to frequent TLS lock/unlock operation in the
> yield() loop. With this patch, the yield() in the wait loop is placed
> outside the TLS lock to avoid frequent TLS lock/unlock.
>
> Fixes: 9ae51bcc51a7 ("Cygwin: signal: Fix another deadlock between main and
> sig thread")
> Reviewed-by:
> Signed-off-by: Takashi Yano <[email protected]>
> ---
> winsup/cygwin/exceptions.cc | 36 ++++++++++-----------------
> winsup/cygwin/local_includes/cygtls.h | 4 +--
> 2 files changed, 15 insertions(+), 25 deletions(-)
>
> diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
> index 4dc4be278..f576c5ff2 100644
> --- a/winsup/cygwin/exceptions.cc
> +++ b/winsup/cygwin/exceptions.cc
> @@ -1420,7 +1420,7 @@ api_fatal_debug ()
>
> /* Attempt to carefully handle SIGCONT when we are stopped. */
> void
> -_cygtls::handle_SIGCONT (threadlist_t * &tl_entry)
> +_cygtls::handle_SIGCONT ()
> {
> if (NOTSTATE (myself, PID_STOPPED))
> return;
> @@ -1431,23 +1431,17 @@ _cygtls::handle_SIGCONT (threadlist_t * &tl_entry)
> Make sure that any pending signal is handled before trying to
> send a new one. Then make sure that SIGCONT has been recognized
> before exiting the loop. */
> - bool sigsent = false;
> - while (1)
> - if (current_sig) /* Assume that it's ok to just test sig outside of a
> - lock since setup_handler does it this way. */
> - {
> - cygheap->unlock_tls (tl_entry);
> - yield (); /* Attempt to schedule another thread. */
> - tl_entry = cygheap->find_tls (_main_tls);
> - }
> - else if (sigsent)
> - break; /* SIGCONT has been recognized by other thread */
> - else
> - {
> - current_sig = SIGCONT;
> - set_signal_arrived (); /* alert sig_handle_tty_stop */
> - sigsent = true;
> - }
> + while (current_sig) /* Assume that it's ok to just test sig outside of a
> */
> + yield (); /* lock since setup_handler does it this way. */
> +
> + lock ();
> + current_sig = SIGCONT;
> + set_signal_arrived (); /* alert sig_handle_tty_stop */
> + unlock ();
> +
> + while (current_sig == SIGCONT)
> + yield ();
> +
> /* Clear pending stop signals */
> sig_clear (SIGSTOP, false);
> sig_clear (SIGTSTP, false);
> @@ -1479,11 +1473,7 @@ sigpacket::process ()
> myself->rusage_self.ru_nsignals++;
>
> if (si.si_signo == SIGCONT)
> - {
> - tl_entry = cygheap->find_tls (_main_tls);
> - _main_tls->handle_SIGCONT (tl_entry);
> - cygheap->unlock_tls (tl_entry);
> - }
> + _main_tls->handle_SIGCONT ();
>
> /* SIGKILL is special. It always goes through. */
> if (si.si_signo == SIGKILL)
> diff --git a/winsup/cygwin/local_includes/cygtls.h
> b/winsup/cygwin/local_includes/cygtls.h
> index 2d490646a..e0de712f4 100644
> --- a/winsup/cygwin/local_includes/cygtls.h
> +++ b/winsup/cygwin/local_includes/cygtls.h
> @@ -194,7 +194,7 @@ public: /* Do NOT remove this public: line, it's a marker
> for gentls_offsets. */
> class cygthread *_ctinfo;
> class san *andreas;
> waitq wq;
> - int current_sig;
> + volatile int current_sig;
> unsigned incyg;
> volatile unsigned stacklock;
> __tlsstack_t *stackptr;
> @@ -274,7 +274,7 @@ public: /* Do NOT remove this public: line, it's a marker
> for gentls_offsets. */
> {
> will_wait_for_signal = false;
> }
> - void handle_SIGCONT (threadlist_t * &);
> + void handle_SIGCONT ();
> static void cleanup_early(struct _reent *);
> private:
> void call2 (DWORD (*) (void *, void *), void *, void *);
>
--
Krogt, n. (chemical symbol: Kr):
The metallic silver coating found on fast-food game cards.
-- Rich Hall, "Sniglets"
