[PATCH v2] Cygwin: signal: Do not handle signal when __SIGFLUSHFAST is sent

[Takashi Yano]

Hi Corinna,

On Sun, 19 Jan 2025 11:49:58 +0900
Takashi Yano wrote:
> On Sat, 18 Jan 2025 17:06:50 -0800 (PST)
> Jeremy Drake wrote:
> > On Sat, 18 Jan 2025, Takashi Yano wrote:
> > 
> > > While debugging this problem, I encountered another hang issue,
> > > which is fixed by:
> > > 0001-Cygwin-signal-Avoid-frequent-tls-lock-unlock-for-SIG.patch
> > 
> > I'm concerned about this patch.  There's a window where current_sig could
> > be changed after exiting the while, before the lock is acquired by
> > cygheap->find_tls (_main_tls);  Should current_sig be rechecked after the
> > lock is acquired to make sure that hasn't happened?  Also, does
> > current_sig need to be volatile, or is yield a sufficient fence for the
> > compiler to know other threads may have changed the value?
> 
> Thanks for pointing out this. You are right if othre threads may
> set current_sig to non-zero value. Current cygwin sets current_sig
> to non-zero only in 
> _cygtls::interrupt_setup()
> and
> _cygtls::handle_SIGCONT()
> both are called from sigpacket::process() as follows.
> 
> wait_sig()->
>  sigpacket::process() +-> sigpacket::setup_handler() -> _cygtls::interrupt_setup()
>                       \-> _cygtls::handle_SIGCONT()
> 
> wait_sig() is a thread which handle received signals, so other
> threads than wait_sig() thread do not set the current_sig to non-zero.
> That is, other threads set current_sig only to zero. Therefore,
> I think we don't have to guard checking current_sig value by lock.
> The only thing we shoud guard is the following case.
> 
> [wait_sig()]               [another thread]
> current_sig = SIGCONT;
>                            current_sig = 0;
> set_signal_arrived();
> 
> So, we should place current_sig = SIGCONT and set_signal_arrived()
> inside the lock.

I think the lock necessary here is _cygtls::lock(), isn't it?
Because the _cygtls::call_signal_handler() uses _cygtls::locl().
I'm asking you because you introduced the find_tls() lock first
in the commit:

commit 26158dc3e9c20fc0488944f0c3eefdc19255e7da
Author: Corinna Vinschen <corinna@vinschen.de>
Date:   Fri Nov 28 20:46:13 2014 +0000

            * cygheap.cc (init_cygheap::init_tls_list): Accommodate threadlist
            having a new type threadlist_t *.  Convert commented out code into an
            #if 0.  Create thread mutex.  Explain why.
            (init_cygheap::remove_tls): Drop timeout value.  Always wait infinitely
            for tls_sentry.  Return mutex HANDLE of just deleted threadlist entry.
            (init_cygheap::find_tls): New implementation taking tls pointer as
            search parameter.  Return threadlist_t *.
            (init_cygheap::find_tls): Return threadlist_t *.  Define ix as auto
            variable.  Drop exception handling since crash must be made impossible
            due to correct synchronization.  Return with locked mutex.
            * cygheap.h (struct threadlist_t): Define.
            (struct init_cygheap): Convert threadlist to threadlist_t type.
            (init_cygheap::remove_tls): Align declaration to above change.
            (init_cygheap::find_tls): Ditto.
            (init_cygheap::unlock_tls): Define.
            * cygtls.cc (_cygtls::remove): Unlock and close mutex when finishing.
            * exceptions.cc (sigpacket::process): Lock _cygtls area of thread before
            accessing it.
            * fhandler_termios.cc (fhandler_termios::bg_check): Ditto.
            * sigproc.cc (sig_send): Ditto.
            * thread.cc (pthread::exit): Ditto.  Add comment.
            (pthread::cancel): Ditto.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>