rts: Refactor handling of dead threads' stacks

This fixes a bug that @JunmingZhao42 and I noticed while working on her
MMTK port. Specifically, in stg_stop_thread we used stg_enter_info as a
sentinel at the tail of a stack after a thread has completed. However,
stg_enter_info expects to have a two-field payload, which we do not
push. Consequently, if the GC ends up somehow the stack it will attempt
to interpret data past the end of the stack as the frame's fields,
resulting in unsound behavior.

To fix this I eliminate this hacky use of `stg_stop_thread` and instead
introduce a new stack frame type, `stg_dead_thread_info`. Not only does
this eliminate the potential for the previously mentioned memory
unsoundness but it also more clearly captures the intended structure of
the dead threads' stacks.

6 files changed, 36 insertions, 14 deletions

diff --git a/rts/Schedule.c b/rts/Schedule.c
index fa48bef1a7..9b1e98ea17 100644
--- a/rts/Schedule.c
+++ b/rts/Schedule.c
@@ -1315,7 +1315,9 @@ scheduleHandleThreadFinished (Capability *cap, Task *task, StgTSO *t)
           if (t->what_next == ThreadComplete) {
               if (task->incall->ret) {
                   // NOTE: return val is stack->sp[1] (see StgStartup.cmm)
-                  *(task->incall->ret) = (StgClosure *)task->incall->tso->stackobj->sp[1];
+                  StgDeadThreadFrame *dead = (StgDeadThreadFrame *) &task->incall->tso->stackobj->sp[0];
+                  ASSERT(dead->header.info == &stg_dead_thread_info);
+                  *(task->incall->ret) = (StgClosure *) dead->result;
               }
               task->incall->rstat = Success;
           } else {
diff --git a/rts/StgMiscClosures.cmm b/rts/StgMiscClosures.cmm
index b9aee516d0..dbdd2d71fa 100644
--- a/rts/StgMiscClosures.cmm
+++ b/rts/StgMiscClosures.cmm
@@ -414,6 +414,16 @@ INFO_TABLE_RET( stg_apply_interp, RET_BCO )
     jump stg_yield_to_interpreter [];
 }
 
+/*
+ * Dummy info table pushed on the top of the stack when the stack's
+ * owning thread has finished.
+ */
+INFO_TABLE_RET( stg_dead_thread, RET_SMALL,
+                W_ info_ptr,
+                PROF_HDR_FIELDS(W_,p1,p2)
+                P_ result )
+{ foreign "C" barf("stg_dead_thread entered!") never returns; }
+
 /* ----------------------------------------------------------------------------
    Entry code for a BCO
    ------------------------------------------------------------------------- */
diff --git a/rts/StgStartup.cmm b/rts/StgStartup.cmm
index 122eace1f3..388e27965e 100644
--- a/rts/StgStartup.cmm
+++ b/rts/StgStartup.cmm
@@ -49,15 +49,15 @@ INFO_TABLE_RET(stg_stop_thread, STOP_FRAME,
        We save R1 on top of the stack where the scheduler can find it,
        tidy up the registers and return to the scheduler.
 
-       We Leave the stack looking like this:
+       We leave the stack looking like this:
 
-                +----------------+
-                |      -------------------> return value
-                +----------------+
-                | stg_enter_info |
-                +----------------+
+                +----------------------+
+        Sp ---> | stg_dead_thread_info |
+                +----------------------+
+                |      -----------------------> return value
+                +----------------------+
 
-       The stg_enter_info is just a dummy info table so that the
+       The stg_dead_thread_info is just a dummy info table so that the
        garbage collector can understand the stack (there must always
        be an info table on top of the stack).
     */
@@ -70,9 +70,9 @@ INFO_TABLE_RET(stg_stop_thread, STOP_FRAME,
     unwind UnwindReturnReg = return;
 #endif
 
-    Sp = Sp + SIZEOF_StgStopFrame - WDS(2);
-    Sp(1) = R1;
-    Sp(0) = stg_enter_info;
+    Sp = Sp + SIZEOF_StgStopFrame - SIZEOF_StgDeadThreadFrame;
+    SET_HDR(Sp, stg_dead_thread_info, CCS_SYSTEM);
+    StgDeadThreadFrame_result(Sp) = R1;
 
     StgTSO_what_next(CurrentTSO) = ThreadComplete::I16;
 
diff --git a/rts/include/rts/storage/Closures.h b/rts/include/rts/storage/Closures.h
index bfc8308c29..09c7bfdb88 100644
--- a/rts/include/rts/storage/Closures.h
+++ b/rts/include/rts/storage/Closures.h
@@ -285,6 +285,13 @@ typedef struct {
     StgHeader  header;
 } StgStopFrame;
 
+// Stack frame indicating that the stack's owning thread has finished.
+//
+// Closure types: RET_SMALL
+typedef struct {
+    StgHeader  header;
+    StgClosure *result;
+} StgDeadThreadFrame;
 
 // A function return stack frame: used when saving the state for a
 // garbage collection at a function entry point.  The function
diff --git a/rts/include/stg/MiscClosures.h b/rts/include/stg/MiscClosures.h
index e87eba0931..b5efd9636b 100644
--- a/rts/include/stg/MiscClosures.h
+++ b/rts/include/stg/MiscClosures.h
@@ -159,6 +159,7 @@ RTS_RET(stg_ctoi_t61);
 RTS_RET(stg_ctoi_t62);
 
 RTS_RET(stg_apply_interp);
+RTS_RET(stg_dead_thread);
 
 RTS_ENTRY(stg_IND);
 RTS_ENTRY(stg_IND_STATIC);
diff --git a/utils/deriveConstants/Main.hs b/utils/deriveConstants/Main.hs
index 9a57ecf682..ca17b061e4 100644
--- a/utils/deriveConstants/Main.hs
+++ b/utils/deriveConstants/Main.hs
@@ -418,9 +418,11 @@ wanteds os = concat
           ,structField  Both "StgEntCounter" "link"
           ,structField  Both "StgEntCounter" "entry_count"
 
-          ,closureSize Both "StgUpdateFrame"
-          ,closureSize C    "StgCatchFrame"
-          ,closureSize C    "StgStopFrame"
+          ,closureSize  Both "StgUpdateFrame"
+          ,closureSize  C    "StgCatchFrame"
+          ,closureSize  C    "StgStopFrame"
+          ,closureSize  C    "StgDeadThreadFrame"
+          ,closureField C    "StgDeadThreadFrame" "result"
 
           ,closureSize  Both "StgMutArrPtrs"
           ,closureField Both "StgMutArrPtrs" "ptrs"