From 50c86112bc83ba849d10532899909bd01436577f Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Tue, 30 May 2023 15:11:00 -0400 Subject: [PATCH] fix atomic intrinsics implementation issues (#49967) * jltypes: add missing GC root for cmpswap_type Tuple. This is called with a fieldtype, which might not even be a DataType. * support Ptr{Union{}} and Ptr{Cvoid} better (cherry picked from commit bd5e6da50f8e9937482bc4317fd571a725b39fde) --- src/intrinsics.cpp | 6 ++++- src/jltypes.c | 12 +++++----- src/runtime_intrinsics.c | 4 ++-- test/intrinsics.jl | 48 +++++++++++++++++++++++++++++++++++----- 4 files changed, 55 insertions(+), 15 deletions(-) diff --git a/src/intrinsics.cpp b/src/intrinsics.cpp index a6411104124330..e0ef95e177c00b 100644 --- a/src/intrinsics.cpp +++ b/src/intrinsics.cpp @@ -932,7 +932,11 @@ static jl_cgval_t emit_atomic_pointerop(jl_codectx_t &ctx, intrinsic f, const jl bool isboxed; Type *ptrty = julia_type_to_llvm(ctx, ety, &isboxed); assert(!isboxed); - Value *thePtr = emit_unbox(ctx, ptrty->getPointerTo(), e, e.typ); + Value *thePtr; + if (!type_is_ghost(ptrty)) + thePtr = emit_unbox(ctx, ptrty->getPointerTo(), e, e.typ); + else + thePtr = nullptr; // could use any value here, since typed_store will not use it jl_cgval_t ret = typed_store(ctx, thePtr, nullptr, x, y, ety, ctx.tbaa().tbaa_data, nullptr, nullptr, isboxed, llvm_order, llvm_failorder, nb, false, issetfield, isreplacefield, isswapfield, ismodifyfield, false, modifyop, "atomic_pointermodify"); if (issetfield) diff --git a/src/jltypes.c b/src/jltypes.c index 6eb6b0b18f807f..657f7f2748c5e8 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -1171,7 +1171,7 @@ jl_datatype_t *jl_apply_modify_type(jl_value_t *dt) return rettyp; } -jl_datatype_t *jl_apply_cmpswap_type(jl_value_t *dt) +jl_datatype_t *jl_apply_cmpswap_type(jl_value_t *ty) { jl_value_t *params[2]; jl_value_t *names = jl_atomic_load_relaxed(&cmpswap_names); @@ -1182,12 +1182,12 @@ jl_datatype_t *jl_apply_cmpswap_type(jl_value_t *dt) if (jl_atomic_cmpswap(&cmpswap_names, &names, lnames)) names = jl_atomic_load_relaxed(&cmpswap_names); // == lnames } - params[0] = dt; + params[0] = ty; params[1] = (jl_value_t*)jl_bool_type; - jl_datatype_t *tuptyp = jl_apply_tuple_type_v(params, 2); - JL_GC_PROMISE_ROOTED(tuptyp); // (JL_ALWAYS_LEAFTYPE) - jl_datatype_t *rettyp = (jl_datatype_t*)jl_apply_type2((jl_value_t*)jl_namedtuple_type, names, (jl_value_t*)tuptyp); - JL_GC_PROMISE_ROOTED(rettyp); // (JL_ALWAYS_LEAFTYPE) + jl_value_t *tuptyp = (jl_value_t*)jl_apply_tuple_type_v(params, 2); + JL_GC_PUSH1(&tuptyp); + jl_datatype_t *rettyp = (jl_datatype_t*)jl_apply_type2((jl_value_t*)jl_namedtuple_type, names, tuptyp); + JL_GC_POP(); return rettyp; } diff --git a/src/runtime_intrinsics.c b/src/runtime_intrinsics.c index a170c81c609cf6..d46ed4c9c8e847 100644 --- a/src/runtime_intrinsics.c +++ b/src/runtime_intrinsics.c @@ -429,6 +429,8 @@ JL_DLLEXPORT jl_value_t *jl_atomic_pointerreplace(jl_value_t *p, jl_value_t *exp jl_atomic_error("atomic_pointerreplace: invalid atomic ordering"); // TODO: filter other invalid orderings jl_value_t *ety = jl_tparam0(jl_typeof(p)); + if (!is_valid_intrinsic_elptr(ety)) + jl_error("atomic_pointerreplace: invalid pointer"); char *pp = (char*)jl_unbox_long(p); jl_datatype_t *rettyp = jl_apply_cmpswap_type(ety); JL_GC_PROMISE_ROOTED(rettyp); // (JL_ALWAYS_LEAFTYPE) @@ -447,8 +449,6 @@ JL_DLLEXPORT jl_value_t *jl_atomic_pointerreplace(jl_value_t *p, jl_value_t *exp return result; } else { - if (!is_valid_intrinsic_elptr(ety)) - jl_error("atomic_pointerreplace: invalid pointer"); if (jl_typeof(x) != ety) jl_type_error("atomic_pointerreplace", ety, x); size_t nb = jl_datatype_size(ety); diff --git a/test/intrinsics.jl b/test/intrinsics.jl index dec4412ffd4d54..78c6af93e0078a 100644 --- a/test/intrinsics.jl +++ b/test/intrinsics.jl @@ -215,14 +215,14 @@ swap(i, j) = j for TT in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Complex{Int512}, Any) r = Ref{TT}(10) GC.@preserve r begin - (function (::Type{TT}) where TT + (@noinline function (::Type{TT}) where TT p = Base.unsafe_convert(Ptr{TT}, r) T(x) = convert(TT, x) S = UInt32 if TT !== Any @test_throws TypeError Core.Intrinsics.atomic_pointerset(p, S(1), :sequentially_consistent) - @test_throws TypeError Core.Intrinsics.atomic_pointerswap(p, S(100), :sequentially_consistent) - @test_throws TypeError Core.Intrinsics.atomic_pointerreplace(p, T(100), S(2), :sequentially_consistent, :sequentially_consistent) + @test_throws TypeError Core.Intrinsics.atomic_pointerswap(p, S(2), :sequentially_consistent) + @test_throws TypeError Core.Intrinsics.atomic_pointerreplace(p, T(10), S(3), :sequentially_consistent, :sequentially_consistent) end @test Core.Intrinsics.pointerref(p, 1, 1) === T(10) === r[] if sizeof(r) > 8 @@ -235,7 +235,10 @@ for TT in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Co @test_throws ErrorException("atomic_pointerreplace: invalid pointer for atomic operation") Core.Intrinsics.atomic_pointerreplace(p, S(100), T(2), :sequentially_consistent, :sequentially_consistent) @test Core.Intrinsics.pointerref(p, 1, 1) === T(10) === r[] else - TT !== Any && @test_throws TypeError Core.Intrinsics.atomic_pointermodify(p, swap, S(1), :sequentially_consistent) + if TT !== Any + @test_throws TypeError Core.Intrinsics.atomic_pointermodify(p, swap, S(4), :sequentially_consistent) + @test_throws TypeError Core.Intrinsics.atomic_pointermodify(p, Returns(S(5)), T(10), :sequentially_consistent) + end @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === T(10) @test Core.Intrinsics.atomic_pointerset(p, T(1), :sequentially_consistent) === p @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === T(1) @@ -249,10 +252,12 @@ for TT in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Co @test Core.Intrinsics.atomic_pointerswap(p, T(103), :sequentially_consistent) === T(102) @test Core.Intrinsics.atomic_pointerreplace(p, S(100), T(2), :sequentially_consistent, :sequentially_consistent) === ReplaceType{TT}((T(103), false)) @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === T(103) + @test Core.Intrinsics.atomic_pointermodify(p, Returns(T(105)), nothing, :sequentially_consistent) === Pair{TT,TT}(T(103), T(105)) + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === T(105) end if TT === Any - @test Core.Intrinsics.atomic_pointermodify(p, swap, S(103), :sequentially_consistent) === Pair{TT,TT}(T(103), S(103)) - @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === S(103) + @test Core.Intrinsics.atomic_pointermodify(p, swap, S(105), :sequentially_consistent) === Pair{TT,TT}(T(105), S(105)) + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === S(105) @test Core.Intrinsics.atomic_pointerset(p, S(1), :sequentially_consistent) === p @test Core.Intrinsics.atomic_pointerswap(p, S(100), :sequentially_consistent) === S(1) @test Core.Intrinsics.atomic_pointerreplace(p, T(100), S(2), :sequentially_consistent, :sequentially_consistent) === ReplaceType{TT}((S(100), false)) @@ -263,6 +268,37 @@ for TT in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Co end end +for TT in (Ptr{Nothing}, Ptr) + r = Ref(nothing) + GC.@preserve r begin + p = Ref{TT}(Base.unsafe_convert(Ptr{Nothing}, r)) + (@noinline function (p::Ref) + p = p[] + S = UInt32 + @test_throws TypeError Core.Intrinsics.atomic_pointerset(p, S(1), :sequentially_consistent) + @test_throws TypeError Core.Intrinsics.atomic_pointerswap(p, S(100), :sequentially_consistent) + @test_throws TypeError Core.Intrinsics.atomic_pointerreplace(p, nothing, S(2), :sequentially_consistent, :sequentially_consistent) + @test Core.Intrinsics.pointerref(p, 1, 1) === nothing === r[] + @test_throws TypeError Core.Intrinsics.atomic_pointermodify(p, swap, S(1), :sequentially_consistent) + @test_throws TypeError Core.Intrinsics.atomic_pointermodify(p, Returns(S(1)), nothing, :sequentially_consistent) + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === nothing + @test Core.Intrinsics.atomic_pointerset(p, nothing, :sequentially_consistent) === p + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === nothing + @test Core.Intrinsics.atomic_pointerreplace(p, nothing, nothing, :sequentially_consistent, :sequentially_consistent) === ReplaceType{Nothing}((nothing, true)) + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === nothing + @test Core.Intrinsics.atomic_pointerreplace(p, S(1), nothing, :sequentially_consistent, :sequentially_consistent) === ReplaceType{Nothing}((nothing, false)) + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === nothing + @test Core.Intrinsics.atomic_pointermodify(p, Returns(nothing), nothing, :sequentially_consistent) === Pair{Nothing,Nothing}(nothing, nothing) + @test Core.Intrinsics.atomic_pointermodify(p, Returns(nothing), S(1), :sequentially_consistent) === Pair{Nothing,Nothing}(nothing, nothing) + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === nothing + @test Core.Intrinsics.atomic_pointerswap(p, nothing, :sequentially_consistent) === nothing + @test Core.Intrinsics.atomic_pointerreplace(p, S(100), nothing, :sequentially_consistent, :sequentially_consistent) === ReplaceType{Nothing}((nothing, false)) + @test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === nothing + end)(p,) + end +end + + mutable struct IntWrap <: Signed x::Int end