Merge pull request #15677 from JuliaLang/yyc/threads/fix

Workaround possible GC dead loop

base/atomics.jl

@@ -186,6 +186,8 @@ for op in [:+, :-, :max, :min]
             cmp = old
             old = atomic_cas!(var, cmp, new)
             reinterpret(IT, old) == reinterpret(IT, cmp) && return new
+            # Temporary solution before we have gc transition support in codegen.
+            ccall(:jl_gc_safepoint, Void, ())
         end
     end
 end

base/locks.jl

@@ -32,10 +32,7 @@ function lock!(l::TatasLock)
         end
         ccall(:jl_cpu_pause, Void, ())
         # Temporary solution before we have gc transition support in codegen.
-        # This could mess up gc state when we add codegen support.
-        # Use these as a safe point
-        gc_state = ccall(:jl_gc_safe_enter, Int8, ())
-        ccall(:jl_gc_safe_leave, Void, (Int8,), gc_state)
+        ccall(:jl_gc_safepoint, Void, ())
     end
 end
 
@@ -76,10 +73,7 @@ function lock!(l::RecursiveTatasLock)
         end
         ccall(:jl_cpu_pause, Void, ())
         # Temporary solution before we have gc transition support in codegen.
-        # This could mess up gc state when we add codegen support.
-        # Use these as a safe point
-        gc_state = ccall(:jl_gc_safe_enter, Int8, ())
-        ccall(:jl_gc_safe_leave, Void, (Int8,), gc_state)
+        ccall(:jl_gc_safepoint, Void, ())
     end
 end
 

src/ccall.cpp

@@ -1084,6 +1084,29 @@ static jl_cgval_t emit_ccall(jl_value_t **args, size_t nargs, jl_codectx_t *ctx)
         JL_GC_POP();
         return ghostValue(jl_void_type);
     }
