From fe0f25adfefccd474b3388ff14e90b45327bab72 Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Sat, 25 Jul 2020 13:17:30 +0200
Subject: [PATCH 01/10] Serialize GAP calls from within Julia.

---
 src/GAP.jl          | 21 ++++++-----
 src/ccalls.jl       | 92 ++++++++++++++++++++++++++-------------------
 src/gap_to_julia.jl |  4 +-
 src/julia_to_gap.jl |  2 +-
 src/sync.jl         | 65 ++++++++++++++++++++++++++++++++
 5 files changed, 133 insertions(+), 51 deletions(-)
 create mode 100644 src/sync.jl

diff --git a/src/GAP.jl b/src/GAP.jl
index 624163a90..88dc79089 100644
--- a/src/GAP.jl
+++ b/src/GAP.jl
@@ -19,6 +19,7 @@ if !isfile(deps_jl)
     Pkg.build("GAP")
 end
 include(deps_jl)
+include("sync.jl")
 
 
 """
@@ -182,7 +183,7 @@ function initialize(argv::Array{String,1})
     # detect by checking for the value of __JULIAINTERNAL_LOADED_FROM_JULIA).
     append!(argv, ["-c", """BindGlobal("__JULIAINTERNAL_LOADED_FROM_JULIA", true );"""])
 
-    ccall(
+    @sync ccall(
         Libdl.dlsym(libgap_handle, :GAP_Initialize),
         Cvoid,
         (Int32, Ptr{Ptr{UInt8}}, Ptr{Cvoid}, Ptr{Cvoid}, Cuint),
@@ -199,7 +200,7 @@ function initialize(argv::Array{String,1})
     # (perhaps this could be a return value for GAP_Initialize?), so instead
     # we check for the presence of a global variable which we ensure is
     # declared near the end of init.g via a `-c` command line argument to GAP.
-    val = ccall(
+    val = @sync ccall(
         Libdl.dlsym(libgap_handle, :GAP_ValueGlobalVariable),
         Ptr{Cvoid},
         (Ptr{Cuchar},),
@@ -214,13 +215,13 @@ function initialize(argv::Array{String,1})
         # FORCE_QUIT_GAP with no arguments, which just calls the `exit`
         # function of the  C standard library with the appropriate exit code.
         # But as mentioned, just before that, it runs `jl_atexit_hook`.
-        FORCE_QUIT_GAP = ccall(
+        FORCE_QUIT_GAP = @sync ccall(
             Libdl.dlsym(libgap_handle, :GAP_ValueGlobalVariable),
             Ptr{Cvoid},
             (Ptr{Cuchar},),
             "FORCE_QUIT_GAP",
         )
-        ccall(
+        @sync ccall(
             Libdl.dlsym(libgap_handle, :GAP_CallFuncArray),
             Ptr{Cvoid},
             (Ptr{Cvoid}, Culonglong, Ptr{Cvoid}),
@@ -264,7 +265,7 @@ function initialize(argv::Array{String,1})
     @assert T_HVARS == Base.invokelatest(ValueGlobalVariable,:T_HVARS)
 
     # load JuliaInterface
-    loadpackage_return = ccall(
+    loadpackage_return = @sync ccall(
         Libdl.dlsym(libgap_handle, :GAP_EvalString),
         Ptr{Cvoid},
         (Ptr{UInt8},),
@@ -276,7 +277,7 @@ function initialize(argv::Array{String,1})
 
     # If we are in "stand-alone mode", stop here
     if isdefined(Main, :__GAP_ARGS__)
-        ccall(Libdl.dlsym(libgap_handle, :SyInstallAnswerIntr), Cvoid, ())
+        @sync ccall(Libdl.dlsym(libgap_handle, :SyInstallAnswerIntr), Cvoid, ())
         return
     end
 
@@ -290,7 +291,7 @@ function initialize(argv::Array{String,1})
 end
 
 function finalize()
-    ccall((:GAP_finalize, "libgap"), Cvoid, ())
+    @sync ccall((:GAP_finalize, "libgap"), Cvoid, ())
 end
 
 function run_it()
@@ -383,10 +384,10 @@ function prompt()
     # save the current SIGINT handler
     # HACK: the hardcoded value for SIG_DFL is not portable, revise this
     # install GAP's SIGINT handler
-    old_sigint = ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, C_NULL)
+    old_sigint = @sync ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, C_NULL)
 
     # install GAP's SIGINT handler
-    ccall(:SyInstallAnswerIntr, Cvoid, ())
+    @sync ccall(:SyInstallAnswerIntr, Cvoid, ())
 
     # restore GAP's error output
     disable_error_handler = true
@@ -404,7 +405,7 @@ function prompt()
     Globals.BreakOnError = false
 
     # restore signal handler
-    ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, old_sigint)
+    @sync ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, old_sigint)
 
     # restore GAP error handler
     disable_error_handler = false
diff --git a/src/ccalls.jl b/src/ccalls.jl
index 592c92d2f..f026c2c7c 100644
--- a/src/ccalls.jl
+++ b/src/ccalls.jl
@@ -13,13 +13,13 @@ function _GAP_TO_JULIA(ptr::Ptr{Cvoid})
     elseif as_int & 2 == 2
         return reinterpret(FFE, ptr)
     end
-    return ccall(:julia_gap, Any, (Ptr{Cvoid},), ptr)
+    return @sync ccall(:julia_gap, Any, (Ptr{Cvoid},), ptr)
 end
 
 #
 # low-level Julia -> GAP conversion
 #
-_JULIA_TO_GAP(val::Any) = ccall(:gap_julia, Ptr{Cvoid}, (Any,), val)
+_JULIA_TO_GAP(val::Any) = @sync ccall(:gap_julia, Ptr{Cvoid}, (Any,), val)
 #_JULIA_TO_GAP(x::Bool) = x ? gap_true : gap_false
 _JULIA_TO_GAP(x::FFE) = reinterpret(Ptr{Cvoid}, x)
 _JULIA_TO_GAP(x::GapObj) = pointer_from_objref(x)
@@ -28,12 +28,12 @@ function _JULIA_TO_GAP(x::Int)
     if x in -1<<60:(1<<60-1)
         return Ptr{Cvoid}(x << 2 | 1)
     end
-    return ccall(:ObjInt_Int, Ptr{Cvoid}, (Int,), x)
+    return @sync_noexcept ccall(:ObjInt_Int, Ptr{Cvoid}, (Int,), x)
 end
 
 
 function evalstr_ex(cmd::String)
-    res = ccall(:GAP_EvalString, GapObj, (Ptr{UInt8},), cmd)
+    res = @sync ccall(:GAP_EvalString, GapObj, (Ptr{UInt8},), cmd)
     return res
 end
 
@@ -73,7 +73,8 @@ end
 # Retrieve the value of a global GAP variable given its name. This function
 # returns a raw Ptr value, and should only be called by plumbing code.
 function _ValueGlobalVariable(name::Union{AbstractString,Symbol})
-    return ccall(:GAP_ValueGlobalVariable, Ptr{Cvoid}, (Ptr{UInt8},), name)
+    return @sync_noexcept ccall(:GAP_ValueGlobalVariable, Ptr{Cvoid},
+        (Ptr{UInt8},), name)
 end
 
 function ValueGlobalVariable(name::Union{AbstractString,Symbol})
@@ -83,51 +84,60 @@ end
 
 # Test whether the global GAP variable with the given name can be assigned to.
 function CanAssignGlobalVariable(name::Union{AbstractString,Symbol})
-    ccall(:GAP_CanAssignGlobalVariable, Bool, (Ptr{UInt8},), name)
+    @sync_noexcept ccall(:GAP_CanAssignGlobalVariable, Bool, (Ptr{UInt8},),
+        name)
 end
 
 # Assign a value to the global GAP variable with the given name. This function
 # assigns a raw Ptr value, and should only be called by plumbing code.
 function _AssignGlobalVariable(name::Union{AbstractString,Symbol}, value::Ptr{Cvoid})
-    ccall(:GAP_AssignGlobalVariable, Cvoid, (Ptr{UInt8}, Ptr{Cvoid}), name, value)
+    @sync_noexcept ccall(:GAP_AssignGlobalVariable, Cvoid, (Ptr{UInt8},
+        Ptr{Cvoid}), name, value)
 end
 
 # Assign a value to the global GAP variable with the given name.
 function AssignGlobalVariable(name::Union{AbstractString,Symbol}, value::Any)
-    if !CanAssignGlobalVariable(name)
-        error("cannot assing to $name in GAP")
+    @sync begin
+      if !CanAssignGlobalVariable(name)
+          error("cannot assing to $name in GAP")
+      end
+      tmp = _JULIA_TO_GAP(value)
+      _AssignGlobalVariable(name, tmp)
     end
-    tmp = _JULIA_TO_GAP(value)
-    _AssignGlobalVariable(name, tmp)
 end
 
-MakeString(val::String) = ccall(:GAP_MakeString, GapObj, (Ptr{UInt8},), val)
+MakeString(val::String) =
+    @sync_noexcept ccall(:GAP_MakeString, GapObj, (Ptr{UInt8},), val)
 
 function CSTR_STRING(val::GapObj)
-    char_ptr = ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
+    char_ptr = @sync_noexcept ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
     return deepcopy(unsafe_string(char_ptr))::String
 end
 
 function CSTR_STRING_AS_ARRAY(val::GapObj)::Array{UInt8,1}
-    string_len = Int64(ccall(:GAP_LenString, Cuint, (Any,), val))
-    char_ptr = ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
+    string_len = @sync_noexcept Int64(ccall(:GAP_LenString, Cuint, (Any,), val))
+    char_ptr = @sync_noexcept ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
     return deepcopy(unsafe_wrap(Array{UInt8,1}, char_ptr, string_len))
 end
 
 
-NewPlist(capacity::Int64) = ccall(:GAP_NewPlist, GapObj, (Int64,), capacity)
+NewPlist(capacity::Int64) =
+    @sync_noexcept ccall(:GAP_NewPlist, GapObj, (Int64,), capacity)
 NewPrecord(capacity::Int64) = ccall(:GAP_NewPrecord, GapObj, (Int64,), capacity)
-NEW_MACFLOAT(x::Float64) = ccall(:NEW_MACFLOAT, GapObj, (Cdouble,), x)
-ValueMacFloat(x::GapObj) = ccall(:GAP_ValueMacFloat, Cdouble, (Any,), x)
-CharWithValue(x::Cuchar) = ccall(:GAP_CharWithValue, GapObj, (Cuchar,), x)
-NewJuliaFunc(x::Function) = ccall(:NewJuliaFunc, GapObj, (Any,), x)
+NEW_MACFLOAT(x::Float64) =
+    @sync_noexcept ccall(:NEW_MACFLOAT, GapObj, (Cdouble,), x)
+ValueMacFloat(x::GapObj) =
+    @sync_noexcept ccall(:GAP_ValueMacFloat, Cdouble, (Any,), x)
+CharWithValue(x::Cuchar) =
+    @sync_noexcept ccall(:GAP_CharWithValue, GapObj, (Cuchar,), x)
+NewJuliaFunc(x::Function) =
+    @sync_noexcept ccall(:NewJuliaFunc, GapObj, (Any,), x)
 
 function ElmList(x::GapObj, position)
-    o = ccall(:GAP_ElmList, Ptr{Cvoid}, (Any, Culong), x, Culong(position))
+    o = @sync ccall(:GAP_ElmList, Ptr{Cvoid}, (Any, Culong), x, Culong(position))
     return _GAP_TO_JULIA(o)
 end
 
-
 """
     call_gap_func(func::GapObj, args...; kwargs...)
 
