Bram Moolenaar wrote:
> Dominique Pelle wrote:
...snip...
>> 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.
Asynchronous things are hard to get right.
In the case of static variables, the compiler could indeed in theory
figure it out (no idea whether it actually does that). But at least in
the case of global variables such as 'got_int' compiler has no way
to know that 'got_int' is used in a signal handler in another object
file.
> Anyway, compilers do work in this broken way, I suppose we should add
> these volatile keywords to avoid trouble.
It's safer. Attached patch contains all changes made in previous
patch and declares a few more 'volatile' variables used in signal
handlers.
Thanks
-- Dominique
--~--~---------~--~----~------------~-------~--~----~
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php
-~----------~----~----~----~------~----~------~--~---
Index: os_unix.c
===================================================================
RCS file: /cvsroot/vim/vim7/src/os_unix.c,v
retrieving revision 1.90
diff -c -r1.90 os_unix.c
*** os_unix.c 22 Feb 2009 01:52:46 -0000 1.90
--- os_unix.c 24 Feb 2009 19:05:11 -0000
***************
*** 181,187 ****
&& defined(FEAT_TITLE) && !defined(FEAT_GUI_GTK)
# define SET_SIG_ALARM
static RETSIGTYPE sig_alarm __ARGS(SIGPROTOARG);
! static int sig_alarm_called;
#endif
static RETSIGTYPE deathtrap __ARGS(SIGPROTOARG);
--- 181,188 ----
&& defined(FEAT_TITLE) && !defined(FEAT_GUI_GTK)
# define SET_SIG_ALARM
static RETSIGTYPE sig_alarm __ARGS(SIGPROTOARG);
! /* volatile, used in signal handler sig_alarm(). */
! static volatile int sig_alarm_called;
#endif
static RETSIGTYPE deathtrap __ARGS(SIGPROTOARG);
***************
*** 201,213 ****
# define SIG_ERR ((RETSIGTYPE (*)())-1)
#endif
! static int do_resize = FALSE;
#ifndef __EMX__
static char_u *extra_shell_arg = NULL;
static int show_shell_mess = TRUE;
#endif
! static int deadly_signal = 0; /* The signal we caught */
! static int in_mch_delay = FALSE; /* sleeping in mch_delay() */
static int curr_tmode = TMODE_COOK; /* contains current terminal mode */
--- 202,217 ----
# define SIG_ERR ((RETSIGTYPE (*)())-1)
#endif
! /* volatile, used in signal handler sig_winch(). */
! static volatile int do_resize = FALSE;
#ifndef __EMX__
static char_u *extra_shell_arg = NULL;
static int show_shell_mess = TRUE;
#endif
! /* volatile, used in signal handler deathtrap(). */
! static volatile int deadly_signal = 0; /* The signal we caught */
! /* volatile, used in signal handler deathtrap(). */
! static volatile int in_mch_delay = FALSE; /* sleeping in mch_delay() */
static int curr_tmode = TMODE_COOK; /* contains current terminal mode */
***************
*** 802,808 ****
#endif
/*
! * We need correct potatotypes for a signal function, otherwise mean compilers
* will barf when the second argument to signal() is ``wrong''.
* Let me try it with a few tricky defines from my own osdef.h (jw).
*/
--- 806,812 ----
#endif
/*
! * We need correct prototypes for a signal function, otherwise mean compilers
* will barf when the second argument to signal() is ``wrong''.
* Let me try it with a few tricky defines from my own osdef.h (jw).
*/
***************
*** 1068,1080 ****
SIGRETURN;
}
! #ifdef _REENTRANT
/*
* On Solaris with multi-threading, suspending might not work immediately.
* Catch the SIGCONT signal, which will be used as an indication whether the
* suspending has been done or not.
*/
! static int sigcont_received;
static RETSIGTYPE sigcont_handler __ARGS(SIGPROTOARG);
/*
--- 1072,1089 ----
SIGRETURN;
}
! #if defined(_REENTRANT) && defined(SIGCONT)
/*
* On Solaris with multi-threading, suspending might not work immediately.
* Catch the SIGCONT signal, which will be used as an indication whether the
* suspending has been done or not.
+ *
+ * On Linux, signal is not always handled immediately either.
+ * See https://bugs.launchpad.net/bugs/291373
+ *
+ * volatile, used in in signal handler sigcont_handler().
*/
! static volatile int sigcont_received;
static RETSIGTYPE sigcont_handler __ARGS(SIGPROTOARG);
/*
***************
*** 1118,1132 ****
}
# endif
! # ifdef _REENTRANT
sigcont_received = FALSE;
# endif
kill(0, SIGTSTP); /* send ourselves a STOP signal */
! # ifdef _REENTRANT
! /* When we didn't suspend immediately in the kill(), do it now. Happens
! * on multi-threaded Solaris. */
! if (!sigcont_received)
! pause();
# endif
# ifdef FEAT_TITLE
--- 1127,1154 ----
}
# endif
! # if defined(_REENTRANT) && defined(SIGCONT)
sigcont_received = FALSE;
# endif
kill(0, SIGTSTP); /* send ourselves a STOP signal */
! # if defined(_REENTRANT) && defined(SIGCONT)
! /*
! * Wait for the SIGCONT signal to be handled. It generally happens
! * immediately, but somehow not all the time. Do not call pause()
! * because there would be race condition which would hang Vim if
! * signal happened in between the test of sigcont_received and the
! * call to pause(). If signal is not yet received, call sleep(0)
! * to just yield CPU. Signal should then be received. If somehow
! * it's still not received, sleep 1, 2, 3 ms. Don't bother waiting
! * further if signal is not received after 1+2+3+4 ms (not expected
! * to happen).
! */
! {
! long wait;
! for (wait = 0; !sigcont_received && wait <= 3L; wait++)
! /* Loop is not entered most of the time */
! mch_delay(wait, FALSE);
! }
# endif
# ifdef FEAT_TITLE
***************
*** 1175,1181 ****
#ifdef SIGTSTP
signal(SIGTSTP, restricted ? SIG_IGN : SIG_DFL);
#endif
! #ifdef _REENTRANT
signal(SIGCONT, sigcont_handler);
#endif
--- 1197,1203 ----
#ifdef SIGTSTP
signal(SIGTSTP, restricted ? SIG_IGN : SIG_DFL);
#endif
! #if defined(_REENTRANT) && defined(SIGCONT)
signal(SIGCONT, sigcont_handler);
#endif
***************
*** 1234,1240 ****
reset_signals()
{
catch_signals(SIG_DFL, SIG_DFL);
! #ifdef _REENTRANT
/* SIGCONT isn't in the list, because its default action is ignore */
signal(SIGCONT, SIG_DFL);
#endif
--- 1256,1262 ----
reset_signals()
{
catch_signals(SIG_DFL, SIG_DFL);
! #if defined(_REENTRANT) && defined(SIGCONT)
/* SIGCONT isn't in the list, because its default action is ignore */
signal(SIGCONT, SIG_DFL);
#endif
***************
*** 5899,5905 ****
--- 5921,5929 ----
* we are going to suspend or starting an external process
* so we shouldn't have problem with this
*/
+ # ifdef SIGTSTP
signal(SIGTSTP, restricted ? SIG_IGN : SIG_DFL);
+ # endif
return 1; /* succeed */
}
if (gpm_fd == -2)
Index: globals.h
===================================================================
RCS file: /cvsroot/vim/vim7/src/globals.h,v
retrieving revision 1.94
diff -c -r1.94 globals.h
*** globals.h 6 Jan 2009 15:14:12 -0000 1.94
--- globals.h 24 Feb 2009 19:05:12 -0000
***************
*** 482,489 ****
/*
* While executing external commands or in Ex mode, should not insert GUI
* events in the input buffer: Set hold_gui_events to non-zero.
*/
! EXTERN int hold_gui_events INIT(= 0);
/*
* When resizing the shell is postponed, remember the new size, and call
--- 482,491 ----
/*
* While executing external commands or in Ex mode, should not insert GUI
* events in the input buffer: Set hold_gui_events to non-zero.
+ *
+ * volatile, used in signal handler sig_sysmouse().
*/
! EXTERN volatile int hold_gui_events INIT(= 0);
/*
* When resizing the shell is postponed, remember the new size, and call
***************
*** 597,602 ****
--- 599,605 ----
EXTERN int really_exiting INIT(= FALSE);
/* TRUE when we are sure to exit, e.g., after
* a deadly signal */
+ /* volatile, used in signal handler deathtrap(). */
EXTERN int full_screen INIT(= FALSE);
/* TRUE when doing full-screen output
* otherwise only writing some messages */
***************
*** 739,748 ****
*/
EXTERN JMP_BUF lc_jump_env; /* argument to SETJMP() */
# ifdef SIGHASARG
! EXTERN int lc_signal; /* catched signal number, 0 when no was signal
! catched; used for mch_libcall() */
# endif
! EXTERN int lc_active INIT(= FALSE); /* TRUE when lc_jump_env is valid. */
#endif
#if defined(FEAT_MBYTE) || defined(FEAT_POSTSCRIPT)
--- 742,753 ----
*/
EXTERN JMP_BUF lc_jump_env; /* argument to SETJMP() */
# ifdef SIGHASARG
! /* volatile, used in signal handlers. */
! EXTERN volatile int lc_signal; /* caught signal number, 0 when no was signal
! caught; used for mch_libcall() */
# endif
! /* volatile, used in signal handler deathtrap(). */
! EXTERN volatile int lc_active INIT(= FALSE); /* TRUE when lc_jump_env is valid. */
#endif
#if defined(FEAT_MBYTE) || defined(FEAT_POSTSCRIPT)
***************
*** 986,992 ****
EXTERN FILE *scriptout INIT(= NULL); /* stream to write script to */
EXTERN int read_cmd_fd INIT(= 0); /* fd to read commands from */
! EXTERN int got_int INIT(= FALSE); /* set to TRUE when interrupt
signal occurred */
#ifdef USE_TERM_CONSOLE
EXTERN int term_console INIT(= FALSE); /* set to TRUE when console used */
--- 991,998 ----
EXTERN FILE *scriptout INIT(= NULL); /* stream to write script to */
EXTERN int read_cmd_fd INIT(= 0); /* fd to read commands from */
! /* volatile, used in signal handler catch_sigint(). */
! EXTERN volatile int got_int INIT(= FALSE); /* set to TRUE when interrupt
signal occurred */
#ifdef USE_TERM_CONSOLE
EXTERN int term_console INIT(= FALSE); /* set to TRUE when console used */
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 24 Feb 2009 19:05:14 -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 24 Feb 2009 19:05:14 -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
