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. Thanks. > 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) I hate this kind of "blame the programmer" solutions. It means compiler writers are lazy and put the burden of getting it right on the programmer. While the actual problem is that the compiler should do this correctly automatically. The compiler can see that the variable is used in a signal handler and act accordingly. Anyway, compilers do work in this broken way, I suppose we should add these volatile keywords to avoid trouble. -- hundred-and-one symptoms of being an internet addict: 122. You ask if the Netaholics Anonymous t-shirt you ordered can be sent to you via e-mail. /// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ download, build and distribute -- http://www.A-A-P.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org /// --~--~---------~--~----~------------~-------~--~----~ You received this message from the "vim_dev" maillist. For more information, visit http://www.vim.org/maillist.php -~----------~----~----~----~------~----~------~--~---