@@ -170,15 +180,17 @@ function call_gap_func(func::GapObj, args...; kwargs...)
         options = true
     end
     try
-        if TNUM_OBJ(func) == T_FUNCTION && length(args) <= 6
-            result = _call_gap_func(func, args...)
-        else
-            result = ccall(:call_gap_func, Any, (Any, Any), func, args)
-        end
-        if options
-            Globals.PopOptions()
+        @sync begin
+            if TNUM_OBJ(func) == T_FUNCTION && length(args) <= 6
+                result = _call_gap_func(func, args...)
+            else
+                result = ccall(:call_gap_func, Any, (Any, Any), func, args)
+            end
+            if options
+                Globals.PopOptions()
+            end
+            result
         end
-        return result
     catch
         if options
             Globals.ResetOptionsStack()
@@ -343,11 +355,13 @@ Main
 const Globals = GlobalsType()
 
 function getproperty(::GlobalsType, name::Symbol)
-    v = _ValueGlobalVariable(name)
-    if v === C_NULL
-        error("GAP variable $name not bound")
+    @sync begin
+        v = _ValueGlobalVariable(name)
+        if v === C_NULL
+            error("GAP variable $name not bound")
+        end
+        _GAP_TO_JULIA(v)
     end
-    return _GAP_TO_JULIA(v)
 end
 
 function hasproperty(::GlobalsType, name::Symbol)
@@ -355,11 +369,13 @@ function hasproperty(::GlobalsType, name::Symbol)
 end
 
 function setproperty!(::GlobalsType, name::Symbol, val::Any)
-    if !CanAssignGlobalVariable(name)
-        error("cannot assing to $name in GAP")
+    @sync begin
+        if !CanAssignGlobalVariable(name)
+            error("cannot assing to $name in GAP")
+        end
+        tmp = (val === nothing) ? C_NULL : _JULIA_TO_GAP(val)
+        _AssignGlobalVariable(name, tmp)
     end
-    tmp = (val === nothing) ? C_NULL : _JULIA_TO_GAP(val)
-    _AssignGlobalVariable(name, tmp)
 end
 
 propertynames(::GlobalsType) = gap_to_julia(Vector{Symbol}, Globals.NamesGVars())
diff --git a/src/gap_to_julia.jl b/src/gap_to_julia.jl
index 61e784f9f..73a4f2cd4 100644
--- a/src/gap_to_julia.jl
+++ b/src/gap_to_julia.jl
@@ -103,13 +103,13 @@ function gap_to_julia(::Type{BigInt}, x::GapObj)
     Globals.IsInt(x) || throw(ConversionError(x, BigInt))
     ## get size of GAP BigInt (in limbs), multiply
     ## by 64 to get bits
-    size_limbs = ccall(:GAP_SizeInt, Cint, (Any,), x)
+    size_limbs = @sync_noexcept ccall(:GAP_SizeInt, Cint, (Any,), x)
     size = abs(size_limbs * sizeof(UInt) * 8)
     ## allocate new GMP
     new_bigint = Base.GMP.MPZ.realloc2(size)
     new_bigint.size = size_limbs
     ## Get limb address ptr
-    addr = ccall(:GAP_AddrInt, Ptr{UInt}, (Any,), x)
+    addr = @sync_noexcept ccall(:GAP_AddrInt, Ptr{UInt}, (Any,), x)
     ## Copy limbs
     unsafe_copyto!(new_bigint.d, addr, abs(size_limbs))
     return new_bigint
diff --git a/src/julia_to_gap.jl b/src/julia_to_gap.jl
index 342c5494e..08f41fa65 100644
--- a/src/julia_to_gap.jl
+++ b/src/julia_to_gap.jl
@@ -57,7 +57,7 @@ function julia_to_gap(x::BigInt)
     if x in -1<<60:(1<<60-1)
         return Int64(x)
     end
-    return ccall(:MakeObjInt, GapObj, (Ptr{UInt64}, Cint), x.d, x.size)
+    return @sync_noexcept ccall(:MakeObjInt, GapObj, (Ptr{UInt64}, Cint), x.d, x.size)
 end
 
 ## Rationals
diff --git a/src/sync.jl b/src/sync.jl
new file mode 100644
index 000000000..be3a0a014
--- /dev/null
+++ b/src/sync.jl
@@ -0,0 +1,65 @@
+module Sync
+    global mutex = ReentrantLock()
+    global sync_level = repeat([ 0 ], Threads.nthreads())
+
+    @inline is_locked() = sync_level[Threads.threadid()] > 0
+
+    @inline function lock()
+        tid = Threads.threadid()
+        if sync_level[tid] == 0
+            Base.lock(mutex)
+        end
+        sync_level[tid] += 1
+    end
+
+    @inline function unlock()
+        tid = Threads.threadid()
+        sync_level[tid] -= 1
+        if sync_level[tid] == 0
+            Base.unlock(mutex)
+        end
+    end
+
+    @inline function check_lock()
+        assert(is_locked())
+    end
+end
+
+macro sync(expr)
+    if Threads.nthreads() > 1
+        quote
+            try
+                Sync.lock()
+                $(esc(expr))
+            finally
+                Sync.unlock()
+            end
+        end
+    else
+        :( $(esc(expr)) )
+    end
+end
+
+macro sync_noexcept(expr)
+    if Threads.nthreads() > 1
+        quote
+            Sync.lock()
+            local t = $(esc(expr))
+            Sync.unlock()
+            t
+        end
+    else
+        :( $(esc(expr)) )
+    end
+end
+
+macro check_sync(expr)
+    if Threads.nthreads() > 1
+        quote
+            Sync.check_lock()
+            $(esc(expr))
+        end
+    else
+        :( $(esc(expr)) )
+    end
+end

