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
