clone 571423 -1 reassign -1 perl-tk 1:804.028-6 retitle -1 perl-tk: broken select mask on 64 bit architectures severity -1 important tags -1 patch fixed-upstream block 571423 by -1 submitter -1 ! thanks
On Wed, Mar 17, 2010 at 07:34:44PM +0100, gregor herrmann wrote: > On Wed, 17 Mar 2010 17:12:39 +0100, Jozef Kutej wrote: > > > the tests failed with: > > perl-tk/squeeze uptodate 1:804.028-6 > > but after I've installed by hand: > > http://search.cpan.org/CPAN/authors/id/S/SR/SREZIC/Tk-804.028_502.tar.gz > > all tests passed. > > Now that's interesting, thanks for your analysis! > > I'm CC'ing the perl-tk maintainer to get his attention for this > issue. This is also reported as [rt.cpan.org #32034]. I'm attaching the relevant patch: URL: https://svn.perl.org/modules/Tk/trunk r10469 | srezic | 2008-01-02 01:01:50 +0200 (Wed, 02 Jan 2008) | 6 lines r1...@biokovo-amd64: eserte | 2008-01-01 22:47:35 +0100 * manually applied the tcl repo diff between 1.11.2.4 and 1.11.2.5 This seems to solve the POE problem reported in http://rt.cpan.org/Ticket/Display.html?id=32034 * new fileevent2.t test for checking this problem I've verified that this fixes libpoe-loop-tk-perl for me on amd64. Cloning, reassigning and blocking accordingly. -- Niko Tyni nt...@debian.org
URL: https://svn.perl.org/modules/Tk/trunk r10469 | srezic | 2008-01-02 01:01:50 +0200 (Wed, 02 Jan 2008) | 6 lines r1...@biokovo-amd64: eserte | 2008-01-01 22:47:35 +0100 * manually applied the tcl repo diff between 1.11.2.4 and 1.11.2.5 This seems to solve the POE problem reported in http://rt.cpan.org/Ticket/Display.html?id=32034 * new fileevent2.t test for checking this problem Index: t/fileevent2.t =================================================================== --- t/fileevent2.t (revision 0) +++ t/fileevent2.t (revision 10469) @@ -0,0 +1,49 @@ +#!/usr/bin/perl -w +# -*- perl -*- + +# +# $Id: $ +# Author: Slaven Rezic +# + +use strict; + +use Tk; + +BEGIN { + if (!eval q{ + use 5.006; # three-arg open + use Test::More; + 1; + }) { + print "1..0 # skip: no Test::More module\n"; + exit; + } +} + +plan tests => 1; + +my @fh; +my $callback_called = 0; + +my $mw = tkinit; +$mw->geometry("+10+10"); +$mw->idletasks; + +# A variant of the problem reported in +# http://rt.cpan.org/Ticket/Display.html?id=32034 +# +# tclUnixNotify.c used to do bit-handling for the select() mask +# itself, but this was broken for 64bit machines. +for (1..100) { + open my $dup, "<&", \*STDIN or die "Can't dup STDIN: $!"; + push @fh, $dup; + $mw->fileevent($dup, "readable", sub { $callback_called++ }); +} + +$mw->after(300, sub { $mw->destroy }); +MainLoop; + +ok($callback_called == 0, "Fileevent callback should never be called"); + +__END__ Property changes on: t/fileevent2.t ___________________________________________________________________ Added: svn:executable + * Index: MANIFEST =================================================================== --- MANIFEST (revision 10468) +++ MANIFEST (revision 10469) @@ -1962,6 +1962,7 @@ t/exefiles.t t/fbox.t t/fileevent.t +t/fileevent2.t t/fileselect.t t/font.t t/fork.t Index: pTk/mTk/tclUnix/tclUnixNotfy.c =================================================================== --- pTk/mTk/tclUnix/tclUnixNotfy.c (revision 10468) +++ pTk/mTk/tclUnix/tclUnixNotfy.c (revision 10469) @@ -69,6 +69,18 @@ } FileHandlerEvent; /* + * + * The following structure contains a set of select() masks to track + * readable, writable, and exceptional conditions. + */ + +typedef struct SelectMasks { + fd_set readable; + fd_set writable; + fd_set exceptional; +} SelectMasks; + +/* * The following static structure contains the state information for the * select based implementation of the Tcl notifier. One of these structures * is created for each thread that is using the notifier. @@ -77,13 +89,12 @@ typedef struct ThreadSpecificData { FileHandler *firstFileHandlerPtr; /* Pointer to head of file handler list. */ - fd_mask checkMasks[3*MASK_SIZE]; - /* This array is used to build up the masks + + SelectMasks checkMasks; /* This structure is used to build up the masks * to be used in the next call to select. * Bits are set in response to calls to * Tcl_CreateFileHandler. */ - fd_mask readyMasks[3*MASK_SIZE]; - /* This array reflects the readable/writable + SelectMasks readyMasks; /* This array reflects the readable/writable * conditions that were found to exist by the * last call to select. */ int numFdBits; /* Number of valid bits in checkMasks @@ -425,7 +436,6 @@ { ThreadSpecificData *tsdPtr = TCL_TSD_INIT(&dataKey); FileHandler *filePtr; - int index, bit; if (tclStubs.tcl_CreateFileHandler != tclOriginalNotifier.createFileHandlerProc) { tclStubs.tcl_CreateFileHandler(fd, mask, proc, clientData); @@ -453,22 +463,20 @@ * Update the check masks for this file. */ - index = fd/(NBBY*sizeof(fd_mask)); - bit = 1 << (fd%(NBBY*sizeof(fd_mask))); - if (mask & TCL_READABLE) { - tsdPtr->checkMasks[index] |= bit; + if ( mask & TCL_READABLE ) { + FD_SET( fd, &(tsdPtr->checkMasks.readable) ); } else { - tsdPtr->checkMasks[index] &= ~bit; + FD_CLR( fd, &(tsdPtr->checkMasks.readable) ); } - if (mask & TCL_WRITABLE) { - (tsdPtr->checkMasks+MASK_SIZE)[index] |= bit; + if ( mask & TCL_WRITABLE ) { + FD_SET( fd, &(tsdPtr->checkMasks.writable) ); } else { - (tsdPtr->checkMasks+MASK_SIZE)[index] &= ~bit; + FD_CLR( fd, &(tsdPtr->checkMasks.writable) ); } - if (mask & TCL_EXCEPTION) { - (tsdPtr->checkMasks+2*(MASK_SIZE))[index] |= bit; + if ( mask & TCL_EXCEPTION ) { + FD_SET( fd, &(tsdPtr->checkMasks.exceptional) ); } else { - (tsdPtr->checkMasks+2*(MASK_SIZE))[index] &= ~bit; + FD_CLR( fd, &(tsdPtr->checkMasks.exceptional) ); } if (tsdPtr->numFdBits <= fd) { tsdPtr->numFdBits = fd+1; @@ -497,8 +505,7 @@ int fd; /* Stream id for which to remove callback procedure. */ { FileHandler *filePtr, *prevPtr; - int index, bit, i; - unsigned long flags; + int i; ThreadSpecificData *tsdPtr = TCL_TSD_INIT(&dataKey); if (tclStubs.tcl_DeleteFileHandler != tclOriginalNotifier.deleteFileHandlerProc) { @@ -524,17 +531,14 @@ * Update the check masks for this file. */ - index = fd/(NBBY*sizeof(fd_mask)); - bit = 1 << (fd%(NBBY*sizeof(fd_mask))); - if (filePtr->mask & TCL_READABLE) { - tsdPtr->checkMasks[index] &= ~bit; + FD_CLR( fd, &(tsdPtr->checkMasks.readable) ); } if (filePtr->mask & TCL_WRITABLE) { - (tsdPtr->checkMasks+MASK_SIZE)[index] &= ~bit; + FD_CLR( fd, &(tsdPtr->checkMasks.writable) ); } if (filePtr->mask & TCL_EXCEPTION) { - (tsdPtr->checkMasks+2*(MASK_SIZE))[index] &= ~bit; + FD_CLR( fd, &(tsdPtr->checkMasks.exceptional) ); } /* @@ -542,17 +546,12 @@ */ if (fd+1 == tsdPtr->numFdBits) { - for (tsdPtr->numFdBits = 0; index >= 0; index--) { - flags = tsdPtr->checkMasks[index] - | (tsdPtr->checkMasks+MASK_SIZE)[index] - | (tsdPtr->checkMasks+2*(MASK_SIZE))[index]; - if (flags) { - for (i = (NBBY*sizeof(fd_mask)); i > 0; i--) { - if (flags & (((unsigned long)1) << (i-1))) { - break; - } - } - tsdPtr->numFdBits = index * (NBBY*sizeof(fd_mask)) + i; + tsdPtr->numFdBits = 0; + for (i = fd-1; i >= 0; i--) { + if ( FD_ISSET( i, &(tsdPtr->checkMasks.readable) ) + || FD_ISSET( i, &(tsdPtr->checkMasks.writable) ) + || FD_ISSET( i, &(tsdPtr->checkMasks.exceptional ) ) ) { + tsdPtr->numFdBits = i+1; break; } } @@ -674,7 +673,7 @@ FileHandler *filePtr; FileHandlerEvent *fileEvPtr; struct timeval timeout, *timeoutPtr; - int bit, index, mask; + int mask; #ifdef TCL_THREADS int waitForFiles; #else @@ -756,7 +755,9 @@ write(triggerPipe, "", 1); } - memset((VOID *) tsdPtr->readyMasks, 0, 3*MASK_SIZE*sizeof(fd_mask)); + FD_ZERO( &(tsdPtr->readyMasks.readable) ); + FD_ZERO( &(tsdPtr->readyMasks.writable) ); + FD_ZERO( &(tsdPtr->readyMasks.exceptional) ); if (!tsdPtr->eventReady) { Tcl_ConditionWait(&tsdPtr->waitCV, ¬ifierMutex, timePtr); @@ -786,12 +787,12 @@ #else - memcpy((VOID *) tsdPtr->readyMasks, (VOID *) tsdPtr->checkMasks, - 3*MASK_SIZE*sizeof(fd_mask)); - numFound = select(tsdPtr->numFdBits, - (SELECT_MASK *) &tsdPtr->readyMasks[0], - (SELECT_MASK *) &tsdPtr->readyMasks[MASK_SIZE], - (SELECT_MASK *) &tsdPtr->readyMasks[2*MASK_SIZE], timeoutPtr); + tsdPtr->readyMasks = tsdPtr->checkMasks; + numFound = select( tsdPtr->numFdBits, + &(tsdPtr->readyMasks.readable), + &(tsdPtr->readyMasks.writable), + &(tsdPtr->readyMasks.exceptional), + timeoutPtr ); /* * Some systems don't clear the masks after an error, so @@ -799,7 +800,9 @@ */ if (numFound == -1) { - memset((VOID *) tsdPtr->readyMasks, 0, 3*MASK_SIZE*sizeof(fd_mask)); + FD_ZERO( &(tsdPtr->readyMasks.readable ) ); + FD_ZERO( &(tsdPtr->readyMasks.writable ) ); + FD_ZERO( &(tsdPtr->readyMasks.exceptional ) ); } #ifdef _LANG @@ -819,17 +822,15 @@ for (filePtr = tsdPtr->firstFileHandlerPtr; (filePtr != NULL); filePtr = filePtr->nextPtr) { - index = filePtr->fd / (NBBY*sizeof(fd_mask)); - bit = 1 << (filePtr->fd % (NBBY*sizeof(fd_mask))); + mask = 0; - - if (tsdPtr->readyMasks[index] & bit) { + if ( FD_ISSET( filePtr->fd, &(tsdPtr->readyMasks.readable) ) ) { mask |= TCL_READABLE; } - if ((tsdPtr->readyMasks+MASK_SIZE)[index] & bit) { + if ( FD_ISSET( filePtr->fd, &(tsdPtr->readyMasks.writable) ) ) { mask |= TCL_WRITABLE; } - if ((tsdPtr->readyMasks+2*(MASK_SIZE))[index] & bit) { + if ( FD_ISSET( filePtr->fd, &(tsdPtr->readyMasks.exceptional) ) ) { mask |= TCL_EXCEPTION; } @@ -888,13 +889,13 @@ ClientData clientData; /* Not used. */ { ThreadSpecificData *tsdPtr; - fd_mask masks[3*MASK_SIZE]; - long *maskPtr = (long *)masks; /* masks[] cast to type long[] */ + fd_set readableMask; + fd_set writableMask; + fd_set exceptionalMask; int fds[2]; - int i, status, index, bit, numFdBits, receivePipe; - long found, word; + int i, status, numFdBits, receivePipe; + long found; struct timeval poll = {0., 0.}, *timePtr; - int maskSize = 3 * ((MASK_SIZE) / sizeof(long)) * sizeof(fd_mask); char buf[2]; if (pipe(fds) != 0) { @@ -942,27 +943,31 @@ */ while (1) { - /* - * Set up the select mask to include the receive pipe. - */ - memset((VOID *)masks, 0, 3*MASK_SIZE*sizeof(fd_mask)); - numFdBits = receivePipe + 1; - index = receivePipe / (NBBY*sizeof(fd_mask)); - bit = 1 << (receivePipe % (NBBY*sizeof(fd_mask))); - masks[index] |= bit; + FD_ZERO( &readableMask ); + FD_ZERO( &writableMask ); + FD_ZERO( &exceptionalMask ); /* - * Add in the check masks from all of the waiting notifiers. + * Compute the logical OR of the select masks from all the + * waiting notifiers. */ Tcl_MutexLock(¬ifierMutex); timePtr = NULL; - for (tsdPtr = waitingListPtr; tsdPtr; tsdPtr = tsdPtr->nextPtr) { - for (i = 0; i < maskSize; i++) { - maskPtr[i] |= ((long*)tsdPtr->checkMasks)[i]; + for (tsdPtr = waitingListPtr; tsdPtr; tsdPtr = tsdPtr->nextPtr) { + for ( i = tsdPtr->numFdBits-1; i >= 0; --i ) { + if ( FD_ISSET( i, &(tsdPtr->checkMasks.readable) ) ) { + FD_SET( i, &readableMask ); + } + if ( FD_ISSET( i, &(tsdPtr->checkMasks.writable) ) ) { + FD_SET( i, &writableMask ); + } + if ( FD_ISSET( i, &(tsdPtr->checkMasks.exceptional) ) ) { + FD_SET( i, &exceptionalMask ); + } } - if (tsdPtr->numFdBits > numFdBits) { + if ( tsdPtr->numFdBits > numFdBits ) { numFdBits = tsdPtr->numFdBits; } if (tsdPtr->pollState & POLL_WANT) { @@ -977,11 +982,17 @@ } Tcl_MutexUnlock(¬ifierMutex); - maskSize = 3 * ((MASK_SIZE) / sizeof(long)) * sizeof(fd_mask); + /* + * Set up the select mask to include the receive pipe. + */ - if (select(numFdBits, (SELECT_MASK *) &masks[0], - (SELECT_MASK *) &masks[MASK_SIZE], - (SELECT_MASK *) &masks[2*MASK_SIZE], timePtr) == -1) { + if ( receivePipe >= numFdBits ) { + numFdBits = receivePipe + 1; + } + FD_SET( receivePipe, &readableMask ); + + if ( select( numFdBits, &readableMask, &writableMask, + &exceptionalMask, timePtr) == -1 ) { /* * Try again immediately on an error. */ @@ -997,11 +1008,24 @@ for (tsdPtr = waitingListPtr; tsdPtr; tsdPtr = tsdPtr->nextPtr) { found = 0; - for (i = 0; i < maskSize; i++) { - word = maskPtr[i] & ((long*)tsdPtr->checkMasks)[i]; - found |= word; - (((long*)(tsdPtr->readyMasks))[i]) = word; + for ( i = tsdPtr->numFdBits-1; i >= 0; --i ) { + if ( FD_ISSET( i, &(tsdPtr->checkMasks.readable) ) + && FD_ISSET( i, &readableMask ) ) { + FD_SET( i, &(tsdPtr->readyMasks.readable) ); + found = 1; + } + if ( FD_ISSET( i, &(tsdPtr->checkMasks.writable) ) + && FD_ISSET( i, &writableMask ) ) { + FD_SET( i, &(tsdPtr->readyMasks.writable) ); + found = 1; + } + if ( FD_ISSET( i, &(tsdPtr->checkMasks.exceptional) ) + && FD_ISSET( i, &exceptionalMask ) ) { + FD_SET( i, &(tsdPtr->readyMasks.exceptional) ); + found = 1; + } } + if (found || (tsdPtr->pollState & POLL_DONE)) { tsdPtr->eventReady = 1; if (tsdPtr->onList) { @@ -1035,7 +1059,7 @@ * to avoid a race condition we only read one at a time. */ - if (masks[index] & bit) { + if ( FD_ISSET( receivePipe, &readableMask ) ) { i = read(receivePipe, buf, 1); if ((i == 0) || ((i == 1) && (buf[0] == 'q'))) { Property changes on: . ___________________________________________________________________ Modified: svk:merge - 8c7cb07b-dc62-db11-ac52-0211d89195f5:/local/Tk:1481 8c7cb07b-dc62-db11-ac52-0211d89195f5:/local/Tk-at-perl/trunk:1991 + 8c7cb07b-dc62-db11-ac52-0211d89195f5:/local/Tk:1481 8c7cb07b-dc62-db11-ac52-0211d89195f5:/local/Tk-at-perl/trunk:1993
