Re: [ANNOUNCEMENT] TEST: Cygwin 3.1.0-0.1

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Takashi,

On Aug 15 17:09, Corinna Vinschen wrote:
> On Aug 15 17:04, Corinna Vinschen wrote:
> > On Aug 15 12:36, Corinna Vinschen wrote:
> > > On Aug 15 09:49, Corinna Vinschen wrote:
> > > > On Aug 15 04:21, Takashi Yano wrote:
> > > > > On Wed, 14 Aug 2019 15:49:00 +0200
> > > > > Corinna Vinschen wrote:
> > > > > > The only reason I can see is if sigwait_common() returns EINTR because
> > > > > > it was interrupted by an unrelated signal.  This in turn lets the read()
> > > > > > call fail with EINTR and that should be expected by the callers, in
> > > > > > theory.
> > > > > 
> > > > > Strangely, this problem also disappears with this patch.
> > > > > 
> > > > > diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
> > > > > index 9cf892801..82ac0674f 100644
> > > > > --- a/winsup/cygwin/select.cc
> > > > > +++ b/winsup/cygwin/select.cc
> > > > > @@ -1869,7 +1869,7 @@ thread_signalfd (void *arg)
> > > > >        switch (WaitForSingleObject (si->evt, INFINITE))
> > > > >         {
> > > > >         case WAIT_OBJECT_0:
> > > > > -         tls->signalfd_select_wait = NULL;
> > > > > +         //tls->signalfd_select_wait = NULL;
> > > > >           event = true;
> > > > >           break;
> > > > >         default:
> > > > 
> > > > The problem with not setting signalfd_select_wait to NULL here is that
> > > > only a subsequent read or sigwaitinfo will do, so there's a time
> > > > post-select which will reroute the signal wrongly.
> > > 
> > > Worse, thread_signalfd() closes the handle on exit, so keeping
> > > signalfd_select_wait set may result in strange behaviour after select
> > > returns.
> > > 
> > > > Any ideas greatly appreciated.
> > [...]

I now had an idea, but I'm not entirely sure if it's the right thing to
do.  Can you please test this?  It consists of two patches, one with the
revamped signalfd handling, and one with the revert of the signalfd
patch I applied a couple of days ago.

Quick description: I dropped signalfd_select_wait entirely.  Instead,
wait_sig sets or resets a manual event object to indicate if there are
signals pending in the queue, even after trying to handle them the
normal way.  That usually means they are blocked.

select() uses the event to wake up from WFMO, if at least one signalfd
is present in the read descriptor set.  The rest is done via the peek
and verify functions in select, which basically just check if this
signalfd is waiting for one of the pending signals.

The reversion of my patch from a couple days ago is not required as
such, but after thinking about this a while I'm convinced that this was
just me not getting the full picture.  Also, reverting this patch would
revert to seeing a SEGV in your testcase and thus a bug in the new code,
too.

I attached both patches.  It would be pretty nice if you could test them
and point out any problems you get with this new code.

Please note that you should ideally perform a full rebuild due to the
slight change in TLS layout.


Thanks a lot,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Attachment: 0001-Cygwin-select-revamp-non-polling-code-for-signalfd.patch
Description: Text document

Attachment: 0002-Revert-Cygwin-fix-potential-SEGV-in-sigwaitinfo-sign.patch
Description: Text document

Attachment: signature.asc
Description: PGP signature