Lock cfunction trampoline cache and freelist accesses. (JuliaLang#39621)

Co-authored-by: Valentin Churavy <[email protected]>
tekverkp · Feb 13, 2021 · 6468dcb · 6468dcb
1 parent 62a3b26
commit 6468dcb
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 9 deletions.
diff --git a/src/runtime_ccall.cpp b/src/runtime_ccall.cpp
@@ -210,9 +210,11 @@ extern "C" JL_DLLEXPORT char *jl_format_filename(const char *output_pattern)
 }
 
 
+static jl_mutex_t trampoline_lock; // for accesses to the cache and freelist
+
 static void *trampoline_freelist;
 
-static void *trampoline_alloc()
+static void *trampoline_alloc() // lock taken by caller
 {
  const int sz = 64; // oversized for most platforms. todo: use precise value?
  if (!trampoline_freelist) {
@@ -245,7 +247,7 @@ static void *trampoline_alloc()
  return tramp;
 }
 
-static void trampoline_free(void *tramp)
+static void trampoline_free(void *tramp) // lock taken by caller
 {
  *(void**)tramp = trampoline_freelist;
  trampoline_freelist = tramp;
@@ -260,17 +262,18 @@ static void trampoline_deleter(void **f)
  f[0] = NULL;
  f[2] = NULL;
  f[3] = NULL;
+ JL_LOCK_NOGC(&trampoline_lock);
  if (tramp)
  trampoline_free(tramp);
  if (fobj && cache)
  ptrhash_remove((htable_t*)cache, fobj);
  if (nval)
  free(nval);
+ JL_UNLOCK_NOGC(&trampoline_lock);
 }
 
 // Use of `cache` is not clobbered in JL_TRY
 JL_GCC_IGNORE_START("-Wclobbered")
-// TODO: need a thread lock around the cache access parts of this function
 extern "C" JL_DLLEXPORT
 jl_value_t *jl_get_cfunction_trampoline(
  // dynamic inputs:
@@ -284,6 +287,7 @@ jl_value_t *jl_get_cfunction_trampoline(
  jl_value_t **vals)
 {
  // lookup (fobj, vals) in cache
+ JL_LOCK_NOGC(&trampoline_lock);
  if (!cache->table)
  htable_new(cache, 1);
  if (fill != jl_emptysvec) {
@@ -295,6 +299,7 @@ jl_value_t *jl_get_cfunction_trampoline(
  }
  }
  void *tramp = ptrhash_get(cache, (void*)fobj);
+ JL_UNLOCK_NOGC(&trampoline_lock);
  if (tramp != HT_NOTFOUND) {
  assert((jl_datatype_t*)jl_typeof(tramp) == result_type);
  return (jl_value_t*)tramp;
@@ -347,10 +352,12 @@ jl_value_t *jl_get_cfunction_trampoline(
  free(nval);
  jl_rethrow();
  }
+ JL_LOCK_NOGC(&trampoline_lock);
  tramp = trampoline_alloc();
  ((void**)result)[0] = tramp;
  tramp = init_trampoline(tramp, nval);
  ptrhash_put(cache, (void*)fobj, result);
+ JL_UNLOCK_NOGC(&trampoline_lock);
  return result;
 }
 JL_GCC_IGNORE_STOP
diff --git a/test/threads_exec.jl b/test/threads_exec.jl
@@ -471,7 +471,6 @@ end
 
 function test_thread_cfunction()
  # ensure a runtime call to `get_trampoline` will be created
- # TODO: get_trampoline is not thread-safe (as this test shows)
  fs = [ Core.Box() for i in 1:1000 ]
  @noinline cf(f) = @cfunction $f Float64 ()
  cfs = Vector{Base.CFunction}(undef, length(fs))
@@ -494,11 +493,7 @@ function test_thread_cfunction()
  @test sum(ok) == 10000
 end
 if cfunction_closure
- if nthreads() == 1
- test_thread_cfunction()
- else
- @test_broken "cfunction trampoline code not thread-safe"
- end
+ test_thread_cfunction()
 end
 
 # Compare the two ways of checking if threading is enabled.