libap: ucontext's three switches must be assembly, not C wrappers

docs/notes/libap.md

@@ -4055,9 +4055,50 @@ means in a host program. *The instrument reporting itself is the shape
 to recognise rather than argue with* -- the assertion was right and the
 fake was wrong.
 
-**What neither covers**: `uc_sigmask` is saved and restored through
-`sigprocmask`, which on APE is a read and a write of one global
-(`signal/sigprocmask.c`), and nothing here exercises a context switch
+### The C wrapper could not save its caller's frame
+
+***THE FIRST VERSION HAD `getcontext`, `setcontext` AND `swapcontext`
+AS C AROUND THREE REGISTER-ONLY PRIMITIVES, AND THE VM REFUTED IT.***
+`ucontext-test` section 0 printed its PASS line **once** instead of
+twice and reported `setcontext came back round: got 1 want 2`.
+
+What a C `getcontext` saves is a point inside `getcontext` itself.
+That is fine until the context is RESUMED after `getcontext` has
+returned -- by then the wrapper's frame is dead stack, its own
+return-address slot has been reused by whatever the caller did next,
+and the resumed epilogue RETs to that. Here it landed after the
+`setcontext` CALL, because *setcontext's* return address was sitting in
+the slot: control appeared to flow straight on, with the counter still
+at 1.
+
+**The same reasoning says why `swapcontext` got away with it**, and
+that half is worth keeping so the lesson is not over-general: a context
+saved by `swapcontext` is resumed while its stack is parked, so nothing
+has run on it and the frame is intact. Sections 1 to 4 all passed.
+*`getcontext` is the one that returns and is then resumed, and it is
+the only one that could have shown this.*
+
+All three are assembly now, which is what `setjmp.s` was all along.
+**The cost is one shared offset** -- `uc_sigmask` at 64 -- and
+`makecontext.c` turns a change to it into a COMPILE error
+(`size of array '_uc_layout_assert' is negative`) rather than a switch
+that quietly restores the wrong mask. The mask is handled the way
+`sigsetjmp` handles it, through `_psigblocked(SB)`, with `MOVL`
+because `sigset_t` is `long` and kencc's `long` is four bytes.
+
+### And section 5 was the test's bug, of the one kind gcc cannot catch
+
+It read `volatile long a = 0x1111111111111111L;`. **kencc's `long` is
+four bytes**, so `a` held `0x11111111` and the comparison against the
+full 64-bit constant was false -- section 5 reported `got 0 want 1` for
+a context switch that was working perfectly. `long long` now.
+
+*The glibc run could not have caught it*: this is the one class of test
+bug the reference implementation is blind to, because the difference IS
+the reference's width. The tree's first invariant, arriving inside a
+file written to check something else.
+
+**What neither instrument covers**: nothing exercises a context switch
 with signals actually blocked. And the header could not be
 syntax-checked against the APE headers on the host -- they reach each
 other through absolute `/sys/include/ape/...` paths that resolve only

sys/include/ape/ucontext.h

@@ -37,17 +37,22 @@ typedef struct {
 } stack_t;
 
 /*
- * `uc_mcontext' IS FIRST and must stay first: `_getmcontext' and
- * friends are handed `&ucp->uc_mcontext' by the C wrappers, so nothing
- * in the assembly depends on this layout -- but keeping the machine
- * state at offset 0 means a debugger looking at a `ucontext_t' finds
- * the registers where it expects them.
+ * ***THE FIRST TWO MEMBERS ARE A CONTRACT WITH `ucontext.s', WHICH
+ * ADDRESSES THEM BY NUMBER.***  `uc_mcontext' at 0 and `uc_sigmask' at
+ * 64 are what `getcontext', `setcontext' and `swapcontext' assume, and
+ * `arch/amd64/makecontext.c' turns a change here into a COMPILE error
+ * rather than a switch that quietly restores the wrong signal mask.
+ * The two after them are C's alone and may be reordered freely.
+ *
+ * *Those three are assembly, rather than C around a smaller primitive,
+ * because a C wrapper cannot save its CALLER's frame* -- `ucontext.s'
+ * has the failure that established it.
  */
 typedef struct __ucontext {
-	mcontext_t          uc_mcontext;
+	mcontext_t          uc_mcontext;	/* 0 */
+	sigset_t            uc_sigmask;		/* 64 */
 	struct __ucontext  *uc_link;
 	stack_t             uc_stack;
-	sigset_t            uc_sigmask;
 } ucontext_t;
 
 extern int  getcontext(ucontext_t *);

