Bram Moolenaar wrote: > Dominique Pelle wrote: > >> Attached patch fixes a race condition which causes Vim to hang >> when resuming (with fg) after having suspended Vim with CTRL-Z. >> Bug does not happen 100% of the time, but once in a while. I >> can reproduce the bug with Vim-7.2.121 in a xterm on Linux x86 >> after doing >> >> CTRL-Z >> fg >> CTRL-Z >> fg >> , etc, ... several times until Vim hangs. >> >> Pressing CTRL-C is a workaround when it hangs. >> >> See also Ubuntu bug https://bugs.launchpad.net/bugs/291373 >> >> Bug only happens when configure script defined -D_REENTRANT >> which happens at least when I configure Vim as follows: >> >> ./configure --enable-tclinterp --enable-rubyinterp \ >> --enable-perlinterp --enable-pythoninterp \ >> --with-features=huge --enable-gui=gnome2 >> >> The problem happens I think because the following 2 lines >> in os_unix.c in mch_suspect() have a race condition: >> >> if (!sigcont_received) >> pause(); >> >> If the signal is handled _after_ the test of sigcont_received >> variable but _before_ pause() is executed, then pause() will >> wait for ever since signal already happened. >> >> Also I think that the variable sigcont_received should also be >> declared volatile since it's modified by a signal handler. >> >> Attached patch fixes the race condition by waiting until >> sigcont_received is set without calling pause(): >> >> long wait; >> for (wait = 0; !sigcont_received && wait <= 3L; wait++) >> /* Loop is not entered most of the time */ >> mch_delay(wait, FALSE); >> >> Note that most of the time the loop does not even need >> to sleep() at all (sigcont_received is already set most of >> the time before entering the loop). And when it it sleeps, >> a sleep(0) seems always enough in practice. I've put >> debug printf() after the loop that waits for sigcont_received >> and I see this when doing several suspend (CTRL-Z, fg): >> >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=1 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=1 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> **** wait=0 >> >> Notice that in 2 occasions, wait was 1 (meaning that the loop >> iterated once with a sleep of 0 (yielding the CPU which is >> enough to get the signal handled). > > Makes sense. Perhaps pause() does some work that increases the change > for the race condition to actually happen (e.g., a aquiring a lock). > > One problem: the Vim code doesn't use volatile yet. I don't think every > C compiler supports it, thus this requires adding a configure check. > AC_C_VOLATILE will do.
OK. I'm no expert with configure scripts, but I assume that what you're asking is what the extra attached patch does. Without marking the variable volatile, there is a risk I think, that some compilers might optimize making code incorrect sometimes by not seeing that the variable gets automagically updated asynchronously by a signal handler as explained here for example: http://www.linuxdevices.com/articles/AT5980346182.html These kind of bugs can be subtle and hard to reproduce. I also see other variables in Vim which are modified or read by signal handlers. They should in theory also be declared volatile: - do_resize (modified by signal handler sig_winch()) - got_int (modified by signal handler catch_sigint()) - sig_alarm_called (modified by signal handler sig_alarm()) - full_screen (modified by signal handler deathtrap()) - in_mch_delay (read by signal handler deathtrap()) - lc_active (read by signal handler deathtrap()) - sigcont_received (modified by signal handler sigcont_handler()) (maybe others) Cheers -- Dominique --~--~---------~--~----~------------~-------~--~----~ You received this message from the "vim_dev" maillist. For more information, visit http://www.vim.org/maillist.php -~----------~----~----~----~------~----~------~--~---
Index: configure.in =================================================================== RCS file: /cvsroot/vim/vim7/src/configure.in,v retrieving revision 1.66 diff -c -r1.66 configure.in *** configure.in 20 Nov 2008 09:36:35 -0000 1.66 --- configure.in 22 Feb 2009 22:35:28 -0000 *************** *** 2148,2153 **** --- 2148,2154 ---- dnl Checks for typedefs, structures, and compiler characteristics. AC_PROG_GCC_TRADITIONAL AC_C_CONST + AC_C_VOLATILE AC_TYPE_MODE_T AC_TYPE_OFF_T AC_TYPE_PID_T Index: config.h.in =================================================================== RCS file: /cvsroot/vim/vim7/src/config.h.in,v retrieving revision 1.11 diff -c -r1.11 config.h.in *** config.h.in 24 Jun 2008 21:47:46 -0000 1.11 --- config.h.in 22 Feb 2009 22:35:28 -0000 *************** *** 50,55 **** --- 50,58 ---- /* Define to empty if the keyword does not work. */ #undef const + /* Define to empty if the keyword does not work. */ + #undef volatile + /* Define to `int' if <sys/types.h> doesn't define. */ #undef mode_t