From 9af4881b5f836602c279f7487eb84b608becb269 Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Fri, 31 Jul 2020 10:47:40 +0200
Subject: [PATCH 02/10] Remove old C-based synchronization code.

With the Julia @sync macros now doing the actual synchronization work,
the C implementation is no longer needed and can be removed.
---
 pkg/JuliaInterface/Makefile.in          |  2 +-
 pkg/JuliaInterface/src/JuliaInterface.c | 29 -------------------------
 pkg/JuliaInterface/src/calls.c          |  7 ------
 pkg/JuliaInterface/src/convert.c        |  3 ---
 pkg/JuliaInterface/src/sync.c           | 26 ----------------------
 pkg/JuliaInterface/src/sync.h           | 20 -----------------
 6 files changed, 1 insertion(+), 86 deletions(-)
 delete mode 100644 pkg/JuliaInterface/src/sync.c
 delete mode 100644 pkg/JuliaInterface/src/sync.h

diff --git a/pkg/JuliaInterface/Makefile.in b/pkg/JuliaInterface/Makefile.in
index b7b58c726..834c91fc2 100644
--- a/pkg/JuliaInterface/Makefile.in
+++ b/pkg/JuliaInterface/Makefile.in
@@ -2,7 +2,7 @@
 # Makefile rules for the JuliaInterface package
 #
 KEXT_NAME = JuliaInterface
-KEXT_SOURCES = src/JuliaInterface.c src/calls.c src/convert.c src/sync.c
+KEXT_SOURCES = src/JuliaInterface.c src/calls.c src/convert.c
 
 KEXT_CFLAGS = 
 KEXT_LDFLAGS = 
diff --git a/pkg/JuliaInterface/src/JuliaInterface.c b/pkg/JuliaInterface/src/JuliaInterface.c
index 147ea5f8b..39cff20b0 100644
--- a/pkg/JuliaInterface/src/JuliaInterface.c
+++ b/pkg/JuliaInterface/src/JuliaInterface.c
@@ -6,7 +6,6 @@
 
 #include "calls.h"
 #include "convert.h"
-#include "sync.h"
 
 #include <src/julia_gc.h>
 
@@ -36,9 +35,7 @@ void handle_jl_exception(void)
     jl_value_t * string_object =
         jl_call1(JULIA_FUNC_take_inplace, JULIA_ERROR_IOBuffer);
     string_object = jl_call1(JULIA_FUNC_String_constructor, string_object);
-    BEGIN_GAP_SYNC();
     ErrorMayQuit(jl_string_data(string_object), 0, 0);
-    END_GAP_SYNC();
 }
 
 static jl_module_t * get_module(const char * name)
@@ -49,9 +46,7 @@ static jl_module_t * get_module(const char * name)
     jl_value_t * module_value = jl_eval_string(name);
     JULIAINTERFACE_EXCEPTION_HANDLER
     if (!jl_is_module(module_value)) {
-        BEGIN_GAP_SYNC();
         ErrorQuit("Not a module", 0, 0);
-        END_GAP_SYNC();
     }
     return (jl_module_t *)module_value;
 }
@@ -64,9 +59,7 @@ Obj Func_JULIAINTERFACE_INTERNAL_INIT(Obj self)
     JULIA_GAPFFE_type =
         (jl_datatype_t *)jl_get_global(gap_module, jl_symbol("FFE"));
     if (!JULIA_GAPFFE_type) {
-        BEGIN_GAP_SYNC();
         ErrorMayQuit("Could not locate the GAP.FFE datatype", 0, 0);
-        END_GAP_SYNC();
     }
     return NULL;
 }
@@ -98,7 +91,6 @@ int is_gapobj(jl_value_t * v)
 
 static Obj Func_NewJuliaCFunc(Obj self, Obj julia_function_ptr, Obj args)
 {
-    BEGIN_GAP_SYNC();
     if (!IS_JULIA_OBJ(julia_function_ptr)) {
         ErrorMayQuit("NewJuliaCFunc: <ptr> must be a Julia object", 0, 0);
     }
@@ -106,7 +98,6 @@ static Obj Func_NewJuliaCFunc(Obj self, Obj julia_function_ptr, Obj args)
 
     jl_value_t * func_ptr = GET_JULIA_OBJ(julia_function_ptr);
     void *       ptr = jl_unbox_voidpointer(func_ptr);
-    END_GAP_SYNC();
     return NewJuliaCFunc(ptr, args);
 }
 
@@ -159,7 +150,6 @@ Obj NewJuliaObj(jl_value_t * v)
 jl_function_t * get_function_from_obj_or_string(Obj func)
 {
     jl_function_t * f = NULL;
-    BEGIN_GAP_SYNC();
     if (IS_JULIA_OBJ(func)) {
         f = (jl_function_t *)GET_JULIA_OBJ(func);
     }
@@ -173,7 +163,6 @@ jl_function_t * get_function_from_obj_or_string(Obj func)
     }
     else
         ErrorMayQuit("argument is not a julia object or string", 0, 0);
-    END_GAP_SYNC();
     return f;
 }
 
@@ -195,7 +184,6 @@ static Obj Func_JuliaFunction(Obj self, Obj func)
  */
 static Obj Func_JuliaFunctionByModule(Obj self, Obj funcName, Obj moduleName)
 {
-    BEGIN_GAP_SYNC();
     RequireStringRep("_JuliaFunctionByModule", funcName);
     RequireStringRep("_JuliaFunctionByModule", moduleName);
 
@@ -205,7 +193,6 @@ static Obj Func_JuliaFunctionByModule(Obj self, Obj funcName, Obj moduleName)
     jl_function_t * f = jl_get_function(m, CONST_CSTR_STRING(funcName));
     if (f == 0)
         ErrorMayQuit("Function is not defined in julia", 0, 0);
-    END_GAP_SYNC();
     return NewJuliaFunc(f);
 }
 
@@ -218,14 +205,12 @@ static Obj FuncIS_JULIA_FUNC(Obj self, Obj obj)
 // Executes the string <string> in the current julia session.
 static Obj FuncJuliaEvalString(Obj self, Obj string)
 {
-    BEGIN_GAP_SYNC();
     RequireStringRep("JuliaEvalString", string);
 
     // It suffices to use JULIAINTERFACE_EXCEPTION_HANDLER here, as
     // jl_eval_string is part of the jlapi, so don't have to be wrapped in
     // JL_TRY/JL_CATCH.
     jl_value_t * result = jl_eval_string(CONST_CSTR_STRING(string));
-    END_GAP_SYNC();
     JULIAINTERFACE_EXCEPTION_HANDLER
     return gap_julia(result);
 }
@@ -234,13 +219,11 @@ static Obj FuncJuliaEvalString(Obj self, Obj string)
 // :<name>.
 static Obj FuncJuliaSymbol(Obj self, Obj name)
 {
-    BEGIN_GAP_SYNC();
     RequireStringRep("JuliaSymbol", name);
 
     // jl_symbol never throws an exception and always returns a valid
     // result, so no need for extra checks.
     jl_sym_t * julia_symbol = jl_symbol(CONST_CSTR_STRING(name));
-    END_GAP_SYNC();
     return NewJuliaObj((jl_value_t *)julia_symbol);
 }
 
@@ -248,12 +231,10 @@ static Obj FuncJuliaSymbol(Obj self, Obj name)
 // This function is for debugging purposes.
 static Obj FuncJuliaSetVal(Obj self, Obj name, Obj val)
 {
-    BEGIN_GAP_SYNC();
     RequireStringRep("JuliaSetVal", name);
 
     jl_value_t * julia_obj = julia_gap(val);
     jl_sym_t *   julia_symbol = jl_symbol(CONST_CSTR_STRING(name));
-    END_GAP_SYNC();
     jl_set_global(jl_main_module, julia_symbol, julia_obj);
     return 0;
 }