sys/lib/tests/makectx-xcheck.c

@@ -4,11 +4,11 @@
  *	sh makectx-xcheck.sh
  *
  * ***A CROSS-CHECK, AND IT COMPILES THE REAL FILE RATHER THAN A COPY
- * OF IT.*** `lib/ap/arch/amd64/ucontext.c' is compiled straight into
+ * OF IT.*** `lib/ap/arch/amd64/makecontext.c' is compiled straight into
  * this program -- no extraction, no transcription, no markers to rot --
- * with stubs standing in for the three assembly primitives and for
- * `sigprocmask'.  That is possible because everything structural in
- * that file is C; the assembly knows only "eight slots".
+ * with stubs standing in for `_ctxreturn' and `setcontext'.  That is
+ * possible because the frame layout lives entirely in C; the assembly
+ * knows only the eight register slots and one asserted offset.
  *
  * WHY IT IS WORTH A FILE.  The one number that has to agree across the
  * two languages is where the ucontext pointer sits in the new frame:
@@ -32,6 +32,13 @@
  *   `base[UC_ARGMAX+1]' -> `base[UC_ARGMAX]'   **3 failures**
  *   `(...)base - 8' -> `(...)base'             **1 failure**
  *
+ * and a third that is not a failure count at all: swapping
+ * `uc_sigmask' and `uc_link' in `<ucontext.h>' is a **compile error**,
+ * `size of array `_uc_layout_assert' is negative'.  That is the one
+ * offset the assembly addresses by number, so it is asserted rather
+ * than trusted -- a wrong answer there would be a context switch
+ * quietly restoring the wrong signal mask.
+ *
  * Exit status is the failure count.
  */
 
@@ -43,38 +50,29 @@
 
 #include <ucontext.h>		/* the tree's, through the script's -I */
 
-/* ---- stubs for what lives in mcontext.s ---------------------- */
+/* ---- stubs for what lives in ucontext.s ---------------------- */
 
 void _ctxreturn(void) { }
 
 /*
  * ***THE STUB MUST NOT RETURN, AND THE FIRST VERSION DID.***  The real
- * `_setmcontext' jumps away and never comes back, so `_ctxfinish's
+ * `setcontext' jumps away and never comes back, so `_ctxfinish's
  * `exit(0)' is unreachable once a `uc_link' has been followed.  A stub
  * that simply recorded and returned fell through to that `exit', and
  * section 3's "did not exit" check failed -- *the instrument reporting
  * its own stub, which is the shape to recognise rather than to argue
  * with*.  It longjmps instead, which is what "does not return" means in
  * a host program.
  */
-static const mcontext_t *last_set;
+static const ucontext_t *last_set;
 static jmp_buf away;
 
-void
-_setmcontext(const mcontext_t *mc)
-{
-	last_set = mc;
-	longjmp(away, 1);
-}
-
-void _swapmcontext(mcontext_t *o, const mcontext_t *n) { (void)o; last_set = n; }
-int  _getmcontext(mcontext_t *mc) { (void)mc; return 0; }
-
 int
-sigprocmask(int how, sigset_t *set, sigset_t *oset)
+setcontext(const ucontext_t *ucp)
 {
-	(void)how; (void)set; (void)oset;
-	return 0;
+	last_set = ucp;
+	longjmp(away, 1);
+	return -1;
 }
 
 /* `_ctxfinish' ends in exit(0) when uc_link is null; catch that. */
@@ -83,7 +81,7 @@ static int exited;
 static void xexit(int n) { (void)n; exited = 1; }
 
 /* ---- the real file ------------------------------------------- */
-#include "ucontext.c"
+#include "makecontext.c"
 
 #undef exit
 
@@ -164,22 +162,22 @@ main(void)
 	 * pointer, uc_link is garbage and the usual result is a silent
 	 * exit.
 	 */
-	last_set = (const mcontext_t *)0;
+	last_set = (const ucontext_t *)0;
 	exited = 0;
 	if(setjmp(away) == 0)
 		_ctxfinish((void *)(base + 1));
-	eq("it followed uc_link", last_set == &link.uc_mcontext, 1);
+	eq("it followed uc_link", last_set == &link, 1);
 	eq("and did not fall through to exit", exited, 0);
 
 	printf("\nSection 4: a null uc_link exits instead\n");
 	uc.uc_link = (ucontext_t *)0;
 	base[UC_ARGMAX+1] = (unsigned long long)(uintptr_t)&uc;
