Skip to content

Commit

Permalink
fix atomic intrinsics implementation issues (JuliaLang#49967)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@vtjnash
vtjnash authored May 30, 2023
1 parent f6f637a commit bd5e6da
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 15 deletions.
6 changes: 5 additions & 1 deletion src/intrinsics.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -926,7 +926,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)
Expand Down
12 changes: 6 additions & 6 deletions src/jltypes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1411,7 +1411,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);
Expand All @@ -1422,12 +1422,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_datatype_t*)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_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;
}

Expand Down
4 changes: 2 additions & 2 deletions src/runtime_intrinsics.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
Expand Down
48 changes: 42 additions & 6 deletions test/intrinsics.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,14 +214,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
Expand All @@ -234,7 +234,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)
Expand All @@ -248,10 +251,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))
Expand All @@ -262,6 +267,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)