altstack: Consolidate thread-local variables

altstack uses a number of __thread variables to track internal state.  This
allows altstack to be thread-safe, although it's still not re-entrant.
This patch gathers all these variables into a single per-thread state
structure.  This makes it easy to see at a glance what the whole of the
required state is, and thereby easier to reason about correctness of
changes to the implementation.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
index 63512931f8955a7efe34b41f52df2696246701fd..edd79f6813d0b2e09e4e7391a1f25f39153b6d1c 100644 (file)
--- a/ccan/altstack/altstack.c
+++ b/ccan/altstack/altstack.c
@@ -11,43 +11,48 @@
 #include <unistd.h>
 #include <sys/mman.h>
 
-static __thread char ebuf[ALTSTACK_ERR_MAXLEN];
-static __thread unsigned elen;
-
-#define bang(x)                                                        \
-       (elen += snprintf(ebuf + elen, sizeof(ebuf) - elen,     \
-                "%s(altstack@%d) %s%s%s",                      \
-                elen  ? "; " : "", __LINE__, (x),              \
-                errno ? ": " : "", errno ? strerror(errno) : ""))
+static __thread struct altstack_state {
+       char ebuf[ALTSTACK_ERR_MAXLEN];
+       unsigned elen;
+       jmp_buf jmp;
+       void *rsp_save[2];
+       rlim_t max;
+       void *(*fn)(void *);
+       void *arg, *out;
+} state;
+
+#define bang(x)                                                                \
+       (state.elen += snprintf(state.ebuf + state.elen,                \
+                               sizeof(state.ebuf) - state.elen,        \
+                               "%s(altstack@%d) %s%s%s",               \
+                               state.elen  ? "; " : "", __LINE__, (x), \
+                               errno ? ": " : "",                      \
+                               errno ? strerror(errno) : ""))
 
 void altstack_perror(void)
 {
-       fprintf(stderr, "%s\n", ebuf);
+       fprintf(stderr, "%s\n", state.ebuf);
 }
 
 char *altstack_geterr(void)
 {
-       return ebuf;
+       return state.ebuf;
 }
 
-static __thread jmp_buf jmp;
-
 static void segvjmp(int signum)
 {
-       longjmp(jmp, 1);
+       longjmp(state.jmp, 1);
 }
 
-static __thread void *rsp_save_[2];
-static __thread rlim_t max_;
 
 rlim_t altstack_max(void) {
-       return max_;
+       return state.max;
 }
 
 static ptrdiff_t rsp_save(unsigned i) {
        assert(i < 2);
-       asm volatile ("movq %%rsp, %0" : "=g" (rsp_save_[i]));
-       return (char *) rsp_save_[0] - (char *) rsp_save_[i];
+       asm volatile ("movq %%rsp, %0" : "=g" (state.rsp_save[i]));
+       return (char *) state.rsp_save[0] - (char *) state.rsp_save[i];
 }
 
 void altstack_rsp_save(void) {
@@ -58,9 +63,6 @@ ptrdiff_t altstack_used(void) {
        return rsp_save(1);
 }
 
-static __thread void *(*fn_)(void *);
-static __thread void *arg_, *out_;
-
 int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
 {
        long pgsz = sysconf(_SC_PAGESIZE);
@@ -73,11 +75,11 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
        assert(max > 0 && fn);
        #define ok(x, y) ({ long __r = (long) (x); if (__r == -1) { bang(#x); if (y) goto out; } __r; })
 
-       fn_  = fn;
-       arg_ = arg;
-       out_ = 0;
-       max_ = max;
-       ebuf[elen = 0] = '\0';
+       state.fn  = fn;
+       state.arg = arg;
+       state.out = 0;
+       state.max = max;
+       state.ebuf[state.elen = 0] = '\0';
        if (out) *out = 0;
 
        // if the first page below the mapping is in use, we get max-pgsz usable bytes
@@ -85,13 +87,13 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
        max += pgsz;
 
        ok(getrlimit(RLIMIT_STACK, &rl_save), 1);
-       ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { max_, rl_save.rlim_max }), 1);
+       ok(setrlimit(RLIMIT_STACK, &(struct rlimit) { state.max, rl_save.rlim_max }), 1);
        undo++;
 
        ok(m = mmap(0, max, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_GROWSDOWN|MAP_NORESERVE, -1, 0), 1);
        undo++;
 
-       if (setjmp(jmp) == 0) {
+       if (setjmp(state.jmp) == 0) {
                unsigned char sigstk[SIGSTKSZ];
                stack_t ss = { .ss_sp = sigstk, .ss_size = sizeof(sigstk) };
                struct sigaction sa = { .sa_handler = segvjmp, .sa_flags = SA_NODEFER|SA_RESETHAND|SA_ONSTACK };
@@ -108,12 +110,13 @@ int altstack(rlim_t max, void *(*fn)(void *), void *arg, void **out)
                        "mov %1, %%rsp\n\t"
                        "sub $8, %%rsp\n\t"
                        "push %%r10"
-                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10", "memory");
-               out_ = fn_(arg_);
+                       : "=r" (state.rsp_save[0])
+                       : "0" (m + max) : "r10", "memory");
+               state.out = state.fn(state.arg);
                asm volatile ("pop %%rsp"
                              : : : "memory");
                ret = 0;
-               if (out) *out = out_;
+               if (out) *out = state.out;
        }
        else {
                errno = 0;
@@ -137,5 +140,5 @@ out:
 
        if (errno_save)
                errno = errno_save;
-       return !ret && elen ? 1 : ret;
+       return !ret && state.elen ? 1 : ret;
 }

diff --git a/ccan/altstack/test/run.c b/ccan/altstack/test/run.c
index 12cc460d8dd264647e11a7d4e0dacc4cf1b60a3d..23dd2e93ef69e83be1312b01da37f88b86d7ea85 100644 (file)
--- a/ccan/altstack/test/run.c
+++ b/ccan/altstack/test/run.c
@@ -24,7 +24,7 @@ char *m_;
 rlim_t msz_;
 #define e(x) (900+(x))
 #define seterr(x) (errno = e(x))
-#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno || out_ ? (x) : 0))
+#define setcall(x) ((call1 |= !errno ? (x) : 0), (call2 |= errno || state.out ? (x) : 0))
 #define getrlimit(...)         (fail&getrlimit_        ? (seterr(getrlimit_),          -1) : (setcall(getrlimit_),     getrlimit(__VA_ARGS__)))
 #define mmap(...)              (fail&mmap_             ? (seterr(mmap_),       (void *)-1) : (setcall(mmap_),          mmap(__VA_ARGS__)))
 #define munmap(a, b)           (fail&munmap_           ? (seterr(munmap_),             -1) : (setcall(munmap_),        munmap(m_=(a), msz_=(b))))