-	last_set = (const mcontext_t *)0;
+	last_set = (const ucontext_t *)0;
 	exited = 0;
 	if(setjmp(away) == 0)
 		_ctxfinish((void *)(base + 1));
 	eq("it exited", exited, 1);
-	eq("and followed no link", last_set == (const mcontext_t *)0, 1);
+	eq("and followed no link", last_set == (const ucontext_t *)0, 1);
 
 	printf("\nSection 5: every argument slot, at the cap\n");
 	memset(&uc, 0, sizeof uc);

sys/lib/tests/makectx-xcheck.sh

@@ -1,15 +1,15 @@
 #!/bin/sh
-# makectx-xcheck.sh -- compile libap's real `ucontext.c' on the build
-# host, beside stubs for the three assembly primitives, and check that
+# makectx-xcheck.sh -- compile libap's real `makecontext.c' on the build
+# host, beside stubs for `_ctxreturn' and `setcontext', and check that
 # the frame `makecontext' builds is the one `_ctxfinish' reads back.
 #
 #	sh makectx-xcheck.sh
 #
 # ***NOTHING IS EXTRACTED OR TRANSCRIBED HERE.*** The harness
-# `#include's the library file itself, which is possible because
-# everything structural in it is C -- the assembly knows only "eight
-# slots".  So there are no markers to rot, and the thing under test is
-# the thing that ships.
+# `#include's the library file itself, which is possible because the
+# frame layout lives entirely in C -- the assembly knows only the eight
+# register slots and one asserted offset.  So there are no markers to
+# rot, and the thing under test is the thing that ships.
 #
 # The three headers it needs are the tree's own: `<ucontext.h>' from
 # `sys/include/ape' and `<ucontext_arch.h>' from `amd64/include/ape'.
@@ -30,7 +30,7 @@ set -e
 
 # This file is three levels below the repository root.
 ROOT=$(cd "$(dirname "$0")/../../.." && pwd)
-SRC=$ROOT/sys/src/ape/lib/ap/arch/amd64/ucontext.c
+SRC=$ROOT/sys/src/ape/lib/ap/arch/amd64/makecontext.c
 HDR=$ROOT/sys/include/ape/ucontext.h
 ARCH=$ROOT/amd64/include/ape/ucontext_arch.h
 HERE=$(cd "$(dirname "$0")" && pwd)
@@ -43,7 +43,7 @@ done
 rm -rf "$TMP"; mkdir -p "$TMP"
 ln -s "$HDR"  "$TMP/ucontext.h"
 ln -s "$ARCH" "$TMP/ucontext_arch.h"
-ln -s "$SRC"  "$TMP/ucontext.c"
+ln -s "$SRC"  "$TMP/makecontext.c"
 : > "$TMP/lib.h"
 cat > "$TMP/signal.h" <<'EOF'
 #ifndef _MAKECTX_STUB_SIGNAL_H

sys/lib/tests/ucontext-test.c

@@ -130,14 +130,27 @@ s4_fn(void)
 static ucontext_t s5_main, s5_ctx;
 static int s5_seen;
 
+/*
+ * ***`long long', NOT `long', AND THAT IS WHAT THE FIRST VM RUN
+ * FOUND.***  This said `volatile long a = 0x1111111111111111L;', which
+ * passes on glibc and cannot on this machine: **kencc's `long` is four
+ * bytes**, so `a' held `0x11111111' and the comparison against the full
+ * 64-bit constant was false.  Section 5 reported `got 0 want 1' for a
+ * context switch that was working perfectly.
+ *
+ * *The gcc check could not have caught it* -- this is the one class of
+ * test bug that the reference implementation is blind to, because the
+ * difference IS the reference's width.  The tree's own first invariant,
+ * arriving in the file written to check something else.
+ */
 static void
 s5_fn(void)
 {
-	volatile long a = 0x1111111111111111L;
-	volatile long b = 0x2222222222222222L;
+	volatile long long a = 0x1111111111111111LL;
+	volatile long long b = 0x2222222222222222LL;
 
 	swapcontext(&s5_ctx, &s5_main);
-	if(a == 0x1111111111111111L && b == 0x2222222222222222L)
+	if(a == 0x1111111111111111LL && b == 0x2222222222222222LL)
 		s5_seen = 1;
 	swapcontext(&s5_ctx, &s5_main);
 }