+    if (fptr == &jl_gc_safepoint ||
+        ((!f_lib || (intptr_t)f_lib == 2) && f_name &&
+         strcmp(f_name, "jl_gc_safepoint") == 0)) {
+        assert(lrt == T_void);
+        assert(!isVa);
+        assert(nargt == 0);
+        JL_GC_POP();
+#ifdef JULIA_ENABLE_THREADING
+        builder.CreateFence(SequentiallyConsistent, SingleThread);
+        Value *addr;
+        if (imaging_mode) {
+            assert(ctx->signalPage);
+            addr = ctx->signalPage;
+        }
+        else {
+            addr = builder.CreateIntToPtr(
+                ConstantInt::get(T_size, (uintptr_t)jl_gc_signal_page), T_pint8);
+        }
+        builder.CreateLoad(addr, true);
+        builder.CreateFence(SequentiallyConsistent, SingleThread);
+#endif
+        return ghostValue(jl_void_type);
+    }
     if (fptr == (void(*)(void))&jl_is_leaf_type ||
         ((f_lib==NULL || (intptr_t)f_lib==2)
          && f_name && !strcmp(f_name, "jl_is_leaf_type"))) {

src/codegen.cpp

@@ -337,6 +337,8 @@ static GlobalVariable *jltls_states_var;
 // Imaging mode only
 static GlobalVariable *jltls_states_func_ptr = NULL;
 static size_t jltls_states_func_idx = 0;
+static GlobalVariable *jl_gc_signal_page_ptr = NULL;
+static size_t jl_gc_signal_page_idx = 0;
 #endif
 
 // important functions
@@ -567,6 +569,9 @@ typedef struct {
     std::vector<bool> inbounds;
 
     CallInst *ptlsStates;
+#ifdef JULIA_ENABLE_THREADING
+    Value *signalPage;
+#endif
 
     llvm::DIBuilder *dbuilder;
     bool debug_enabled;
@@ -3493,6 +3498,12 @@ static void allocate_gc_frame(BasicBlock *b0, jl_codectx_t *ctx)
 {
     // allocate a placeholder gc instruction
     ctx->ptlsStates = builder.CreateCall(prepare_call(jltls_states_func));
+#ifdef JULIA_ENABLE_THREADING
+    if (imaging_mode) {
+        ctx->signalPage =
+            tbaa_decorate(tbaa_const, builder.CreateLoad(jl_declare_global(jl_Module, jl_gc_signal_page_ptr)));
+    }
+#endif
 }
 
 void jl_codegen_finalize_temp_arg(CallInst *ptlsStates, Type *T_pjlvalue);
@@ -3519,35 +3530,16 @@ static void finalize_gc_frame(Function *F)
 
     jl_codegen_finalize_temp_arg(ptlsStates, T_pjlvalue);
 
-    GlobalVariable *oldGV = NULL;
-#ifdef JULIA_ENABLE_THREADING
-    if (imaging_mode)
-        oldGV = jltls_states_func_ptr;
-#else
-    oldGV = jltls_states_var;
-#endif
-
-    GlobalVariable *GV = NULL;
-    if (oldGV) {
-        GV = M->getGlobalVariable(oldGV->getName(), true /* AllowLocal */);
-        if (GV == NULL) {
-            GV = new GlobalVariable(*M, oldGV->getType()->getElementType(),
-                                    oldGV->isConstant(),
-                                    GlobalValue::ExternalLinkage, NULL,
-                                    oldGV->getName());
-            GV->copyAttributesFrom(oldGV);
-        }
-    }
-
 #ifdef JULIA_ENABLE_THREADING
-    if (GV) {
+    if (imaging_mode) {
+        GlobalVariable *GV = jl_declare_global(M, jltls_states_func_ptr);
         Value *getter = tbaa_decorate(tbaa_const,
                                       new LoadInst(GV, "", ptlsStates));
         ptlsStates->setCalledFunction(getter);
     }
     ptlsStates->setAttributes(jltls_states_func->getAttributes());
 #else
-    ptlsStates->replaceAllUsesWith(GV);
+    ptlsStates->replaceAllUsesWith(jl_declare_global(M, jltls_states_var));
     ptlsStates->eraseFromParent();
 #endif
 }
@@ -5122,27 +5114,14 @@ static void init_julia_llvm_env(Module *m)
     add_named_global(jltls_states_func, jl_get_ptls_states_getter());
     if (imaging_mode) {
         PointerType *pfunctype = jltls_states_func->getFunctionType()->getPointerTo();
-        // This is **NOT** a external variable or a normal global variable
-        // This is a special internal global slot with a special index
-        // in the global variable table.
         jltls_states_func_ptr =
-            new GlobalVariable(*m, pfunctype,
-                               false, GlobalVariable::InternalLinkage,
-                               ConstantPointerNull::get(pfunctype),
-                               "jl_get_ptls_states.ptr");
-        addComdat(jltls_states_func_ptr);
-        // make the pointer valid for this session
-#if defined(USE_MCJIT) || defined(USE_ORCJIT)
-        auto p = new uintptr_t(0);
-        jl_ExecutionEngine->addGlobalMapping(jltls_states_func_ptr->getName(),
-                                             (uintptr_t)p);
-#else
-        uintptr_t *p = (uintptr_t*)jl_ExecutionEngine->getPointerToGlobal(jltls_states_func_ptr);
-#endif
-        *p = (uintptr_t)jl_get_ptls_states_getter();
-        jl_sysimg_gvars.push_back(ConstantExpr::getBitCast(jltls_states_func_ptr,
-                                                           T_psize));
-        jltls_states_func_idx = jl_sysimg_gvars.size();
+            jl_emit_sysimg_slot(m, pfunctype, "jl_get_ptls_states.ptr",
+                                (uintptr_t)jl_get_ptls_states_getter(),
+                                jltls_states_func_idx);
+        jl_gc_signal_page_ptr =
+            jl_emit_sysimg_slot(m, T_pint8, "jl_gc_signal_page.ptr",
+                                (uintptr_t)jl_gc_signal_page,
+                                jl_gc_signal_page_idx);
     }
 
 #endif

src/dump.c

@@ -236,6 +236,9 @@ static int jl_load_sysimg_so(void)
                                                    "jl_ptls_states_getter_idx");
         *sysimg_gvars[tls_getter_idx - 1] =
             (jl_value_t*)jl_get_ptls_states_getter();
+        size_t signal_page_idx = *(size_t*)jl_dlsym(jl_sysimg_handle,
+                                                    "jl_gc_signal_page_idx");
+        *sysimg_gvars[signal_page_idx - 1] = (jl_value_t*)jl_gc_signal_page;
 #endif
         const char *cpu_target = (const char*)jl_dlsym(jl_sysimg_handle, "jl_sysimg_cpu_target");
         if (strcmp(cpu_target,jl_options.cpu_target) != 0)

src/jitlayers.cpp

@@ -776,6 +776,52 @@ static void* jl_emit_and_add_to_shadow(GlobalVariable *gv, void *gvarinit = NULL
 #endif
 }
 
+// Emit a slot in the system image to be filled at sysimg init time.
+// Returns the global var. Fill `idx` with 1-base index in the sysimg gv.
+// Use as an optimization for runtime constant addresses to have one less
+// load.
+static GlobalVariable *jl_emit_sysimg_slot(Module *m, Type *typ, const char *name,
+                                           uintptr_t init, size_t &idx)
+{
+    assert(imaging_mode);
+    // This is **NOT** a external variable or a normal global variable
+    // This is a special internal global slot with a special index
+    // in the global variable table.
+    GlobalVariable *gv = new GlobalVariable(*m, typ, false,
+                                            GlobalVariable::InternalLinkage,
+                                            ConstantPointerNull::get((PointerType*)typ), name);
+    addComdat(gv);
+    // make the pointer valid for this session
+#if defined(USE_MCJIT) || defined(USE_ORCJIT)
+    auto p = new uintptr_t(init);
+    jl_ExecutionEngine->addGlobalMapping(gv->getName(), (uintptr_t)p);
+#else
+    uintptr_t *p = (uintptr_t*)jl_ExecutionEngine->getPointerToGlobal(gv);
+    *p = init;
+#endif
+    jl_sysimg_gvars.push_back(ConstantExpr::getBitCast(gv, T_psize));
+    idx = jl_sysimg_gvars.size();
+    return gv;
+}
+
+// Make sure `GV` belongs to `M` or create a declaration of `GV` in `M` (to
+// be linked later) that has the same properties.
+static GlobalVariable *jl_declare_global(Module *M, GlobalVariable *oldGV)
+{
+    if (!oldGV)
+        return NULL;
+    GlobalVariable *GV = M->getGlobalVariable(oldGV->getName(),
+                                              true /* AllowLocal */);
+    if (GV)
+        return GV;
+    GV = new GlobalVariable(*M, oldGV->getType()->getElementType(),
+                            oldGV->isConstant(),
+                            GlobalValue::ExternalLinkage, NULL,
+                            oldGV->getName());
+    GV->copyAttributesFrom(oldGV);
+    return GV;
+}
+
 static void* jl_get_global(GlobalVariable *gv)
 {
 #if defined(USE_MCJIT) || defined(USE_ORCJIT)
@@ -848,6 +894,12 @@ static void jl_gen_llvm_globaldata(llvm::Module *mod, ValueToValueMapTy &VMap,
                                  GlobalVariable::ExternalLinkage,
                                  ConstantInt::get(T_size, jltls_states_func_idx),
                                  "jl_ptls_states_getter_idx"));
+    addComdat(new GlobalVariable(*mod,
+                                 T_size,
+                                 true,
+                                 GlobalVariable::ExternalLinkage,
+                                 ConstantInt::get(T_size, jl_gc_signal_page_idx),
+                                 "jl_gc_signal_page_idx"));
 #endif
 
     Constant *feature_string = ConstantDataArray::getString(jl_LLVMContext, jl_options.cpu_target);

src/jlapi.c

@@ -342,6 +342,11 @@ JL_DLLEXPORT void (jl_gc_safe_leave)(int8_t state)
     jl_gc_safe_leave(state);
 }
 
