Clean up and clarify slim_setjmp() / waserror()
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 3 Apr 2019 19:21:15 +0000 (15:21 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 3 Apr 2019 19:23:59 +0000 (15:23 -0400)
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>
Documentation/plan9.txt
kern/include/err.h
kern/include/setjmp.h

index acf509b..89fe6a8 100644 (file)
@@ -141,6 +141,41 @@ If you overflow or underflow the error stack the kernel will panic.
 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/ 
index 317d186..7768bf0 100644 (file)
@@ -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, ...);
index f45808a..f6d821b 100644 (file)
@@ -8,11 +8,10 @@ int slim_setjmp(struct jmpbuf *env) __attribute__((returns_twice));
 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