@@ -262,11 +243,9 @@ static Obj FuncJuliaSetVal(Obj self, Obj name, Obj val)
 // currently bound to the julia identifier <name>.
 static Obj Func_JuliaGetGlobalVariable(Obj self, Obj name)
 {
-    BEGIN_GAP_SYNC();
     RequireStringRep("_JuliaGetGlobalVariable", name);
 
     jl_sym_t * symbol = jl_symbol(CONST_CSTR_STRING(name));
-    END_GAP_SYNC();
     if (!jl_boundp(jl_main_module, symbol)) {
         return Fail;
     }
@@ -278,7 +257,6 @@ static Obj Func_JuliaGetGlobalVariable(Obj self, Obj name)
 // currently bound to the julia identifier <moduleName>.<name>.
 static Obj Func_JuliaGetGlobalVariableByModule(Obj self, Obj name, Obj module)
 {
-    BEGIN_GAP_SYNC();
     RequireStringRep("_JuliaGetGlobalVariableByModule", name);
 
     jl_module_t * m = 0;
@@ -296,7 +274,6 @@ static Obj Func_JuliaGetGlobalVariableByModule(Obj self, Obj name, Obj module)
                      0, 0);
     }
     jl_sym_t * symbol = jl_symbol(CONST_CSTR_STRING(name));
-    END_GAP_SYNC();
     if (!jl_boundp(m, symbol)) {
         return Fail;
     }
@@ -309,7 +286,6 @@ static Obj Func_JuliaGetGlobalVariableByModule(Obj self, Obj name, Obj module)
 // <super_object> must be a julia object GAP object, and <name> a string.
 static Obj FuncJuliaGetFieldOfObject(Obj self, Obj super_obj, Obj field_name)
 {
-    BEGIN_GAP_SYNC();
     if (!IS_JULIA_OBJ(super_obj)) {
         ErrorMayQuit(
             "JuliaGetFieldOfObject: <super_obj> must be a Julia object", 0,
@@ -324,7 +300,6 @@ static Obj FuncJuliaGetFieldOfObject(Obj self, Obj super_obj, Obj field_name)
     // JL_TRY/JL_CATCH.
     jl_value_t * field_value =
         jl_get_field(extracted_superobj, CONST_CSTR_STRING(field_name));
-    END_GAP_SYNC();
     JULIAINTERFACE_EXCEPTION_HANDLER
     return gap_julia(field_value);
 }
@@ -335,7 +310,6 @@ static Obj FuncSTREAM_CALL(Obj self, Obj stream, Obj func, Obj obj)
 {
     syJmp_buf readJmpError;
 
-    BEGIN_GAP_SYNC();
 
     if (CALL_1ARGS(IsOutputStream, stream) != True) {
         ErrorQuit("STREAM_CALL: <outstream> must be an output stream", 0, 0);
@@ -351,7 +325,6 @@ static Obj FuncSTREAM_CALL(Obj self, Obj stream, Obj func, Obj obj)
     if (!CloseOutput()) {
         ErrorQuit("STREAM_CALL: cannot close output", 0, 0);
     }
-    END_GAP_SYNC();
 
     return 0;
 }
@@ -396,8 +369,6 @@ static Int InitKernel(StructInitInfo * module)
         jl_options.can_inline = 0;
     }
 
-    InitGapSync();
-
     // init filters and functions
     InitHdlrFuncsFromTable(GVarFuncs);
 
diff --git a/pkg/JuliaInterface/src/calls.c b/pkg/JuliaInterface/src/calls.c
index 1495c3d01..5dcb25a70 100644
--- a/pkg/JuliaInterface/src/calls.c
+++ b/pkg/JuliaInterface/src/calls.c
@@ -1,6 +1,5 @@
 #include "calls.h"
 #include "convert.h"
-#include "sync.h"
 #include "JuliaInterface.h"
 
 
@@ -23,7 +22,6 @@ jl_value_t * call_gap_func(Obj func, jl_value_t * args)
 
     size_t len = jl_nfields(args);
     Obj    return_value = NULL;
-    BEGIN_GAP_SYNC();
     if (IS_FUNC(func) && len <= 6) {
         switch (len) {
         case 0:
@@ -73,7 +71,6 @@ jl_value_t * call_gap_func(Obj func, jl_value_t * args)
         }
         return_value = CallFuncList(func, arg_list);
     }
-    END_GAP_SYNC();
     if (return_value == NULL) {
         return jl_nothing;
     }
@@ -191,7 +188,6 @@ static Obj DoCallJuliaFuncXArg(Obj func, Obj args)
 //
 Obj NewJuliaFunc(jl_function_t * function)
 {
-    BEGIN_GAP_SYNC();
     Obj name = MakeImmString(jl_symbol_name(jl_gf_name(function)));
     Obj func = NewFunctionT(T_FUNCTION, sizeof(JuliaFuncBag), name, -1,
                             ArgStringToList("arg"), 0);
@@ -217,7 +213,6 @@ Obj NewJuliaFunc(jl_function_t * function)
     SET_BODY_FUNC(func, body);
     CHANGED_BAG(body);
     CHANGED_BAG(func);
-    END_GAP_SYNC();
 
     return func;
 }
@@ -310,7 +305,6 @@ Obj NewJuliaCFunc(void * function, Obj args)
 {
     ObjFunc handler;
 
-    BEGIN_GAP_SYNC();
     switch (LEN_PLIST(args)) {
     case 0:
         handler = DoCallJuliaCFunc0Arg;
@@ -345,7 +339,6 @@ Obj NewJuliaCFunc(void * function, Obj args)
     // store it as a valid julia obj (i.e., void ptr).
     ((JuliaFuncBag *)ADDR_OBJ(func))->juliaFunc =
         NewJuliaObj(jl_box_voidpointer(function));
-    END_GAP_SYNC();
 
     return func;
 }
diff --git a/pkg/JuliaInterface/src/convert.c b/pkg/JuliaInterface/src/convert.c
index db42df125..695792838 100644
--- a/pkg/JuliaInterface/src/convert.c
+++ b/pkg/JuliaInterface/src/convert.c
@@ -1,7 +1,6 @@
 #include "convert.h"
 
 #include "calls.h"
-#include "sync.h"
 #include "JuliaInterface.h"
 
 // This function is used by GAP.jl
@@ -29,7 +28,6 @@ jl_value_t * julia_gap(Obj obj)
         return jl_false;
     }
     jl_value_t * result = (jl_value_t *)obj;
-    BEGIN_GAP_SYNC();
     if (TNUM_OBJ(obj) >= FIRST_EXTERNAL_TNUM &&
         CALL_1ARGS(JULIAINTERFACE_IsJuliaWrapper, obj) == True) {
         obj = CALL_1ARGS(JULIAINTERFACE_JuliaPointer, obj);
@@ -49,7 +47,6 @@ jl_value_t * julia_gap(Obj obj)
                          (Int)TNAM_OBJ(obj), 0);
         }
     }
-    END_GAP_SYNC();
     return result;
 }
 
diff --git a/pkg/JuliaInterface/src/sync.c b/pkg/JuliaInterface/src/sync.c
deleted file mode 100644
index e70884fd9..000000000
--- a/pkg/JuliaInterface/src/sync.c
+++ /dev/null
@@ -1,26 +0,0 @@
-#include "sync.h"
-#include <pthread.h>
-#include <stdlib.h>
-
-#ifndef HPCGAP
-static pthread_mutex_t GapLock;
-
-void InitGapSync(void)
-{
-    pthread_mutexattr_t attr;
-    pthread_mutexattr_init(&attr);
-    pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
-    pthread_mutex_init(&GapLock, &attr);
-    pthread_mutexattr_destroy(&attr);
-}
-
-void BeginGapSync(void)
-{
-    pthread_mutex_lock(&GapLock);
-}
-
-void EndGapSync(void)
-{
-    pthread_mutex_unlock(&GapLock);
-}
-#endif
diff --git a/pkg/JuliaInterface/src/sync.h b/pkg/JuliaInterface/src/sync.h
deleted file mode 100644
index 82d9eba26..000000000
--- a/pkg/JuliaInterface/src/sync.h
+++ /dev/null
@@ -1,20 +0,0 @@
-#ifndef JULIAINTERFACE_SYNC_H
-#define JULIAINTERFACE_SYNC_H
-
-// #define THREADSAFE_GAP_JL 1
-
-#include <src/compiled.h>
-
-#if defined(HPCGAP) || !defined(THREADSAFE_GAP_JL)
-#define BEGIN_GAP_SYNC() ((void)0)
-#define END_GAP_SYNC() ((void)0)
-#else
-void BeginGapSync(void);
-void EndGapSync(void);
-#define BEGIN_GAP_SYNC() BeginGapSync()
-#define END_GAP_SYNC() EndGapSync()
-#endif
-
-void InitGapSync(void);
-
-#endif

