From e1a8b94b9a144442725fd7c92a75479ce3abd370 Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Thu, 11 May 2023 12:02:51 -0400 Subject: [PATCH 1/5] Allow for :foreigncall to transition to GC safe automatically Co-authored-by: Gabriel Baraldi --- Compiler/src/abstractinterpretation.jl | 2 +- Compiler/src/validation.jl | 2 +- base/c.jl | 51 ++++++++++++++++++++++---- base/meta.jl | 2 +- base/strings/string.jl | 2 +- doc/src/devdocs/ast.md | 4 +- src/ccall.cpp | 16 ++++++-- src/codegen.cpp | 4 +- src/llvm-codegen-shared.h | 14 +++---- src/llvm-late-gc-lowering.cpp | 41 ++++++++++++++++----- src/llvm-pass-helpers.cpp | 21 +++++++++++ src/llvm-pass-helpers.h | 5 ++- src/llvm-ptls.cpp | 7 +++- src/method.c | 5 ++- test/ccall.jl | 11 ++++++ 15 files changed, 146 insertions(+), 41 deletions(-) diff --git a/Compiler/src/abstractinterpretation.jl b/Compiler/src/abstractinterpretation.jl index 8a7e8aee715a6..73949068ce8c6 100644 --- a/Compiler/src/abstractinterpretation.jl +++ b/Compiler/src/abstractinterpretation.jl @@ -3409,7 +3409,7 @@ function abstract_eval_foreigncall(interp::AbstractInterpreter, e::Expr, sstate: abstract_eval_value(interp, x, sstate, sv) end cconv = e.args[5] - if isa(cconv, QuoteNode) && (v = cconv.value; isa(v, Tuple{Symbol, UInt16})) + if isa(cconv, QuoteNode) && (v = cconv.value; isa(v, Tuple{Symbol, UInt16, Bool})) override = decode_effects_override(v[2]) effects = override_effects(effects, override) end diff --git a/Compiler/src/validation.jl b/Compiler/src/validation.jl index 9bde405a49956..4f9362e97b30d 100644 --- a/Compiler/src/validation.jl +++ b/Compiler/src/validation.jl @@ -23,7 +23,7 @@ const VALID_EXPR_HEADS = IdDict{Symbol,UnitRange{Int}}( :meta => 0:typemax(Int), :global => 1:1, :globaldecl => 1:2, - :foreigncall => 5:typemax(Int), # name, RT, AT, nreq, (cconv, effects), args..., roots... + :foreigncall => 5:typemax(Int), # name, RT, AT, nreq, (cconv, effects, gc_safe), args..., roots... :cfunction => 5:5, :isdefined => 1:2, :code_coverage_effect => 0:0, diff --git a/base/c.jl b/base/c.jl index c1b34579e0a0b..b02c3293f5269 100644 --- a/base/c.jl +++ b/base/c.jl @@ -268,7 +268,32 @@ The above input outputs this: (:printf, :Cvoid, [:Cstring, :Cuint], ["%d", :value]) """ -function ccall_macro_parse(expr::Expr) +function ccall_macro_parse(exprs) + gc_safe = false + expr = nothing + ccall(:jl_, Cvoid, (Any,), exprs) + if exprs isa Expr + expr = exprs + elseif length(exprs) == 1 + expr = exprs[1] + elseif length(exprs) == 2 + gc_expr = exprs[1] + expr = exprs[2] + if gc_expr.head == :(=) && gc_expr.args[1] == :gc_safe + if gc_expr.args[2] == true + gc_safe = true + elseif gc_expr.args[2] == false + gc_safe = false + else + throw(ArgumentError("gc_safe must be true or false")) + end + else + throw(ArgumentError("@ccall option must be `gc_safe=true` or `gc_safe=false`")) + end + else + throw(ArgumentError("@ccall needs a function signature with a return type")) + end + # setup and check for errors if !isexpr(expr, :(::)) throw(ArgumentError("@ccall needs a function signature with a return type")) @@ -328,12 +353,11 @@ function ccall_macro_parse(expr::Expr) pusharg!(a) end end - - return func, rettype, types, args, nreq + return func, rettype, types, args, gc_safe, nreq end -function ccall_macro_lower(convention, func, rettype, types, args, nreq) +function ccall_macro_lower(convention, func, rettype, types, args, gc_safe, nreq) statements = [] # if interpolation was used, ensure the value is a function pointer at runtime. @@ -351,9 +375,15 @@ function ccall_macro_lower(convention, func, rettype, types, args, nreq) else func = esc(func) end + cconv = nothing + if convention isa Tuple + cconv = Expr(:cconv, (convention..., gc_safe), nreq) + else + cconv = Expr(:cconv, (convention, UInt16(0), gc_safe), nreq) + end return Expr(:block, statements..., - Expr(:call, :ccall, func, Expr(:cconv, convention, nreq), esc(rettype), + Expr(:call, :ccall, func, cconv, esc(rettype), Expr(:tuple, map(esc, types)...), map(esc, args)...)) end @@ -404,9 +434,16 @@ Example using an external library: The string literal could also be used directly before the function name, if desired `"libglib-2.0".g_uri_escape_string(...` + +It's possible to declare the ccall as `gc_safe` by using the `gc_safe = true` option: + @ccall gc_safe=true strlen(s::Cstring)::Csize_t +This allows the garbage collector to run concurrently with the ccall, which can be useful whenever +the ccall may block outside of julia. +WARNING: This option should be used with caution, as it can lead to undefined behavior if the ccall +calls back into the julia runtime. (@cfunction/@ccallables are safe however) """ -macro ccall(expr) - return ccall_macro_lower(:ccall, ccall_macro_parse(expr)...) +macro ccall(exprs...) + return ccall_macro_lower((:ccall), ccall_macro_parse(exprs)...) end macro ccall_effects(effects::UInt16, expr) diff --git a/base/meta.jl b/base/meta.jl index 36875b8e2c625..4807b910c494a 100644 --- a/base/meta.jl +++ b/base/meta.jl @@ -427,7 +427,7 @@ function _partially_inline!(@nospecialize(x), slot_replacements::Vector{Any}, elseif i == 4 @assert isa(x.args[4], Int) elseif i == 5 - @assert isa((x.args[5]::QuoteNode).value, Union{Symbol, Tuple{Symbol, UInt8}}) + @assert isa((x.args[5]::QuoteNode).value, Union{Symbol, Tuple{Symbol, UInt16, Bool}}) else x.args[i] = _partially_inline!(x.args[i], slot_replacements, type_signature, static_param_values, diff --git a/base/strings/string.jl b/base/strings/string.jl index 9f3c3d00e4b81..79ec12d11cb94 100644 --- a/base/strings/string.jl +++ b/base/strings/string.jl @@ -107,7 +107,7 @@ end # but the macro is not available at this time in bootstrap, so we write it manually. const _string_n_override = 0x04ee @eval _string_n(n::Integer) = $(Expr(:foreigncall, QuoteNode(:jl_alloc_string), Ref{String}, - :(Core.svec(Csize_t)), 1, QuoteNode((:ccall, _string_n_override)), :(convert(Csize_t, n)))) + :(Core.svec(Csize_t)), 1, QuoteNode((:ccall, _string_n_override, false)), :(convert(Csize_t, n)))) """ String(s::AbstractString) diff --git a/doc/src/devdocs/ast.md b/doc/src/devdocs/ast.md index fe63dfe35edac..706dfc34875fa 100644 --- a/doc/src/devdocs/ast.md +++ b/doc/src/devdocs/ast.md @@ -498,9 +498,9 @@ These symbols appear in the `head` field of [`Expr`](@ref)s in lowered form. The number of required arguments for a varargs function definition. - * `args[5]::QuoteNode{<:Union{Symbol,Tuple{Symbol,UInt16}}`: calling convention + * `args[5]::QuoteNode{<:Union{Symbol,Tuple{Symbol,UInt16}, Tuple{Symbol,UInt16,Bool}}`: calling convention - The calling convention for the call, optionally with effects. + The calling convention for the call, optionally with effects, and `gc_safe` (safe to execute concurrently to GC.). * `args[6:5+length(args[3])]` : arguments diff --git a/src/ccall.cpp b/src/ccall.cpp index 6c03f9532d9a8..70ead262751d4 100644 --- a/src/ccall.cpp +++ b/src/ccall.cpp @@ -1134,6 +1134,7 @@ class function_sig_t { AttributeList attributes; // vector of function call site attributes Type *lrt; // input parameter of the llvm return type (from julia_struct_to_llvm) bool retboxed; // input parameter indicating whether lrt is jl_value_t* + bool gc_safe; // input parameter indicating whether the call is safe to execute concurrently to GC Type *prt; // out parameter of the llvm return type for the function signature int sret; // out parameter for indicating whether return value has been moved to the first argument position std::string err_msg; @@ -1146,8 +1147,8 @@ class function_sig_t { size_t nreqargs; // number of required arguments in ccall function definition jl_codegen_params_t *ctx; - function_sig_t(const char *fname, Type *lrt, jl_value_t *rt, bool retboxed, jl_svec_t *at, jl_unionall_t *unionall_env, size_t nreqargs, CallingConv::ID cc, bool llvmcall, jl_codegen_params_t *ctx) - : lrt(lrt), retboxed(retboxed), + function_sig_t(const char *fname, Type *lrt, jl_value_t *rt, bool retboxed, bool gc_safe, jl_svec_t *at, jl_unionall_t *unionall_env, size_t nreqargs, CallingConv::ID cc, bool llvmcall, jl_codegen_params_t *ctx) + : lrt(lrt), retboxed(retboxed), gc_safe(gc_safe), prt(NULL), sret(0), cc(cc), llvmcall(llvmcall), at(at), rt(rt), unionall_env(unionall_env), nccallargs(jl_svec_len(at)), nreqargs(nreqargs), @@ -1295,6 +1296,9 @@ std::string generate_func_sig(const char *fname) RetAttrs = RetAttrs.addAttribute(LLVMCtx, Attribute::NonNull); if (rt == jl_bottom_type) FnAttrs = FnAttrs.addAttribute(LLVMCtx, Attribute::NoReturn); + if (gc_safe) + FnAttrs = FnAttrs.addAttribute(LLVMCtx, "julia.gc_safe"); + assert(attributes.isEmpty()); attributes = AttributeList::get(LLVMCtx, FnAttrs, RetAttrs, paramattrs); return ""; @@ -1412,7 +1416,7 @@ static const std::string verify_ccall_sig(jl_value_t *&rt, jl_value_t *at, const int fc_args_start = 6; -// Expr(:foreigncall, pointer, rettype, (argtypes...), nreq, [cconv | (cconv, effects)], args..., roots...) +// Expr(:foreigncall, pointer, rettype, (argtypes...), nreq, gc_safe, [cconv | (cconv, effects)], args..., roots...) static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs) { JL_NARGSV(ccall, 5); @@ -1424,11 +1428,15 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs) assert(jl_is_quotenode(args[5])); jl_value_t *jlcc = jl_quotenode_value(args[5]); jl_sym_t *cc_sym = NULL; + // TODO: Can we introduce a intrinsic token = @julia.gc_safe_begin() + // and "grow" gc safe regions so that we minimize the overhead? + bool gc_safe = false; if (jl_is_symbol(jlcc)) { cc_sym = (jl_sym_t*)jlcc; } else if (jl_is_tuple(jlcc)) { cc_sym = (jl_sym_t*)jl_get_nth_field_noalloc(jlcc, 0); + gc_safe = jl_unbox_bool(jl_get_nth_field_checked(jlcc, 2)); } assert(jl_is_symbol(cc_sym)); native_sym_arg_t symarg = {}; @@ -1547,7 +1555,7 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs) } if (rt != args[2] && rt != (jl_value_t*)jl_any_type) jl_temporary_root(ctx, rt); - function_sig_t sig("ccall", lrt, rt, retboxed, + function_sig_t sig("ccall", lrt, rt, retboxed, gc_safe, (jl_svec_t*)at, unionall, nreqargs, cc, llvmcall, &ctx.emission_context); for (size_t i = 0; i < nccallargs; i++) { diff --git a/src/codegen.cpp b/src/codegen.cpp index 5507bd7bad801..2312e99ed1d78 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -8065,7 +8065,7 @@ static jl_cgval_t emit_cfunction(jl_codectx_t &ctx, jl_value_t *output_type, con if (rt != declrt && rt != (jl_value_t*)jl_any_type) jl_temporary_root(ctx, rt); - function_sig_t sig("cfunction", lrt, rt, retboxed, argt, unionall_env, false, CallingConv::C, false, &ctx.emission_context); + function_sig_t sig("cfunction", lrt, rt, retboxed, false, argt, unionall_env, false, CallingConv::C, false, &ctx.emission_context); assert(sig.fargt.size() + sig.sret == sig.fargt_sig.size()); if (!sig.err_msg.empty()) { emit_error(ctx, sig.err_msg); @@ -8205,7 +8205,7 @@ const char *jl_generate_ccallable(Module *llvmmod, void *sysimg_handle, jl_value } jl_value_t *err; { // scope block for sig - function_sig_t sig("cfunction", lcrt, crt, toboxed, + function_sig_t sig("cfunction", lcrt, crt, toboxed, false, argtypes, NULL, false, CallingConv::C, false, ¶ms); if (sig.err_msg.empty()) { if (sysimg_handle) { diff --git a/src/llvm-codegen-shared.h b/src/llvm-codegen-shared.h index ff6f5a97299d7..d474fb4f61183 100644 --- a/src/llvm-codegen-shared.h +++ b/src/llvm-codegen-shared.h @@ -244,21 +244,17 @@ static inline llvm::Value *emit_gc_state_set(llvm::IRBuilder<> &builder, llvm::T unsigned offset = offsetof(jl_tls_states_t, gc_state); Value *gc_state = builder.CreateConstInBoundsGEP1_32(T_int8, ptls, offset, "gc_state"); if (old_state == nullptr) { - old_state = builder.CreateLoad(T_int8, gc_state); + old_state = builder.CreateLoad(T_int8, gc_state, "old_state"); cast(old_state)->setOrdering(AtomicOrdering::Monotonic); } builder.CreateAlignedStore(state, gc_state, Align(sizeof(void*)))->setOrdering(AtomicOrdering::Release); if (auto *C = dyn_cast(old_state)) - if (C->isZero()) - return old_state; - if (auto *C = dyn_cast(state)) - if (!C->isZero()) - return old_state; + if (auto *C2 = dyn_cast(state)) + if (C->getZExtValue() == C2->getZExtValue()) + return old_state; BasicBlock *passBB = BasicBlock::Create(builder.getContext(), "safepoint", builder.GetInsertBlock()->getParent()); BasicBlock *exitBB = BasicBlock::Create(builder.getContext(), "after_safepoint", builder.GetInsertBlock()->getParent()); - Constant *zero8 = ConstantInt::get(T_int8, 0); - builder.CreateCondBr(builder.CreateOr(builder.CreateICmpEQ(old_state, zero8), // if (!old_state || !state) - builder.CreateICmpEQ(state, zero8)), + builder.CreateCondBr(builder.CreateICmpEQ(old_state, state, "is_new_state"), // Safepoint whenever we change the GC state passBB, exitBB); builder.SetInsertPoint(passBB); MDNode *tbaa = get_tbaa_const(builder.getContext()); diff --git a/src/llvm-late-gc-lowering.cpp b/src/llvm-late-gc-lowering.cpp index 7d6fba65a79e7..6e75d52aa7d3a 100644 --- a/src/llvm-late-gc-lowering.cpp +++ b/src/llvm-late-gc-lowering.cpp @@ -2181,16 +2181,39 @@ bool LateLowerGCFrame::CleanupIR(Function &F, State *S, bool *CFGModified) { NewCall->copyMetadata(*CI); CI->replaceAllUsesWith(NewCall); UpdatePtrNumbering(CI, NewCall, S); - } else if (CI->arg_size() == CI->getNumOperands()) { - /* No operand bundle to lower */ - ++it; - continue; } else { - CallInst *NewCall = CallInst::Create(CI, None, CI); - NewCall->takeName(CI); - NewCall->copyMetadata(*CI); - CI->replaceAllUsesWith(NewCall); - UpdatePtrNumbering(CI, NewCall, S); + if (CI->getFnAttr("julia.gc_safe").isValid()) { + // Insert the operations to switch to gc_safe if necessary. + IRBuilder<> builder(CI); + Value *pgcstack = getOrAddPGCstack(F); + assert(pgcstack); + // We dont use emit_state_set here because safepoints are unconditional for any code that reaches this + // We are basically guaranteed to go from gc_unsafe to gc_safe and back, and both transitions need a safepoint + // We also can't add any BBs here, so just avoiding the branches is good + Value *ptls = get_current_ptls_from_task(builder, get_current_task_from_pgcstack(builder, pgcstack), tbaa_gcframe); + unsigned offset = offsetof(jl_tls_states_t, gc_state); + Value *gc_state = builder.CreateConstInBoundsGEP1_32(Type::getInt8Ty(builder.getContext()), ptls, offset, "gc_state"); + LoadInst *last_gc_state = builder.CreateAlignedLoad(Type::getInt8Ty(builder.getContext()), gc_state, Align(sizeof(void*))); + last_gc_state->setOrdering(AtomicOrdering::Monotonic); + builder.CreateAlignedStore(builder.getInt8(JL_GC_STATE_SAFE), gc_state, Align(sizeof(void*)))->setOrdering(AtomicOrdering::Release); + MDNode *tbaa = get_tbaa_const(builder.getContext()); + emit_gc_safepoint(builder, T_size, ptls, tbaa, false); + builder.SetInsertPoint(CI->getNextNode()); + builder.CreateAlignedStore(last_gc_state, gc_state, Align(sizeof(void*)))->setOrdering(AtomicOrdering::Release); + emit_gc_safepoint(builder, T_size, ptls, tbaa, false); + } + if (CI->arg_size() == CI->getNumOperands()) { + /* No operand bundle to lower */ + ++it; + continue; + } else { + // remove operand bundle + CallInst *NewCall = CallInst::Create(CI, None, CI); + NewCall->takeName(CI); + NewCall->copyMetadata(*CI); + CI->replaceAllUsesWith(NewCall); + UpdatePtrNumbering(CI, NewCall, S); + } } if (!CI->use_empty()) { CI->replaceAllUsesWith(UndefValue::get(CI->getType())); diff --git a/src/llvm-pass-helpers.cpp b/src/llvm-pass-helpers.cpp index ca25251040fb2..9d415d923ecb6 100644 --- a/src/llvm-pass-helpers.cpp +++ b/src/llvm-pass-helpers.cpp @@ -88,6 +88,27 @@ llvm::CallInst *JuliaPassContext::getPGCstack(llvm::Function &F) const return nullptr; } +llvm::CallInst *JuliaPassContext::getOrAddPGCstack(llvm::Function &F) +{ + if (pgcstack_getter || adoptthread_func) + for (auto &I : F.getEntryBlock()) { + if (CallInst *callInst = dyn_cast(&I)) { + Value *callee = callInst->getCalledOperand(); + if ((pgcstack_getter && callee == pgcstack_getter) || + (adoptthread_func && callee == adoptthread_func)) { + return callInst; + } + } + } + IRBuilder<> builder(&F.getEntryBlock().front()); + if (pgcstack_getter) + return builder.CreateCall(pgcstack_getter); + auto FT = FunctionType::get(PointerType::get(F.getContext(), 0), false); + auto F2 = Function::Create(FT, Function::ExternalLinkage, "julia.get_pgcstack", F.getParent()); + pgcstack_getter = F2; + return builder.CreateCall( F2); +} + llvm::Function *JuliaPassContext::getOrNull( const jl_intrinsics::IntrinsicDescription &desc) const { diff --git a/src/llvm-pass-helpers.h b/src/llvm-pass-helpers.h index d46f1f46634e6..ac08cda2d61e0 100644 --- a/src/llvm-pass-helpers.h +++ b/src/llvm-pass-helpers.h @@ -87,7 +87,10 @@ struct JuliaPassContext { // point of the given function, if there exists such a call. // Otherwise, `nullptr` is returned. llvm::CallInst *getPGCstack(llvm::Function &F) const; - + // Gets a call to the `julia.get_pgcstack' intrinsic in the entry + // point of the given function, if there exists such a call. + // Otherwise, creates a new call to the intrinsic + llvm::CallInst *getOrAddPGCstack(llvm::Function &F); // Gets the intrinsic or well-known function that conforms to // the given description if it exists in the module. If not, // `nullptr` is returned. diff --git a/src/llvm-ptls.cpp b/src/llvm-ptls.cpp index e36136859517a..e5ceec8f8a419 100644 --- a/src/llvm-ptls.cpp +++ b/src/llvm-ptls.cpp @@ -196,8 +196,13 @@ void LowerPTLS::fix_pgcstack_use(CallInst *pgcstack, Function *pgcstack_getter, last_gc_state->addIncoming(prior, fastTerm->getParent()); for (auto &BB : *pgcstack->getParent()->getParent()) { if (isa(BB.getTerminator())) { + // Don't use emit_gc_safe_leave here, as that introduces a new BB while iterating BBs builder.SetInsertPoint(BB.getTerminator()); - emit_gc_unsafe_leave(builder, T_size, get_current_ptls_from_task(builder, get_current_task_from_pgcstack(builder, phi), tbaa), last_gc_state, true); + Value *ptls = get_current_ptls_from_task(builder, get_current_task_from_pgcstack(builder, pgcstack), tbaa_gcframe); + unsigned offset = offsetof(jl_tls_states_t, gc_state); + Value *gc_state = builder.CreateConstInBoundsGEP1_32(Type::getInt8Ty(builder.getContext()), ptls, offset, "gc_state"); + builder.CreateAlignedStore(last_gc_state, gc_state, Align(sizeof(void*)))->setOrdering(AtomicOrdering::Release); + emit_gc_safepoint(builder, T_size, ptls, tbaa, true); } } } diff --git a/src/method.c b/src/method.c index 1b38a16649d8a..a5276b77b6a23 100644 --- a/src/method.c +++ b/src/method.c @@ -138,7 +138,7 @@ static jl_value_t *resolve_definition_effects(jl_value_t *expr, jl_module_t *mod return expr; } if (e->head == jl_foreigncall_sym) { - JL_NARGSV(ccall method definition, 5); // (fptr, rt, at, nreq, (cc, effects)) + JL_NARGSV(ccall method definition, 5); // (fptr, rt, at, nreq, (cc, effects, gc_safe)) jl_task_t *ct = jl_current_task; jl_value_t *rt = jl_exprarg(e, 1); jl_value_t *at = jl_exprarg(e, 2); @@ -172,11 +172,12 @@ static jl_value_t *resolve_definition_effects(jl_value_t *expr, jl_module_t *mod jl_value_t *cc = jl_quotenode_value(jl_exprarg(e, 4)); if (!jl_is_symbol(cc)) { JL_TYPECHK(ccall method definition, tuple, cc); - if (jl_nfields(cc) != 2) { + if (jl_nfields(cc) != 3) { jl_error("In ccall calling convention, expected two argument tuple or symbol."); } JL_TYPECHK(ccall method definition, symbol, jl_get_nth_field(cc, 0)); JL_TYPECHK(ccall method definition, uint16, jl_get_nth_field(cc, 1)); + JL_TYPECHK(ccall method definition, bool, jl_get_nth_field(cc, 2)); } } if (e->head == jl_call_sym && nargs > 0 && diff --git a/test/ccall.jl b/test/ccall.jl index b10504de21abc..53aed19451c01 100644 --- a/test/ccall.jl +++ b/test/ccall.jl @@ -1966,3 +1966,14 @@ let llvm = sprint(code_llvm, world_counter, ()) # the world age should be -1 in generated functions (or other pure contexts) @test (generated_world_counter() == reinterpret(UInt, -1)) end + +function gc_safe_ccall() + @ccall gc_safe=true jl_get_cpu_features()::String +end + +let llvm = sprint(code_llvm, gc_safe_ccall, ()) + # check that the call works + @test gc_safe_ccall() isa String + # check for the gc_safe store + @test occursin("store atomic i8 2", llvm) +end From 06af3f6b6ef1dd12dc8526e7b51ed876f3da4f78 Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Thu, 6 Feb 2025 10:44:40 +0100 Subject: [PATCH 2/5] Apply suggestions from code review --- base/c.jl | 4 ++-- src/ccall.cpp | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/base/c.jl b/base/c.jl index b02c3293f5269..721b2a82a2c79 100644 --- a/base/c.jl +++ b/base/c.jl @@ -438,9 +438,9 @@ name, if desired `"libglib-2.0".g_uri_escape_string(...` It's possible to declare the ccall as `gc_safe` by using the `gc_safe = true` option: @ccall gc_safe=true strlen(s::Cstring)::Csize_t This allows the garbage collector to run concurrently with the ccall, which can be useful whenever -the ccall may block outside of julia. +the `ccall` may block outside of julia. WARNING: This option should be used with caution, as it can lead to undefined behavior if the ccall -calls back into the julia runtime. (@cfunction/@ccallables are safe however) +calls back into the julia runtime. (`@cfunction`/`@ccallables` are safe however) """ macro ccall(exprs...) return ccall_macro_lower((:ccall), ccall_macro_parse(exprs)...) diff --git a/src/ccall.cpp b/src/ccall.cpp index 70ead262751d4..20b079b87b2e8 100644 --- a/src/ccall.cpp +++ b/src/ccall.cpp @@ -1428,8 +1428,6 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs) assert(jl_is_quotenode(args[5])); jl_value_t *jlcc = jl_quotenode_value(args[5]); jl_sym_t *cc_sym = NULL; - // TODO: Can we introduce a intrinsic token = @julia.gc_safe_begin() - // and "grow" gc safe regions so that we minimize the overhead? bool gc_safe = false; if (jl_is_symbol(jlcc)) { cc_sym = (jl_sym_t*)jlcc; From 8a688dd2e50d09d5c57697fd154e114ddbab1887 Mon Sep 17 00:00:00 2001 From: gbaraldi Date: Thu, 6 Feb 2025 11:10:43 -0300 Subject: [PATCH 3/5] Remove print statement --- base/c.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/base/c.jl b/base/c.jl index 721b2a82a2c79..77ef631e295a3 100644 --- a/base/c.jl +++ b/base/c.jl @@ -271,7 +271,6 @@ The above input outputs this: function ccall_macro_parse(exprs) gc_safe = false expr = nothing - ccall(:jl_, Cvoid, (Any,), exprs) if exprs isa Expr expr = exprs elseif length(exprs) == 1 From 221236e4cc56736f40635bb1b7a8b06bd20dac80 Mon Sep 17 00:00:00 2001 From: gbaraldi Date: Thu, 6 Feb 2025 13:48:26 -0300 Subject: [PATCH 4/5] Fix adopt_thread interaction + fix ccall tests --- src/llvm-ptls.cpp | 2 +- test/ccall.jl | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/llvm-ptls.cpp b/src/llvm-ptls.cpp index e5ceec8f8a419..15f5a5574a6d3 100644 --- a/src/llvm-ptls.cpp +++ b/src/llvm-ptls.cpp @@ -198,7 +198,7 @@ void LowerPTLS::fix_pgcstack_use(CallInst *pgcstack, Function *pgcstack_getter, if (isa(BB.getTerminator())) { // Don't use emit_gc_safe_leave here, as that introduces a new BB while iterating BBs builder.SetInsertPoint(BB.getTerminator()); - Value *ptls = get_current_ptls_from_task(builder, get_current_task_from_pgcstack(builder, pgcstack), tbaa_gcframe); + Value *ptls = get_current_ptls_from_task(builder, get_current_task_from_pgcstack(builder, phi), tbaa_gcframe); unsigned offset = offsetof(jl_tls_states_t, gc_state); Value *gc_state = builder.CreateConstInBoundsGEP1_32(Type::getInt8Ty(builder.getContext()), ptls, offset, "gc_state"); builder.CreateAlignedStore(last_gc_state, gc_state, Align(sizeof(void*)))->setOrdering(AtomicOrdering::Release); diff --git a/test/ccall.jl b/test/ccall.jl index 53aed19451c01..145b87adbcaf1 100644 --- a/test/ccall.jl +++ b/test/ccall.jl @@ -1745,6 +1745,7 @@ using Base: ccall_macro_parse, ccall_macro_lower :Cvoid, # returntype Any[:Cstring, :Cstring, :Cint], # argument types Any["%s = %d\n", :name, :value], # argument symbols + false, # is gc_safe 1 # number of required arguments (for varargs) ) end @@ -1757,7 +1758,7 @@ end )::Cstring))...) @test call == Base.remove_linenums!( quote - ccall($(Expr(:escape, :((:func, libstring)))), $(Expr(:cconv, :ccall, 0)), $(Expr(:escape, :Cstring)), ($(Expr(:escape, :Cstring)), $(Expr(:escape, :Cint)), $(Expr(:escape, :Cint))), $(Expr(:escape, :str)), $(Expr(:escape, :num1)), $(Expr(:escape, :num2))) + ccall($(Expr(:escape, :((:func, libstring)))), $(Expr(:cconv, (:ccall, UInt16(0), false), 0)), $(Expr(:escape, :Cstring)), ($(Expr(:escape, :Cstring)), $(Expr(:escape, :Cint)), $(Expr(:escape, :Cint))), $(Expr(:escape, :str)), $(Expr(:escape, :num1)), $(Expr(:escape, :num2))) end) local fptr = :x From dad3a35fed8224b896338f1a19d509913dbfba97 Mon Sep 17 00:00:00 2001 From: gbaraldi Date: Fri, 7 Feb 2025 09:35:49 -0300 Subject: [PATCH 5/5] Update test --- test/ccall.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ccall.jl b/test/ccall.jl index 145b87adbcaf1..c7421ed2c8a23 100644 --- a/test/ccall.jl +++ b/test/ccall.jl @@ -1969,7 +1969,7 @@ let llvm = sprint(code_llvm, world_counter, ()) end function gc_safe_ccall() - @ccall gc_safe=true jl_get_cpu_features()::String + @ccall gc_safe=true sleep(1::Cint)::Cvoid end let llvm = sprint(code_llvm, gc_safe_ccall, ())