On Jan 19 19:42, Takashi Yano wrote: > 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:
Yeah, _cygtls::lock() of the target thread should be right. The mutex in find_tls was for guarding threadlist_t, not the thread's _cygtls. Corinna
