Clean up and clarify slim_setjmp() / waserror() Any time you use a variable inside a waserror() block that has been written since the waserror() / setjmp(), that variable must be marked volatile. The bool err; in slim_setjmp served little purpose, other than confusion. Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
diff --git a/Documentation/plan9.txt b/Documentation/plan9.txt index acf509b..89fe6a8 100644 --- a/Documentation/plan9.txt +++ b/Documentation/plan9.txt
@@ -141,6 +141,41 @@ The implementation is in kern/src/error.c, the macros are in kern/include/plan9.h. +Giant warning: if you access any automatic (local) variables inside the waserror +block that may have been modified after you started the waserror, those +variables need to be volatile. waserror() is implemented with setjmp and +according to one of the man pages: + + automatic variables are unspecified after a call to longjmp() if + they meet all the following criteria: + - they are local to the function that made the corresponding setjmp(3) + call; + - their values are changed between the calls to setjmp(3) and + longjmp(); and + - they are not declared as volatile. + +We could ask the compiler to tell us which variables are potentially clobbered +with -Wclobbered, however it is a noisy warning. It will warn even if the +variables are not used in the error case. That may be because the compiler has +a hard time deciding whether a variable is used or not, since we often longjmp +from within an error handler, though on gcc 4.9.2, even if we return +immediately, we still get a warning. On a related note, it's not always +possible to tell which case is the error handling case - consider the "discard" +pattern for waserror. + +No amount of "returns_twice" or register clobbering or "memory" clobbering is +enough. Think about what happens when the variable is changed after the setjmp +call, i.e. farther down in the function. It's may be in a register, then we +call some other function, and that longjmps. That register value is gone (it +might be somewhere else on the stack). The compiler needs to know when it makes +that write that it needs to go onto the stack storage of the automatic variable. +That's 'volatile.' + +The best we can hope for is the compiler to know what variables could be written +from one side of the setjmp and used on the other. Perhaps that will show up, +and then we can turn on -Wclobbered. Until then, we have to be vigilant, or use +different patterns for waserror. Note there are a bunch of bugs with +-Wclobbered detection, e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65041. For info on the format of Ms (which are part of the kernel interface): http://9p.cat-v.org/documentation/rfc/
diff --git a/kern/include/err.h b/kern/include/err.h index 317d186..7768bf0 100644 --- a/kern/include/err.h +++ b/kern/include/err.h
@@ -9,7 +9,7 @@ #define ERRSTACK(x) struct errbuf *prev_errbuf; struct errbuf errstack[(x)]; \ int curindex = 0; #define waserror() setjmp(&(errpush(errstack, ARRAY_SIZE(errstack), &curindex, \ - &prev_errbuf)->jmpbuf)) + &prev_errbuf)->jmpbuf)) /* In syscall.h, but want to avoid circular include issues. */ extern void set_error(int error, const char *fmt, ...);
diff --git a/kern/include/setjmp.h b/kern/include/setjmp.h index f45808a..f6d821b 100644 --- a/kern/include/setjmp.h +++ b/kern/include/setjmp.h
@@ -8,11 +8,10 @@ void longjmp(struct jmpbuf *env, int val) __attribute__((noreturn)); #pragma GCC diagnostic push +/* Currently, this only throws in tcpackproc(). Not sure why, but if you take + * out the loop++ > 1000, it won't warn. */ #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" -#define setjmp(jb) ({bool err; \ - __ros_clobber_callee_regs(); \ - err = slim_setjmp(jb); \ - err;}) +#define setjmp(jb) ({ __ros_clobber_callee_regs(); slim_setjmp(jb); }) #pragma GCC diagnostic pop