Lock libmap and use atomic load/store on library handle/symbol GVs

Fix JuliaLang#18020

src/ccall.cpp

@@ -142,10 +142,21 @@ static Value *runtime_sym_lookup(PointerType *funcptype, const char *f_lib,
  // *llvmgv = jl_load_and_lookup(f_lib, f_name, libptrgv);
  // }
  // return (*llvmgv)
+ BasicBlock *enter_bb = builder.GetInsertBlock();
  BasicBlock *dlsym_lookup = BasicBlock::Create(jl_LLVMContext, "dlsym");
  BasicBlock *ccall_bb = BasicBlock::Create(jl_LLVMContext, "ccall");
  Constant *initnul = ConstantPointerNull::get((PointerType*)T_pvoidfunc);
- builder.CreateCondBr(builder.CreateICmpNE(builder.CreateLoad(llvmgv), initnul), ccall_bb, dlsym_lookup);
+ LoadInst *llvmf_orig = builder.CreateAlignedLoad(llvmgv, sizeof(void*));
+ // This in principle needs a consume ordering so that load from
+ // this pointer sees valid value. However, this is not supported by
+ // LLVM (or agreed on in the C/C++ standard FWIW) and should be
+ // almost impossible to happen on every platform we support since this
+ // ordering is enforced by the hardware and LLVM has to speculate a
+ // invalid load from the `cglobal` but doesn't depend on the `cglobal`
+ // value for this to happen.
+ // llvmf_orig->setAtomic(AtomicOrdering::Consume);
+ builder.CreateCondBr(builder.CreateICmpNE(llvmf_orig, initnul),
+ ccall_bb, dlsym_lookup);
 
  assert(f->getParent() != NULL);
  f->getBasicBlockList().push_back(dlsym_lookup);
@@ -162,13 +173,20 @@ static Value *runtime_sym_lookup(PointerType *funcptype, const char *f_lib,
 #else
  Value *llvmf = builder.CreateCall3(prepare_call(jldlsym_func), libname, stringConstPtr(f_name), libptrgv);
 #endif
- builder.CreateStore(llvmf, llvmgv);
+ auto store = builder.CreateAlignedStore(llvmf, llvmgv, sizeof(void*));
+# ifdef LLVM39
+ store->setAtomic(AtomicOrdering::Release);
+# else
+ store->setAtomic(Release);
+# endif
  builder.CreateBr(ccall_bb);
 
  f->getBasicBlockList().push_back(ccall_bb);
  builder.SetInsertPoint(ccall_bb);
- llvmf = builder.CreateLoad(llvmgv);
- return builder.CreatePointerCast(llvmf,funcptype);
+ PHINode *p = builder.CreatePHI(T_pvoidfunc, 2);
+ p->addIncoming(llvmf_orig, enter_bb);
+ p->addIncoming(llvmf, dlsym_lookup);
+ return builder.CreatePointerCast(p, funcptype);
 }
 
 static Value *runtime_sym_lookup(PointerType *funcptype, const char *f_lib,
@@ -247,7 +265,12 @@ static Value *emit_plt(FunctionType *functype, const AttributeSet &attrs,
  builder.SetInsertPoint(b0);
  Value *ptr = runtime_sym_lookup(funcptype, f_lib, f_name, plt, libptrgv,
  llvmgv, runtime_lib);
- builder.CreateStore(builder.CreateBitCast(ptr, T_pvoidfunc), got);
+ auto store = builder.CreateAlignedStore(builder.CreateBitCast(ptr, T_pvoidfunc), got, sizeof(void*));
+# ifdef LLVM39
+ store->setAtomic(AtomicOrdering::Release);
+# else
+ store->setAtomic(Release);
+# endif
  SmallVector<Value*, 16> args;
  for (Function::arg_iterator arg = plt->arg_begin(), arg_e = plt->arg_end(); arg != arg_e; ++arg)
  args.push_back(&*arg);
@@ -292,7 +315,13 @@ static Value *emit_plt(FunctionType *functype, const AttributeSet &attrs,
  assert(!LM.m);
  got = prepare_global(slot);
  }
- return builder.CreateBitCast(builder.CreateLoad(got), funcptype);
+ LoadInst *got_val = builder.CreateAlignedLoad(got, sizeof(void*));
+ // See comment in `runtime_sym_lookup` above. This in principle needs a
+ // consume ordering too. This is even less likely to cause issue though
+ // since the only thing we do to this loaded pointer is to call it
+ // immediately.
+ // got_val->setAtomic(AtomicOrdering::Consume);
+ return builder.CreateBitCast(got_val, funcptype);
 }
 
 // --- ABI Implementations ---

src/runtime_ccall.cpp

@@ -133,7 +133,7 @@ extern "C" void jl_init_runtime_ccall(void)
 
 // map from user-specified lib names to handles
 static std::map<std::string, void*> libMap;
-
+static jl_mutex_t libmap_lock;
 extern "C"
 void *jl_get_library(const char *f_lib)
 {
@@ -146,21 +146,27 @@ void *jl_get_library(const char *f_lib)
 #endif
  if (f_lib == NULL)
  return jl_RTLD_DEFAULT_handle;
- hnd = libMap[f_lib];
+ JL_LOCK_NOGC(&libmap_lock);
+ // This is the only operation we do on the map, which doesn't invalidate
+ // any references or iterators.
+ void **map_slot = &libMap[f_lib];
+ JL_UNLOCK_NOGC(&libmap_lock);
+ hnd = jl_atomic_load_acquire(map_slot);
  if (hnd != NULL)
  return hnd;
+ // We might run this concurrently on two threads but it doesn't matter.
  hnd = jl_load_dynamic_library(f_lib, JL_RTLD_DEFAULT);
  if (hnd != NULL)
- libMap[f_lib] = hnd;
+ jl_atomic_store_release(map_slot, hnd);
  return hnd;
 }
 
 extern "C" JL_DLLEXPORT
 void *jl_load_and_lookup(const char *f_lib, const char *f_name, void **hnd)
 {
- void *handle = *hnd;
+ void *handle = jl_atomic_load_acquire(hnd);
  if (!handle)
- *hnd = handle = jl_get_library(f_lib);
+ jl_atomic_store_release(hnd, (handle = jl_get_library(f_lib)));
  return jl_dlsym(handle, f_name);
 }
 

test/threads.jl

@@ -385,3 +385,15 @@ if ccall(:jl_threading_enabled, Cint, ()) == 0
 else
  @test_throws ErrorException cglobal(:jl_tls_states)
 end
+
+# Thread safety of `jl_load_and_lookup`.
+function test_load_and_lookup_18020(n)
+ @threads for i in 1:n
+ try
+ ccall(:jl_load_and_lookup,
+ Ptr{Void}, (Cstring, Cstring, Ref{Ptr{Void}}),
+ "$i", :f, C_NULL)
+ end
+ end
+end
+test_load_and_lookup_18020(10000)