+JL_DLLEXPORT void (jl_gc_safepoint)(void)
+{
+    jl_gc_safepoint();
+}
+
 JL_DLLEXPORT void (jl_cpu_pause)(void)
 {
     jl_cpu_pause();

src/julia_threads.h

@@ -256,16 +256,15 @@ JL_DLLEXPORT void jl_set_ptls_states_getter(jl_get_ptls_states_func f);
 // Make sure jl_gc_state() is always a rvalue
 #define jl_gc_state() ((int8_t)(jl_get_ptls_states()->gc_state))
 JL_DLLEXPORT extern volatile size_t *jl_gc_signal_page;
-STATIC_INLINE void jl_gc_safepoint(void)
-{
-    // This triggers a SegFault when we are in GC
-    // Assign it to a variable to make sure the compiler emit the load
-    // and to avoid Clang warning for -Wunused-volatile-lvalue
-    jl_signal_fence();
-    size_t v = *jl_gc_signal_page;
-    jl_signal_fence();
-    (void)v;
-}
+// This triggers a SegFault when we are in GC
+// Assign it to a variable to make sure the compiler emit the load
+// and to avoid Clang warning for -Wunused-volatile-lvalue
+#define jl_gc_safepoint() do {                          \
+        jl_signal_fence();                              \
+        size_t safepoint_load = *jl_gc_signal_page;     \
+        jl_signal_fence();                              \
+        (void)safepoint_load;                           \
+    } while (0)
 STATIC_INLINE int8_t jl_gc_state_set(int8_t state, int8_t old_state)
 {
     jl_get_ptls_states()->gc_state = state;
@@ -284,6 +283,7 @@ STATIC_INLINE int8_t jl_gc_state_save_and_set(int8_t state)
 #define jl_gc_unsafe_leave(state) ((void)jl_gc_state_set((state), 0))
 #define jl_gc_safe_enter() jl_gc_state_save_and_set(JL_GC_STATE_SAFE)
 #define jl_gc_safe_leave(state) ((void)jl_gc_state_set((state), JL_GC_STATE_SAFE))
+JL_DLLEXPORT void (jl_gc_safepoint)(void);
 
 // Locks
 #ifdef JULIA_ENABLE_THREADING

test/threads.jl

@@ -166,6 +166,8 @@ function test_atomic_read(commbuf::CommBuf, n::Int)
         var1 = commbuf.var1[]
         correct &= var1 >= var2
         var1 == n && break
+        # Temporary solution before we have gc transition support in codegen.
+        ccall(:jl_gc_safepoint, Void, ())
     end
     commbuf.correct_read = correct
 end
@@ -209,6 +211,8 @@ function test_fence(p::Peterson, id::Int, n::Int)
         atomic_fence()
         while p.flag[otherid][] != 0 && p.turn[] == otherid
             # busy wait
+            # Temporary solution before we have gc transition support in codegen.
+            ccall(:jl_gc_safepoint, Void, ())
         end
         # critical section
         p.critical[id][] = 1
@@ -260,6 +264,8 @@ function test_atomic_cas!{T}(var::Atomic{T}, range::StepRange{Int,Int})
         while true
             old = atomic_cas!(var, T(i-1), T(i))
             old == T(i-1) && break
+            # Temporary solution before we have gc transition support in codegen.
+            ccall(:jl_gc_safepoint, Void, ())
         end
     end
 end