Prevent use of unsaved thread context registers in incremental GC (Windows)

(fix of commits 449eda034, 190e18c75)

During incremental collection GC_push_all_stacks() may be called when
the world is not stopped.  If the context registers and sp value are
saved during thread suspension then GC_push_all_stacks() gets these
values of the previous GC cycle or, even, gets all zeros instead the
context registers and sp on the first GC after the thread creation.
This commit fixes this by calling GetThreadContext in GC_push_stack_for
if thread is not suspended.

* win32_threads.c (first_thread): Refine the comment.
* win32_threads.c [!GC_NO_THREADS_DISCOVERY && RETRY_GET_THREAD_CONTEXT]
(GC_delete_gc_thread_no_free): Reset context_sp if GC_win32_dll_threads.
* win32_threads.c (GC_push_stack_for): Change the assertion expression
from thread->suspended to thread->suspended||!GC_world_stopped (the
assertion is off unless THREAD_LOCAL_ALLOC).
* win32_threads.c [RETRY_GET_THREAD_CONTEXT] (GC_push_stack_for): If
thread is not suspended then call GetThreadContext(), if it fails then
use sp value and the context registers saved during the latest
stop-the-world; add comments.
* win32_threads.c (GC_push_all_stacks): Add the title comment (copied
from pthread_stop_world.c).

1 files changed, 31 insertions, 10 deletions

diff --git a/win32_threads.c b/win32_threads.c
index 650fe815..c04ff765 100644
--- a/win32_threads.c
+++ b/win32_threads.c
@@ -352,8 +352,8 @@ STATIC volatile LONG GC_max_thread_index = 0;
 STATIC GC_thread GC_threads[THREAD_TABLE_SZ];
 
 /* It may not be safe to allocate when we register the first thread.    */
-/* Thus we allocated one statically.  It does not contain any field we  */
-/* need to push ("next" and "status" fields are unused).                */
+/* Thus we allocated one statically.  It does not contain any pointer   */
+/* field we need to push ("next" and "status" fields are unused).       */
 static struct GC_Thread_Rep first_thread;
 static GC_bool first_thread_used = FALSE;
 
@@ -693,6 +693,9 @@ STATIC void GC_delete_gc_thread_no_free(GC_vthread t)
       /* see GC_stop_world() for the information.                       */
       t -> stack_base = 0;
       t -> id = 0;
+#     ifdef RETRY_GET_THREAD_CONTEXT
+        t -> context_sp = NULL;
+#     endif
       AO_store_release(&t->tm.in_use, FALSE);
     } else
 # endif
@@ -1509,20 +1512,36 @@ STATIC word GC_push_stack_for(GC_thread thread, DWORD me)
       /* require looping.                                               */
       word *regs = thread->context_regs;
 
-      GC_ASSERT(thread->suspended);
-      sp = thread->context_sp;
+      if (thread->suspended) {
+        sp = thread->context_sp;
+      } else
 #   else
-      /* For unblocked threads call GetThreadContext(). */
       word regs[PUSHED_REGS_COUNT];
-      {
+#   endif
+
+      /* else */ {
         CONTEXT context;
 
-        GC_ASSERT(thread->suspended);
+        /* For unblocked threads call GetThreadContext().       */
         context.ContextFlags = GET_THREAD_CONTEXT_FLAGS;
-        if (!GetThreadContext(THREAD_HANDLE(thread), &context))
-          ABORT("GetThreadContext failed");
-        sp = copy_ptr_regs(regs, &context);
+        if (GetThreadContext(THREAD_HANDLE(thread), &context)) {
+          sp = copy_ptr_regs(regs, &context);
+        } else {
+#         ifdef RETRY_GET_THREAD_CONTEXT
+            /* At least, try to use the stale context if saved. */
+            sp = thread->context_sp;
+            if (NULL == sp) {
+              /* Skip the current thread, anyway its stack will */
+              /* be pushed when the world is stopped.           */
+              return 0;
+            }
+#         else
+            ABORT("GetThreadContext failed");
+#         endif
+        }
       }
+#   ifdef THREAD_LOCAL_ALLOC
+      GC_ASSERT(thread->suspended || !GC_world_stopped);
 #   endif
 
 #   ifdef WOW64_THREAD_CONTEXT_WORKAROUND
@@ -1670,6 +1689,8 @@ STATIC word GC_push_stack_for(GC_thread thread, DWORD me)
   return thread->stack_base - sp; /* stack grows down */
 }
 
+/* We hold allocation lock.  Should do exactly the right thing if the   */
+/* world is stopped.  Should not fail if it isn't.                      */
 GC_INNER void GC_push_all_stacks(void)
 {
   DWORD thread_id = GetCurrentThreadId();