Fix issues with signal handling

Native signal handler callbacks were doing too much, now they just set
the interrupt flag on the main thread and let the VM do the rest.

examples/signals.lox

@@ -1,10 +1,10 @@
 var stop = false;
 Signal.trap(Signal::QUIT, fun() {
-  print "In handler";
+  print "In signal handler";
   stop = true;
 });
 
-// make sure signal handlers functions are marked
+// test that signal handler functions get marked during GC and are not collected
 GC.collect();
 GC.collect();
 
@@ -13,10 +13,9 @@ var childPid = Process.fork(fun() {
   print "Signalling from child";
   Process.signal(Process.ppid(), Signal::QUIT);
 });
-Process.detach(childPid);
 
 print "Looping in parent";
-while(stop == false) {
+while (stop == false) {
 }
 print "Loop stopped";
 
@@ -25,5 +24,5 @@ print "Loop stopped";
 __END__
 -- expect: --
 Looping in parent
-In handler
+In signal handler
 Loop stopped

process.c

@@ -42,6 +42,7 @@ static Value lxForkStatic(int argCount, Value *args) {
         return NUMBER_VAL(pid);
     } else { // in child
         if (argCount == 2) {
+            vm.mainThread->pid = getpid();
             callCallable(func, 0, false, NULL);
             stopVM(0);
         }

signal.c

@@ -37,29 +37,28 @@ int getSignal() {
     return -1;
 }
 
-void execSignal(LxThread *th, int signum) {
+/* Execute one queued signal handler on the main thread */
+int execSignal(LxThread *th, int signum) {
     (void)th;
     SigHandler *cur = sigHandlers;
     while (cur) {
         if (cur->signum == signum) {
+            ASSERT(th == vm.mainThread);
             (void)callFunctionValue(OBJ_VAL(cur->callable), 0, NULL);
+            return 0;
         }
         cur = cur->next;
     }
+    return -1;
 }
 
-// callback when a native signal gets sent
+// Callback when a native signal gets sent to the process. We enqueue the signal
+// and set the interrupt flag on the main thread.
 static void sigHandlerFunc(int signum, siginfo_t *sinfo, void *_context) {
     enqueueSignal(signum);
     pthread_mutex_lock(&vm.mainThread->interruptLock);
     SET_TRAP_INTERRUPT(vm.mainThread);
     pthread_mutex_unlock(&vm.mainThread->interruptLock);
-    // execute the interrupts on the main thread
-    if (vm.mainThread != vm.curThread) {
-        threadSchedule(vm.mainThread);
-    } else {
-        vmCheckInts(vm.curThread);
-    }
 }
 
 void removeVMSignalHandlers(void) {

thread.c

@@ -52,19 +52,23 @@ const char *threadStatusName(ThreadStatus status) {
     }
 }
 
+/*
+ * Execute all interrupt signal handlers on the main thread.
+ */
 void threadExecuteInterrupts(LxThread *th) {
     ASSERT(vm.curThread == th);
     int interrupt = 0;
     while ((interrupt = threadGetInterrupt(th)) != INTERRUPT_NONE) {
         if (interrupt == INTERRUPT_TRAP && th == vm.mainThread) {
-            ASSERT(th == vm.mainThread);
             th->interruptFlags &= (~interrupt);
-            int sig = -1;
+            int sig;
             while ((sig = getSignal()) != -1) {
-                execSignal(th, sig);
+                if (execSignal(th, sig) != 0) {
+                    break;
+                }
             }
-        } else if (interrupt == INTERRUPT_GENERAL) { // kill signal
-            THREAD_DEBUG(1, "Thread %lu got interrupt, exiting", th->tid);
+        } else if (interrupt == INTERRUPT_GENERAL) { // sent 'exit' interrupt to living thread
+            THREAD_DEBUG(1, "Thread %lu got exit interrupt, exiting", th->tid);
             ASSERT(th != vm.mainThread);
             if (GVLOwner == th->tid) {
                 THREAD_DEBUG(1, "thread releasing GVL before exit");
@@ -177,10 +181,8 @@ typedef struct NewThreadArgs {
     LxThread *th;
 } NewThreadArgs;
 
-// NOTE: vm.curThread is NOT the current thread here, and doesn't hold
-// the GVL. We can't call ANY functions that call THREAD() in them, as they'll
-// fail (phthread_self() tid is NOT in vm.threads vector until end of
-// function).
+// NOTE: This thread is not running yet, this is just setting it up.
+// It doesn't have a thread id (tid) or its own stack yet.
 static ObjInstance *newThreadSetup(LxThread *parentThread) {
     ASSERT(parentThread);
     THREAD_DEBUG(3, "New thread setup");
@@ -241,11 +243,9 @@ static void *runCallableInNewThread(void *arg) {
     NewThreadArgs *tArgs = (NewThreadArgs*)arg;
     pthread_t tid = pthread_self();
     LxThread *th = tArgs->th;
-    if (th->tid != 0) {
-        ASSERT(th->tid == tid);
-    }
     ASSERT(th->status == THREAD_READY);
     th->tid = tid;
+    th->pid = getpid();
     ASSERT(th->tid > 0);
     THREAD_DEBUG(2, "switching to newly created thread, acquiring lock %lu", th->tid);
     THREAD_DEBUG(2, "acquiring GVL");
@@ -273,7 +273,7 @@ static void *runCallableInNewThread(void *arg) {
     th->status = THREAD_RUNNING;
     callCallable(OBJ_VAL(closure), 0, false, NULL);
     THREAD_DEBUG(2, "Exiting thread (returned) %lu", pthread_self());
-    stopVM(0); // actually just exits the thread
+    stopVM(0); // actually just exits the thread if it's not the main thread
 }
 
 
@@ -297,6 +297,7 @@ Value lxNewThread(int argCount, Value *args) {
     releaseGVL(THREAD_STOPPED);
     if (pthread_create(&tnew, NULL, runCallableInNewThread, thArgs) == 0) {
         acquireGVL();
+        // it's important the new thread has a tid even if it hasn't started running yet
         if (th->tid == 0) {
             th->tid = tnew;
         }

vm.c

@@ -465,6 +465,7 @@ static ObjInstance *initMainThread(void) {
 
     pthread_t tid = pthread_self();
     threadSetId(mainThread, tid);
+    th->pid = getpid();
     vm.numLivingThreads++;
     threadSetStatus(mainThread, THREAD_RUNNING);
     acquireGVL();
@@ -1326,7 +1327,7 @@ static void closeUpvalues(Value *last);
 
 void popFrame(void) {
     DBG_ASSERT(vm.inited);
-    register LxThread *th = vm.curThread;
+    LxThread *th = vm.curThread;
     ASSERT(EC->frameCount >= 1);
     CallFrame *frame = getFrame();
     Value *newTop = frame->slots;
@@ -2192,18 +2193,19 @@ NORETURN void throwErrorFmt(ObjClass *klass, const char *format, ...) {
 
 void printVMStack(FILE *f, LxThread *th) {
     pthread_t tid = 0;
+    pid_t pid = th->pid;
     if (th != vm.mainThread) {
         tid = th->tid;
     }
     if (th->ec->stackTop == th->ec->stack && th->v_ecs.length == 1) {
-        fprintf(f, "[DEBUG %d (th=%lu)]: Stack: empty\n", th->vmRunLvl, (unsigned long)tid);
+        fprintf(f, "[DEBUG %d (pid=%d, th=%lu)]: Stack: empty\n", th->vmRunLvl, (int)pid, (unsigned long)tid);
         return;
     }
     VMExecContext *ec = NULL; int i = 0;
     int numCallFrames = VMNumCallFrames();
     int numStackFrames = VMNumStackFrames();
-    fprintf(f, "[DEBUG %d (th=%lu)]: Stack (%d stack frames, %d call frames):\n", th->vmRunLvl,
-            (unsigned long)tid, numStackFrames, numCallFrames);
+    fprintf(f, "[DEBUG %d (pid=%d, th=%lu)]: Stack (%d stack frames, %d call frames):\n", th->vmRunLvl,
+            (int)pid, (unsigned long)tid, numStackFrames, numCallFrames);
     // print VM stack values from bottom of stack to top
     fprintf(f, "[DEBUG %d]: ", th->vmRunLvl);
     int callFrameIdx = 0;
@@ -2354,12 +2356,12 @@ static InterpretResult vm_run0() {
  * Run the VM's instructions.
  */
 static InterpretResult vm_run() {
-    register LxThread *th = vm.curThread;
-    register Chunk *ch = currentChunk();
-    register Value *constantSlots = ch->constants->values;
-    register CallFrame *frame = getFrame();
+    LxThread *th = vm.curThread;
+    Chunk *ch = currentChunk();
+    Value *constantSlots = ch->constants->values;
+    CallFrame *frame = getFrame();
     ObjScope *scope = frame->scope;
-    register VMExecContext *ctx = EC;
+    VMExecContext *ctx = EC;
     th->vmRunLvl++;
     if (ch->catchTbl != NULL) {
         int jumpRes = setjmp(frame->jmpBuf);
@@ -2458,10 +2460,12 @@ static InterpretResult vm_run() {
     if (CLOX_OPTION_T(stepVMExecution) && vm.instructionStepperOn) {
         fprintf(stderr, "STEPPER> ");
         while (getline(&stepLine, &stepLineSz, stdin)) {
+            // continue, turn off stepper
             if (strcmp("c\n", stepLine) == 0) {
                 vm.instructionStepperOn = false;
                 xfree(stepLine);
                 break;
+            // next instruction
             } else if (strcmp("n\n", stepLine) == 0) {
                 xfree(stepLine);
                 break;
@@ -2877,7 +2881,7 @@ static InterpretResult vm_run() {
               DBG_ASSERT(ipOffset > 0);
               frame->ip += (ipOffset-1);
           }
-          VM_CHECK_INTS(vm.curThread);
+          VM_CHECK_INTS(th);
           DISPATCH_BOTTOM();
       }
       CASE_OP(JUMP_IF_TRUE): {
@@ -2887,7 +2891,7 @@ static InterpretResult vm_run() {
               DBG_ASSERT(ipOffset > 0);
               frame->ip += (ipOffset-1);
           }
-          VM_CHECK_INTS(vm.curThread);
+          VM_CHECK_INTS(th);
           DISPATCH_BOTTOM();
       }
       CASE_OP(JUMP_IF_FALSE_PEEK): {
@@ -2897,7 +2901,7 @@ static InterpretResult vm_run() {
               DBG_ASSERT(ipOffset > 0);
               frame->ip += (ipOffset-1);
           }
-          VM_CHECK_INTS(vm.curThread);
+          VM_CHECK_INTS(th);
           DISPATCH_BOTTOM();
       }
       CASE_OP(JUMP_IF_TRUE_PEEK): {
@@ -2907,14 +2911,14 @@ static InterpretResult vm_run() {
               DBG_ASSERT(ipOffset > 0);
               frame->ip += (ipOffset-1);
           }
-          VM_CHECK_INTS(vm.curThread);
+          VM_CHECK_INTS(th);
           DISPATCH_BOTTOM();
       }
       CASE_OP(JUMP): {
           bytecode_t ipOffset = READ_WORD();
           ASSERT(ipOffset > 0);
           frame->ip += (ipOffset-1);
-          VM_CHECK_INTS(vm.curThread);
+          VM_CHECK_INTS(th);
           DISPATCH_BOTTOM();
       }
       CASE_OP(LOOP): {
@@ -2923,7 +2927,7 @@ static InterpretResult vm_run() {
           // add 1 for the instruction we just read, and 1 to go 1 before the
           // instruction we want to execute next.
           frame->ip -= (ipOffset+2);
-          VM_CHECK_INTS(vm.curThread);
+          VM_CHECK_INTS(th);
           DISPATCH_BOTTOM();
       }
       CASE_OP(BREAK): {
@@ -2934,7 +2938,7 @@ static InterpretResult vm_run() {
               VM_POP(); numPops--;
           }
           frame->ip += (ipOffset-3); // -3 to for the 2 just read instructions, and 1 more to go to 1 before the instruction
-          VM_CHECK_INTS(vm.curThread);
+          VM_CHECK_INTS(th);
           DISPATCH_BOTTOM();
       }
       CASE_OP(BLOCK_BREAK): {
@@ -3952,8 +3956,6 @@ void terminateThreads() {
     vm.numLivingThreads = 1;
 }
 
-// TODO: rename to exitThread(), as this either stops the VM or exits the
-// current non-main thread
 NORETURN void stopVM(int status) {
     LxThread *th = vm.curThread;
     if (th == vm.mainThread) {

vm.h

@@ -4,6 +4,7 @@
 #include <pthread.h>
 #include <stdio.h>
 #include <setjmp.h>
+#include <unistd.h>
 #include "chunk.h"
 #include "object.h"
 #include "table.h"
@@ -113,13 +114,16 @@ typedef struct BlockStackEntry {
 } BlockStackEntry;
 
 #define INTERRUPT_NONE 0
+// internal exit interrupt in VM
 #define INTERRUPT_GENERAL 1
+// all signals use this interrupt
 #define INTERRUPT_TRAP 2
 #define SET_TRAP_INTERRUPT(th) (th->interruptFlags |= INTERRUPT_TRAP)
 #define SET_INTERRUPT(th) (th->interruptFlags |= INTERRUPT_GENERAL)
 #define INTERRUPTED_ANY(th) (th->interruptFlags != INTERRUPT_NONE)
 typedef struct LxThread {
     pthread_t tid;
+    pid_t pid;
     volatile ThreadStatus status;
     VMExecContext *ec; // current execution context of vm
     vec_void_t v_ecs; // stack of execution contexts. Top of stack is current context.
@@ -138,7 +142,7 @@ typedef struct LxThread {
     bool returnedFromNativeErr;
     jmp_buf cCallJumpBuf;
     bool cCallJumpBufSet;
-    int vmRunLvl;
+    int vmRunLvl; // how many calls to vm_run() are in the current thread's native stack
     int lastSplatNumArgs;
     // stack of object pointers created during C function calls. When
     // control returns to the VM, these are popped. Stack objects aren't
@@ -149,7 +153,7 @@ typedef struct LxThread {
     pthread_mutex_t sleepMutex;
     pthread_cond_t sleepCond;
     pthread_mutex_t interruptLock;
-    int interruptFlags;
+    volatile int interruptFlags;
     int opsRemaining;
     int exitStatus;
     int lastOp; // for debugging
@@ -266,7 +270,7 @@ typedef struct SigHandler {
 extern SigHandler *sigHandlers;
 void removeVMSignalHandlers(void);
 void enqueueSignal(int signo);
-void execSignal(LxThread *th, int signo);
+int execSignal(LxThread *th, int signo);
 void threadCheckSignals(LxThread *th);
 int getSignal(void);