From 7b639b10bf8983b922a691513c8fc080e9efd3c0 Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Fri, 31 Jul 2020 10:55:31 +0200
Subject: [PATCH 03/10] Make global variables const instead.

---
 src/sync.jl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/sync.jl b/src/sync.jl
index be3a0a014..d353efcd7 100644
--- a/src/sync.jl
+++ b/src/sync.jl
@@ -1,6 +1,6 @@
 module Sync
-    global mutex = ReentrantLock()
-    global sync_level = repeat([ 0 ], Threads.nthreads())
+    const mutex = ReentrantLock()
+    const sync_level = repeat([ 0 ], Threads.nthreads())
 
     @inline is_locked() = sync_level[Threads.threadid()] > 0
 

From 48ed5e3eefb3ddc41a6c24e6719d5f756912045d Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Sun, 2 Aug 2020 01:36:09 +0200
Subject: [PATCH 04/10] Wrap entire initialize() function in @sync.

---
 src/GAP.jl | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/GAP.jl b/src/GAP.jl
index 88dc79089..e49b3e337 100644
--- a/src/GAP.jl
+++ b/src/GAP.jl
@@ -183,7 +183,7 @@ function initialize(argv::Array{String,1})
     # detect by checking for the value of __JULIAINTERNAL_LOADED_FROM_JULIA).
     append!(argv, ["-c", """BindGlobal("__JULIAINTERNAL_LOADED_FROM_JULIA", true );"""])
 
-    @sync ccall(
+    ccall(
         Libdl.dlsym(libgap_handle, :GAP_Initialize),
         Cvoid,
         (Int32, Ptr{Ptr{UInt8}}, Ptr{Cvoid}, Ptr{Cvoid}, Cuint),
@@ -200,7 +200,7 @@ function initialize(argv::Array{String,1})
     # (perhaps this could be a return value for GAP_Initialize?), so instead
     # we check for the presence of a global variable which we ensure is
     # declared near the end of init.g via a `-c` command line argument to GAP.
-    val = @sync ccall(
+    val = ccall(
         Libdl.dlsym(libgap_handle, :GAP_ValueGlobalVariable),
         Ptr{Cvoid},
         (Ptr{Cuchar},),
@@ -215,13 +215,13 @@ function initialize(argv::Array{String,1})
         # FORCE_QUIT_GAP with no arguments, which just calls the `exit`
         # function of the  C standard library with the appropriate exit code.
         # But as mentioned, just before that, it runs `jl_atexit_hook`.
-        FORCE_QUIT_GAP = @sync ccall(
+        FORCE_QUIT_GAP = ccall(
             Libdl.dlsym(libgap_handle, :GAP_ValueGlobalVariable),
             Ptr{Cvoid},
             (Ptr{Cuchar},),
             "FORCE_QUIT_GAP",
         )
-        @sync ccall(
+        ccall(
             Libdl.dlsym(libgap_handle, :GAP_CallFuncArray),
             Ptr{Cvoid},
             (Ptr{Cvoid}, Culonglong, Ptr{Cvoid}),
@@ -265,7 +265,7 @@ function initialize(argv::Array{String,1})
     @assert T_HVARS == Base.invokelatest(ValueGlobalVariable,:T_HVARS)
 
     # load JuliaInterface
-    loadpackage_return = @sync ccall(
+    loadpackage_return = ccall(
         Libdl.dlsym(libgap_handle, :GAP_EvalString),
         Ptr{Cvoid},
         (Ptr{UInt8},),
@@ -277,7 +277,7 @@ function initialize(argv::Array{String,1})
 
     # If we are in "stand-alone mode", stop here
     if isdefined(Main, :__GAP_ARGS__)
-        @sync ccall(Libdl.dlsym(libgap_handle, :SyInstallAnswerIntr), Cvoid, ())
+        ccall(Libdl.dlsym(libgap_handle, :SyInstallAnswerIntr), Cvoid, ())
         return
     end
 
@@ -314,7 +314,7 @@ function run_it()
         # Do not show the main GAP banner by default.
         push!(cmdline_options, "-b")
     end
-    initialize(cmdline_options)
+    @sync initialize(cmdline_options)
 
     if !show_banner
         # Leave it to GAP's `LoadPackage` whether package banners are shown.

From 8dfd8510c652f1400c85a24479d52664d2a842b9 Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Wed, 5 Aug 2020 16:34:46 +0200
Subject: [PATCH 05/10] Make locking logic task-based instead of thread-based.

Locking relied on tasks not being migrated between threads; we
now associate the lock status with the locking task.
---
 src/sync.jl | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/sync.jl b/src/sync.jl
index d353efcd7..03dd3b3fb 100644
--- a/src/sync.jl
+++ b/src/sync.jl
@@ -1,27 +1,35 @@
 module Sync
+    mutable struct LockStatus
+        nested :: Int
+	owner :: Union{Task, Nothing}
+    end
+
     const mutex = ReentrantLock()
-    const sync_level = repeat([ 0 ], Threads.nthreads())
+    const lock_status = LockStatus(0, nothing)
 
-    @inline is_locked() = sync_level[Threads.threadid()] > 0
+    @inline is_locked() = lock_status.owner == Base.current_task()
 
     @inline function lock()
-        tid = Threads.threadid()
-        if sync_level[tid] == 0
-            Base.lock(mutex)
-        end
-        sync_level[tid] += 1
+        if is_locked()
+	  lock_status.nested += 1
+	else
+	  Base.lock(mutex)
+	  lock_status.nested = 1
+	  lock_status.owner = Base.current_task()
+	end
     end
 
     @inline function unlock()
-        tid = Threads.threadid()
-        sync_level[tid] -= 1
-        if sync_level[tid] == 0
-            Base.unlock(mutex)
-        end
+        @assert is_locked()
+	lock_status.nested -= 1
+	if lock_status.nested == 0
+	  lock_status.owner = nothing
+	  Base.unlock(mutex)
+	end
     end
 
     @inline function check_lock()
-        assert(is_locked())
+        @assert is_locked()
     end
 end
 

From b4125baad88849519de81d4bda02655b92b297f5 Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Fri, 7 Aug 2020 06:41:54 +0200
Subject: [PATCH 06/10] Fix precompilation issues arising from use of @sync
 macros.

Macros and precompilation do not necessarily mix in Julia. If
macro evaluation is dependent on program state and the program
state changes later, the macro does not get reevaluated.

However, Julia does track function dependencies, so we can force
recompilation by altering any underlying regular functions.
---
 src/sync.jl | 115 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 67 insertions(+), 48 deletions(-)

diff --git a/src/sync.jl b/src/sync.jl
index 03dd3b3fb..1c84c0f2d 100644
--- a/src/sync.jl
+++ b/src/sync.jl
@@ -1,73 +1,92 @@
 module Sync
-    mutable struct LockStatus
-        nested :: Int
-	owner :: Union{Task, Nothing}
-    end
 
     const mutex = ReentrantLock()
-    const lock_status = LockStatus(0, nothing)
-
-    @inline is_locked() = lock_status.owner == Base.current_task()
 
     @inline function lock()
-        if is_locked()
-	  lock_status.nested += 1
-	else
-	  Base.lock(mutex)
-	  lock_status.nested = 1
-	  lock_status.owner = Base.current_task()
-	end
+        Base.lock(mutex)
     end
 
     @inline function unlock()
-        @assert is_locked()
-	lock_status.nested -= 1
-	if lock_status.nested == 0
-	  lock_status.owner = nothing
-	  Base.unlock(mutex)
-	end
+        Base.unlock(mutex)
     end
 
     @inline function check_lock()
-        @assert is_locked()
+        @assert mutex.locked_by === Base.current_task()
     end
-end
 
-macro sync(expr)
-    if Threads.nthreads() > 1
-        quote
+    # To switch between multi-threaded and single-threaded mode, we
+    # define functions that install the appropriate handlers for sync()
+    # etc.
+    #
+    # This is necessary because otherwise precompilation would fix
+    # the mode at whatever state it was during precompilation. Thus,
+    # initially loading GAP.jl in single-threaded mode would also keep
+    # synchronization off in multi-threaded mode.
+    #
+    # However, Julia tracks function dependencies. If a function changes
+    # upon which another depends, both are being recompiled. Thus,
+    # by installing the proper version during __init__(), we force
+    # selective recompilation of the affected functions as needed.
+
+    function enable_sync()
+        Sync.eval(:(@inline function sync(f::Function)
             try
-                Sync.lock()
-                $(esc(expr))
+                lock()
+                f()
             finally
-                Sync.unlock()
+                unlock()
             end
+        end))
+        Sync.eval(:(@inline function sync_noexcept(f::Function)
+            lock()
+            t = f()
+            unlock()
+            t
+        end))
+        Sync.eval(:(@inline function check_sync(f::Function)
+            check_lock()
+            f()
+        end))
+    end
+
+    function disable_sync()
+        Sync.eval(:(@inline function sync(f::Function)
+            f()
+        end))
+        Sync.eval(:(@inline function sync_noexcept(f::Function)
+            f()
+        end))
+        Sync.eval(:(@inline function check_sync(f::Function)
+            f()
+        end))
+    end
+
+    # Initialization is tricky. __init__() can be called from
+    # within the first sync() call if the module has already
+    # been precompiled. Thus, we default to enable_sync() for
+    # precompilation and then set the actual sync mode during
+    # __init__(). Dropping back from sync enabled to being
+    # disabled is safe, but not the other way round.
+
+    enable_sync()
+
+    function __init__()
+        if Threads.nthreads() > 1
+            enable_sync()
+        else
+            disable_sync()
         end
-    else
-        :( $(esc(expr)) )
     end
 end
 
+macro sync(expr)
+    :( Sync.sync(()->$(esc(expr))) )
+end
+
 macro sync_noexcept(expr)
-    if Threads.nthreads() > 1
-        quote
-            Sync.lock()
-            local t = $(esc(expr))
-            Sync.unlock()
-            t
-        end
-    else
-        :( $(esc(expr)) )
-    end
+    :( Sync.sync_noexcept(()->$(esc(expr))) )
 end
 
 macro check_sync(expr)
-    if Threads.nthreads() > 1
-        quote
-            Sync.check_lock()
-            $(esc(expr))
-        end
-    else
-        :( $(esc(expr)) )
-    end
+    :( Sync.check_sync(()->$(esc(expr))) )
 end

From 54b99fc41ee9065edfefdbeabc093f4c9fb94e4c Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Fri, 7 Aug 2020 17:03:51 +0200
Subject: [PATCH 07/10] Add tests for thread safety.

---
 test/runtests.jl |  1 +
 test/sync.jl     | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 test/sync.jl

diff --git a/test/runtests.jl b/test/runtests.jl
index e90c1df8d..49748b06a 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -10,4 +10,5 @@ include("convert.jl")
 include("macros.jl")
 include("packages.jl")
 include("help.jl")
+include("sync.jl")
 # include("testmanual.jl")  # leave this out until doctestfilters can be used here
diff --git a/test/sync.jl b/test/sync.jl
new file mode 100644
index 000000000..b52659efa
--- /dev/null
+++ b/test/sync.jl
@@ -0,0 +1,36 @@
+function run_timed(script :: String, timeout :: Int)
+  cmd = Cmd(Base.julia_cmd();
+    env = merge(ENV, Dict("JULIA_NUM_THREADS" => "2")))
+  push!(cmd.exec, "-e", script)
+  process = open(cmd)
+  task = @async begin
+    wait(process)
+  end
+  for t in 1:(timeout*10)
+    sleep(0.1)
+    if task.state == :done
+      return process.exitcode == 0
+    end
+  end
+  kill(process, 9)
+  return false
+end
+
+@testset "sync" begin
+  @test run_timed("""
+  using GAP
+  Threads.@threads for i = 1:20000
+    GAP.Globals.SymmetricGroup(7)
+  end
+  """, 10)
+  @test run_timed("""
+  using GAP
+  function test()
+    perm = @gap "()"
+    Threads.@threads for i = 1:2000
+      perm *= GAP.evalstr("(\$(i), \$(i+1))")
+    end
+  end
+  test()
+  """, 10)
+end

From f6b28f75e33b318e576f8cca1e685ff0eab56537 Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Fri, 14 Aug 2020 13:19:49 +0200
Subject: [PATCH 08/10] Allow pinning GAP to a specific thread.

We allow for two alternative synchronization modes. One uses a
standard mutex, the other pins GAP to a specific thread (the main
thread by default). When pinned to a thread, GAP can only be
called from that thread. Calling it from another thread will
result in an error.

While pinning is more restrictive than locking, it is also
considerably faster, incurring very little overhead.
---
 src/sync.jl | 71 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 55 insertions(+), 16 deletions(-)

diff --git a/src/sync.jl b/src/sync.jl
index 1c84c0f2d..1f8fb2279 100644
--- a/src/sync.jl
+++ b/src/sync.jl
@@ -1,17 +1,24 @@
 module Sync
 
-    const mutex = ReentrantLock()
+    @enum SyncMode mutex pin disabled
+
+    const gap_lock = ReentrantLock()
+    const pinned_thread = Ref{Int}(1)
 
     @inline function lock()
-        Base.lock(mutex)
+        Base.lock(gap_lock)
     end
 
     @inline function unlock()
-        Base.unlock(mutex)
+        Base.unlock(gap_lock)
     end
 
     @inline function check_lock()
-        @assert mutex.locked_by === Base.current_task()
+        @assert gap_lock.locked_by === Base.current_task()
+    end
+
+    @inline function check_pinned()
+        @assert Threads.threadid() == pinned_thread[]
     end
 
     # To switch between multi-threaded and single-threaded mode, we
@@ -28,7 +35,7 @@ module Sync
     # by installing the proper version during __init__(), we force
     # selective recompilation of the affected functions as needed.
 
-    function enable_sync()
+    function _mode_mutex()
         Sync.eval(:(@inline function sync(f::Function)
             try
                 lock()
@@ -49,32 +56,64 @@ module Sync
         end))
     end
 
-    function disable_sync()
+    function _mode_pinned()
         Sync.eval(:(@inline function sync(f::Function)
+            check_pinned()
             f()
         end))
         Sync.eval(:(@inline function sync_noexcept(f::Function)
+            check_pinned()
             f()
         end))
         Sync.eval(:(@inline function check_sync(f::Function)
+            check_pinned()
+            f()
+        end))
+    end
+
+    function _mode_nosync()
+        Sync.eval(:(@inline function sync(f::Function)
             f()
         end))
+        Sync.eval(:(@inline function sync_noexcept(f::Function)
+            f()
+        end))
+        Sync.eval(:(@inline function check_sync(f::Function)
+            f()
+        end))
+    end
+
+    function switch(mode::SyncMode)
+        stack = stacktrace()
+        file = stack[1].file
+        for frame in stack[2:end]
+            if frame.func in [ :sync, :sync_noexcept ] && frame.file == file
+                @error "trying to change sync mode within critical region"
+            end
+        end
+        if mode == pin
+            _mode_pinned()
+        elseif mode == mutex
+            _mode_mutex()
+        else # mode == disabled
+            _mode_nosync()
+        end
     end
 
-    # Initialization is tricky. __init__() can be called from
-    # within the first sync() call if the module has already
-    # been precompiled. Thus, we default to enable_sync() for
-    # precompilation and then set the actual sync mode during
-    # __init__(). Dropping back from sync enabled to being
-    # disabled is safe, but not the other way round.
+    # Initialization is tricky. __init__() can be called from within the
+    # first sync() call if the module has already been precompiled. Thus,
+    # we default to enabling synchronization during precompilation and
+    # then set the actual sync mode during __init__(). Dropping back from
+    # synchronization being enabled to being disabled is safe, but not the
+    # other way round.
 
-    enable_sync()
+    _mode_pinned()
 
     function __init__()
-        if Threads.nthreads() > 1
-            enable_sync()
+        if Threads.nthreads() == 1
+            _mode_nosync()
         else
-            disable_sync()
+            _mode_pinned()
         end
     end
 end

From 6707eebbbc9f9f90ebdceeac075bf1c244c3abcc Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Tue, 8 Sep 2020 12:00:46 +0200
Subject: [PATCH 09/10] Rename @sync -> @lock.

This is to avoid conflicts with Base.@sync.
---
 src/GAP.jl          | 10 ++++----
 src/ccalls.jl       | 42 +++++++++++++++---------------
 src/gap_to_julia.jl |  4 +--
 src/julia_to_gap.jl |  2 +-
 src/sync.jl         | 62 ++++++++++++++++++++++-----------------------
 5 files changed, 60 insertions(+), 60 deletions(-)

diff --git a/src/GAP.jl b/src/GAP.jl
index e49b3e337..efc5d1bb7 100644
--- a/src/GAP.jl
+++ b/src/GAP.jl
@@ -291,7 +291,7 @@ function initialize(argv::Array{String,1})
 end
 
 function finalize()
-    @sync ccall((:GAP_finalize, "libgap"), Cvoid, ())
+    @lock ccall((:GAP_finalize, "libgap"), Cvoid, ())
 end
 
 function run_it()
@@ -314,7 +314,7 @@ function run_it()
         # Do not show the main GAP banner by default.
         push!(cmdline_options, "-b")
     end
-    @sync initialize(cmdline_options)
+    @lock initialize(cmdline_options)
 
     if !show_banner
         # Leave it to GAP's `LoadPackage` whether package banners are shown.
@@ -384,10 +384,10 @@ function prompt()
     # save the current SIGINT handler
     # HACK: the hardcoded value for SIG_DFL is not portable, revise this
     # install GAP's SIGINT handler
-    old_sigint = @sync ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, C_NULL)
+    old_sigint = @lock ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, C_NULL)
 
     # install GAP's SIGINT handler
-    @sync ccall(:SyInstallAnswerIntr, Cvoid, ())
+    @lock ccall(:SyInstallAnswerIntr, Cvoid, ())
 
     # restore GAP's error output
     disable_error_handler = true
@@ -405,7 +405,7 @@ function prompt()
     Globals.BreakOnError = false
 
     # restore signal handler
-    @sync ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, old_sigint)
+    @lock ccall(:signal, Ptr{Cvoid}, (Cint, Ptr{Cvoid}), Base.SIGINT, old_sigint)
 
     # restore GAP error handler
     disable_error_handler = false
diff --git a/src/ccalls.jl b/src/ccalls.jl
index f026c2c7c..77f0c0855 100644
--- a/src/ccalls.jl
+++ b/src/ccalls.jl
@@ -13,13 +13,13 @@ function _GAP_TO_JULIA(ptr::Ptr{Cvoid})
     elseif as_int & 2 == 2
         return reinterpret(FFE, ptr)
     end
-    return @sync ccall(:julia_gap, Any, (Ptr{Cvoid},), ptr)
+    return @lock ccall(:julia_gap, Any, (Ptr{Cvoid},), ptr)
 end
 
 #
 # low-level Julia -> GAP conversion
 #
-_JULIA_TO_GAP(val::Any) = @sync ccall(:gap_julia, Ptr{Cvoid}, (Any,), val)
+_JULIA_TO_GAP(val::Any) = @lock ccall(:gap_julia, Ptr{Cvoid}, (Any,), val)
 #_JULIA_TO_GAP(x::Bool) = x ? gap_true : gap_false
 _JULIA_TO_GAP(x::FFE) = reinterpret(Ptr{Cvoid}, x)
 _JULIA_TO_GAP(x::GapObj) = pointer_from_objref(x)
@@ -28,12 +28,12 @@ function _JULIA_TO_GAP(x::Int)
     if x in -1<<60:(1<<60-1)
         return Ptr{Cvoid}(x << 2 | 1)
     end
-    return @sync_noexcept ccall(:ObjInt_Int, Ptr{Cvoid}, (Int,), x)
+    return @lock_noexcept ccall(:ObjInt_Int, Ptr{Cvoid}, (Int,), x)
 end
 
 
 function evalstr_ex(cmd::String)
-    res = @sync ccall(:GAP_EvalString, GapObj, (Ptr{UInt8},), cmd)
+    res = @lock ccall(:GAP_EvalString, GapObj, (Ptr{UInt8},), cmd)
     return res
 end
 
@@ -73,7 +73,7 @@ end
 # Retrieve the value of a global GAP variable given its name. This function
 # returns a raw Ptr value, and should only be called by plumbing code.
 function _ValueGlobalVariable(name::Union{AbstractString,Symbol})
-    return @sync_noexcept ccall(:GAP_ValueGlobalVariable, Ptr{Cvoid},
+    return @lock_noexcept ccall(:GAP_ValueGlobalVariable, Ptr{Cvoid},
         (Ptr{UInt8},), name)
 end
 
@@ -84,20 +84,20 @@ end
 
 # Test whether the global GAP variable with the given name can be assigned to.
 function CanAssignGlobalVariable(name::Union{AbstractString,Symbol})
-    @sync_noexcept ccall(:GAP_CanAssignGlobalVariable, Bool, (Ptr{UInt8},),
+    @lock_noexcept ccall(:GAP_CanAssignGlobalVariable, Bool, (Ptr{UInt8},),
         name)
 end
 
 # Assign a value to the global GAP variable with the given name. This function
 # assigns a raw Ptr value, and should only be called by plumbing code.
 function _AssignGlobalVariable(name::Union{AbstractString,Symbol}, value::Ptr{Cvoid})
-    @sync_noexcept ccall(:GAP_AssignGlobalVariable, Cvoid, (Ptr{UInt8},
+    @lock_noexcept ccall(:GAP_AssignGlobalVariable, Cvoid, (Ptr{UInt8},
         Ptr{Cvoid}), name, value)
 end
 
 # Assign a value to the global GAP variable with the given name.
 function AssignGlobalVariable(name::Union{AbstractString,Symbol}, value::Any)
-    @sync begin
+    @lock begin
       if !CanAssignGlobalVariable(name)
           error("cannot assing to $name in GAP")
       end
@@ -107,34 +107,34 @@ function AssignGlobalVariable(name::Union{AbstractString,Symbol}, value::Any)
 end
 
 MakeString(val::String) =
-    @sync_noexcept ccall(:GAP_MakeString, GapObj, (Ptr{UInt8},), val)
+    @lock_noexcept ccall(:GAP_MakeString, GapObj, (Ptr{UInt8},), val)
 
 function CSTR_STRING(val::GapObj)
-    char_ptr = @sync_noexcept ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
+    char_ptr = @lock_noexcept ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
     return deepcopy(unsafe_string(char_ptr))::String
 end
 
 function CSTR_STRING_AS_ARRAY(val::GapObj)::Array{UInt8,1}
-    string_len = @sync_noexcept Int64(ccall(:GAP_LenString, Cuint, (Any,), val))
-    char_ptr = @sync_noexcept ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
+    string_len = @lock_noexcept Int64(ccall(:GAP_LenString, Cuint, (Any,), val))
+    char_ptr = @lock_noexcept ccall(:GAP_CSTR_STRING, Ptr{UInt8}, (Any,), val)
     return deepcopy(unsafe_wrap(Array{UInt8,1}, char_ptr, string_len))
 end
 
 
 NewPlist(capacity::Int64) =
-    @sync_noexcept ccall(:GAP_NewPlist, GapObj, (Int64,), capacity)
+    @lock_noexcept ccall(:GAP_NewPlist, GapObj, (Int64,), capacity)
 NewPrecord(capacity::Int64) = ccall(:GAP_NewPrecord, GapObj, (Int64,), capacity)
 NEW_MACFLOAT(x::Float64) =
-    @sync_noexcept ccall(:NEW_MACFLOAT, GapObj, (Cdouble,), x)
+    @lock_noexcept ccall(:NEW_MACFLOAT, GapObj, (Cdouble,), x)
 ValueMacFloat(x::GapObj) =
-    @sync_noexcept ccall(:GAP_ValueMacFloat, Cdouble, (Any,), x)
+    @lock_noexcept ccall(:GAP_ValueMacFloat, Cdouble, (Any,), x)
 CharWithValue(x::Cuchar) =
-    @sync_noexcept ccall(:GAP_CharWithValue, GapObj, (Cuchar,), x)
+    @lock_noexcept ccall(:GAP_CharWithValue, GapObj, (Cuchar,), x)
 NewJuliaFunc(x::Function) =
-    @sync_noexcept ccall(:NewJuliaFunc, GapObj, (Any,), x)
+    @lock_noexcept ccall(:NewJuliaFunc, GapObj, (Any,), x)
 
 function ElmList(x::GapObj, position)
-    o = @sync ccall(:GAP_ElmList, Ptr{Cvoid}, (Any, Culong), x, Culong(position))
+    o = @lock ccall(:GAP_ElmList, Ptr{Cvoid}, (Any, Culong), x, Culong(position))
     return _GAP_TO_JULIA(o)
 end
 
@@ -180,7 +180,7 @@ function call_gap_func(func::GapObj, args...; kwargs...)
         options = true
     end
     try
-        @sync begin
+        @lock begin
             if TNUM_OBJ(func) == T_FUNCTION && length(args) <= 6
                 result = _call_gap_func(func, args...)
             else
@@ -355,7 +355,7 @@ Main
 const Globals = GlobalsType()
 
 function getproperty(::GlobalsType, name::Symbol)
-    @sync begin
+    @lock begin
         v = _ValueGlobalVariable(name)
         if v === C_NULL
             error("GAP variable $name not bound")
@@ -369,7 +369,7 @@ function hasproperty(::GlobalsType, name::Symbol)
 end
 
 function setproperty!(::GlobalsType, name::Symbol, val::Any)
-    @sync begin
+    @lock begin
         if !CanAssignGlobalVariable(name)
             error("cannot assing to $name in GAP")
         end
diff --git a/src/gap_to_julia.jl b/src/gap_to_julia.jl
index 73a4f2cd4..782d44654 100644
--- a/src/gap_to_julia.jl
+++ b/src/gap_to_julia.jl
@@ -103,13 +103,13 @@ function gap_to_julia(::Type{BigInt}, x::GapObj)
     Globals.IsInt(x) || throw(ConversionError(x, BigInt))
     ## get size of GAP BigInt (in limbs), multiply
     ## by 64 to get bits
-    size_limbs = @sync_noexcept ccall(:GAP_SizeInt, Cint, (Any,), x)
+    size_limbs = @lock_noexcept ccall(:GAP_SizeInt, Cint, (Any,), x)
     size = abs(size_limbs * sizeof(UInt) * 8)
     ## allocate new GMP
     new_bigint = Base.GMP.MPZ.realloc2(size)
     new_bigint.size = size_limbs
     ## Get limb address ptr
-    addr = @sync_noexcept ccall(:GAP_AddrInt, Ptr{UInt}, (Any,), x)
+    addr = @lock_noexcept ccall(:GAP_AddrInt, Ptr{UInt}, (Any,), x)
     ## Copy limbs
     unsafe_copyto!(new_bigint.d, addr, abs(size_limbs))
     return new_bigint
diff --git a/src/julia_to_gap.jl b/src/julia_to_gap.jl
index 08f41fa65..f998a29de 100644
--- a/src/julia_to_gap.jl
+++ b/src/julia_to_gap.jl
@@ -57,7 +57,7 @@ function julia_to_gap(x::BigInt)
     if x in -1<<60:(1<<60-1)
         return Int64(x)
     end
-    return @sync_noexcept ccall(:MakeObjInt, GapObj, (Ptr{UInt64}, Cint), x.d, x.size)
+    return @lock_noexcept ccall(:MakeObjInt, GapObj, (Ptr{UInt64}, Cint), x.d, x.size)
 end
 
 ## Rationals
diff --git a/src/sync.jl b/src/sync.jl
index 1f8fb2279..1e59b9e98 100644
--- a/src/sync.jl
+++ b/src/sync.jl
@@ -5,12 +5,12 @@ module Sync
     const gap_lock = ReentrantLock()
     const pinned_thread = Ref{Int}(1)
 
-    @inline function lock()
-        Base.lock(gap_lock)
+    @inline function _lock()
+        Base._lock(gap_lock)
     end
 
-    @inline function unlock()
-        Base.unlock(gap_lock)
+    @inline function _unlock()
+        Base._unlock(gap_lock)
     end
 
     @inline function check_lock()
@@ -22,7 +22,7 @@ module Sync
     end
 
     # To switch between multi-threaded and single-threaded mode, we
-    # define functions that install the appropriate handlers for sync()
+    # define functions that install the appropriate handlers for lock()
     # etc.
     #
     # This is necessary because otherwise precompilation would fix
@@ -36,49 +36,49 @@ module Sync
     # selective recompilation of the affected functions as needed.
 
     function _mode_mutex()
-        Sync.eval(:(@inline function sync(f::Function)
+        Sync.eval(:(@inline function lock(f::Function)
             try
-                lock()
+                _lock()
                 f()
             finally
-                unlock()
+                _unlock()
             end
         end))
-        Sync.eval(:(@inline function sync_noexcept(f::Function)
-            lock()
+        Sync.eval(:(@inline function lock_noexcept(f::Function)
+            _lock()
             t = f()
-            unlock()
+            _unlock()
             t
         end))
-        Sync.eval(:(@inline function check_sync(f::Function)
+        Sync.eval(:(@inline function check_lock(f::Function)
             check_lock()
             f()
         end))
     end
 
     function _mode_pinned()
-        Sync.eval(:(@inline function sync(f::Function)
+        Sync.eval(:(@inline function lock(f::Function)
             check_pinned()
             f()
         end))
-        Sync.eval(:(@inline function sync_noexcept(f::Function)
+        Sync.eval(:(@inline function lock_noexcept(f::Function)
             check_pinned()
             f()
         end))
-        Sync.eval(:(@inline function check_sync(f::Function)
+        Sync.eval(:(@inline function check_lock(f::Function)
             check_pinned()
             f()
         end))
     end
 
-    function _mode_nosync()
-        Sync.eval(:(@inline function sync(f::Function)
+    function _mode_nolock()
+        Sync.eval(:(@inline function lock(f::Function)
             f()
         end))
-        Sync.eval(:(@inline function sync_noexcept(f::Function)
+        Sync.eval(:(@inline function lock_noexcept(f::Function)
             f()
         end))
-        Sync.eval(:(@inline function check_sync(f::Function)
+        Sync.eval(:(@inline function check_lock(f::Function)
             f()
         end))
     end
@@ -87,8 +87,8 @@ module Sync
         stack = stacktrace()
         file = stack[1].file
         for frame in stack[2:end]
-            if frame.func in [ :sync, :sync_noexcept ] && frame.file == file
-                @error "trying to change sync mode within critical region"
+            if frame.func in [ :lock, :lock_noexcept ] && frame.file == file
+                @error "trying to change lock mode within critical region"
             end
         end
         if mode == pin
@@ -96,14 +96,14 @@ module Sync
         elseif mode == mutex
             _mode_mutex()
         else # mode == disabled
-            _mode_nosync()
+            _mode_nolock()
         end
     end
 
     # Initialization is tricky. __init__() can be called from within the
-    # first sync() call if the module has already been precompiled. Thus,
+    # first lock() call if the module has already been precompiled. Thus,
     # we default to enabling synchronization during precompilation and
-    # then set the actual sync mode during __init__(). Dropping back from
+    # then set the actual lock mode during __init__(). Dropping back from
     # synchronization being enabled to being disabled is safe, but not the
     # other way round.
 
@@ -111,21 +111,21 @@ module Sync
 
     function __init__()
         if Threads.nthreads() == 1
-            _mode_nosync()
+            _mode_nolock()
         else
             _mode_pinned()
         end
     end
 end
 
-macro sync(expr)
-    :( Sync.sync(()->$(esc(expr))) )
+macro lock(expr)
+    :( Sync.lock(()->$(esc(expr))) )
 end
 
-macro sync_noexcept(expr)
-    :( Sync.sync_noexcept(()->$(esc(expr))) )
+macro lock_noexcept(expr)
+    :( Sync.lock_noexcept(()->$(esc(expr))) )
 end
 
-macro check_sync(expr)
-    :( Sync.check_sync(()->$(esc(expr))) )
+macro check_lock(expr)
+    :( Sync.check_lock(()->$(esc(expr))) )
 end

From d677f3031c0ca0fb889510584a256700f6609488 Mon Sep 17 00:00:00 2001
From: Reimer Behrends <behrends@gmail.com>
Date: Tue, 8 Sep 2020 12:06:30 +0200
Subject: [PATCH 10/10] Document GAP.Sync.switch() method.

---
 src/sync.jl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/sync.jl b/src/sync.jl
index 1e59b9e98..8492d5154 100644
--- a/src/sync.jl
+++ b/src/sync.jl
@@ -83,6 +83,20 @@ module Sync
         end))
     end
 
+    # The switch() method allows GAP users to switch at runtime between
+    # various modes:
+    #
+    # 1. Sync.pin pins the GAP interpreter to the current thread. It
+    #    cannot be called from other threads. This is the default mode,
+    #    as it combines best performance with thread safety.
+    # 2. Sync.mutex serializes all GAP calls via a global mutex. This
+    #    offers thread safety and the ability to call GAP from multiple
+    #    threads, but has worse performance than pinning the GAP interpreter
+    #    to a thread.
+    # 3. Sync.nolock removes all thread-safety checks. It eliminates all
+    #    overhead, but it is up to the user to ensure that no race
+    #    conditions occur.
+
     function switch(mode::SyncMode)
         stack = stacktrace()
         file = stack[1].file