Skip to content

Commit

Permalink
fix spurious errors
Browse files Browse the repository at this point in the history
  • Loading branch information
@vtjnash
vtjnash committed Mar 27, 2021
1 parent b4c7420 commit 8242c8d
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 53 deletions.
13 changes: 3 additions & 10 deletions src/atomics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,9 @@ enum jl_memory_order {
* are). We also need to access these atomic variables from the LLVM JIT code
* which is very hard unless the layout of the object is fully specified.
*/
#if defined(__GNUC__)
# define jl_fence() __atomic_thread_fence(__ATOMIC_SEQ_CST)
# define jl_fence_release() __atomic_thread_fence(__ATOMIC_RELEASE)
# define jl_signal_fence() __atomic_signal_fence(__ATOMIC_SEQ_CST)
#elif defined(_COMPILER_MICROSOFT_)
// TODO: these only define compiler barriers, and aren't correct outside of x86
# define jl_fence() _ReadWriteBarrier()
# define jl_fence_release() _WriteBarrier()
# define jl_signal_fence() _ReadWriteBarrier()
#endif
#define jl_fence() __atomic_thread_fence(__ATOMIC_SEQ_CST)
#define jl_fence_release() __atomic_thread_fence(__ATOMIC_RELEASE)
#define jl_signal_fence() __atomic_signal_fence(__ATOMIC_SEQ_CST)


# define jl_atomic_fetch_add_relaxed(obj, arg) \
Expand Down
12 changes: 6 additions & 6 deletions src/builtins.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ extern "C" {

// egal and object_id ---------------------------------------------------------

static int bits_equal(void *a, void *b, int sz) JL_NOTSAFEPOINT
static int bits_equal(const void *a, const void *b, int sz) JL_NOTSAFEPOINT
{
switch (sz) {
case 1: return *(int8_t*)a == *(int8_t*)b;
Expand Down Expand Up @@ -76,7 +76,7 @@ static int NOINLINE compare_svec(jl_svec_t *a, jl_svec_t *b) JL_NOTSAFEPOINT
}

// See comment above for an explanation of NOINLINE.
static int NOINLINE compare_fields(jl_value_t *a, jl_value_t *b, jl_datatype_t *dt) JL_NOTSAFEPOINT
static int NOINLINE compare_fields(const jl_value_t *a, const jl_value_t *b, jl_datatype_t *dt) JL_NOTSAFEPOINT
{
size_t f, nf = jl_datatype_nfields(dt);
for (f = 0; f < nf; f++) {
Expand Down Expand Up @@ -126,7 +126,7 @@ static int NOINLINE compare_fields(jl_value_t *a, jl_value_t *b, jl_datatype_t *
return 1;
}

static int egal_types(jl_value_t *a, jl_value_t *b, jl_typeenv_t *env, int tvar_names) JL_NOTSAFEPOINT
static int egal_types(const jl_value_t *a, const jl_value_t *b, jl_typeenv_t *env, int tvar_names) JL_NOTSAFEPOINT
{
if (a == b)
return 1;
Expand Down Expand Up @@ -192,13 +192,13 @@ JL_DLLEXPORT int jl_types_egal(jl_value_t *a, jl_value_t *b)
return egal_types(a, b, NULL, 0);
}

JL_DLLEXPORT int (jl_egal)(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNROOTED) JL_NOTSAFEPOINT
JL_DLLEXPORT int (jl_egal)(const jl_value_t *a JL_MAYBE_UNROOTED, const jl_value_t *b JL_MAYBE_UNROOTED) JL_NOTSAFEPOINT
{
// warning: a,b may NOT have been gc-rooted by the caller
return jl_egal(a, b);
}

int jl_egal__special(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT
int jl_egal__special(const jl_value_t *a JL_MAYBE_UNROOTED, const jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT
{
if (dt == jl_simplevector_type)
return compare_svec((jl_svec_t*)a, (jl_svec_t*)b);
Expand All @@ -221,7 +221,7 @@ int jl_egal__special(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNR
return 0;
}

int jl_egal__bits(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT
int jl_egal__bits(const jl_value_t *a JL_MAYBE_UNROOTED, const jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT
{
size_t sz = jl_datatype_size(dt);
if (sz == 0)
Expand Down
13 changes: 7 additions & 6 deletions src/datatype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1292,11 +1292,11 @@ jl_value_t *modify_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_valu
size_t offs = jl_field_offset(st, i);
jl_value_t *ty = jl_field_type_concrete(st, i);
jl_value_t *r = jl_get_nth_field_checked(v, i);
// TODO: atomic ordering fence REQUIRED here
jl_value_t **args;
JL_GC_PUSHARGS(args, 2);
args[0] = r;
while (1) {
// TODO: atomic ordering fence required here if hasptr?
args[1] = rhs;
jl_value_t *y = jl_apply_generic(op, args, 2);
args[1] = y;
Expand All @@ -1306,8 +1306,6 @@ jl_value_t *modify_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_valu
// TODO: jl_fence(); // `st->[idx]` will have at least relaxed ordering
if (jl_field_isptr(st, i)) {
jl_value_t **p = (jl_value_t**)((char*)v + offs);
// TODO: this should loop until !jl_egal(r, args[0])
// TODO: may need barrier before jl_egal
if (isatomic ? jl_atomic_cmpswap(p, &r, y) : jl_atomic_cmpswap_relaxed(p, &r, y))
break;
}
Expand Down Expand Up @@ -1336,7 +1334,7 @@ jl_value_t *modify_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_valu
else {
if (needlock)
jl_lock_value(v);
int success = memcmp((char*)v + offs, r, fsz) == 0; // TODO: use jl_egal_
int success = memcmp((char*)v + offs, r, fsz) == 0;
if (success) {
if (isunion) {
size_t fsz = jl_field_size(st, i);
Expand Down Expand Up @@ -1416,7 +1414,7 @@ jl_value_t *cmpswap_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_val
size_t fsz = jl_datatype_size((jl_datatype_t*)rty); // need to shrink-wrap the final copy
int needlock = (isatomic && fsz > MAX_ATOMIC_SIZE);
if (isatomic && !needlock) {
r = jl_atomic_cmpswap_bits(rty, (char*)v + offs, r, rhs, fsz);
r = jl_atomic_cmpswap_bits((jl_datatype_t*)rty, (char*)v + offs, r, rhs, fsz);
int success = *((uint8_t*)r + fsz);
if (success && hasptr)
jl_gc_multi_wb(v, rhs); // rhs is immutable
Expand Down Expand Up @@ -1450,7 +1448,10 @@ jl_value_t *cmpswap_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_val
jl_lock_value(v);
if (success) {
memcpy((char*)r, (char*)v + offs, fsz);
success = memcmp((char*)r, (char*)expected, fsz) == 0; // TODO: use jl_egal_
if (((jl_datatype_t*)rty)->layout->haspadding)
success = jl_egal__bits(r, expected, (jl_datatype_t*)rty);
else
success = memcmp((char*)r, (char*)expected, fsz) == 0;
}
*((uint8_t*)r + fsz) = success ? 1 : 0;
if (success) {
Expand Down
10 changes: 5 additions & 5 deletions src/julia.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1325,12 +1325,12 @@ STATIC_INLINE int jl_is_array_zeroinit(jl_array_t *a) JL_NOTSAFEPOINT
}

// object identity
JL_DLLEXPORT int jl_egal(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNROOTED) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_egal__bits(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_egal__special(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_egal(const jl_value_t *a JL_MAYBE_UNROOTED, const jl_value_t *b JL_MAYBE_UNROOTED) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_egal__bits(const jl_value_t *a JL_MAYBE_UNROOTED, const jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT;
JL_DLLEXPORT int jl_egal__special(const jl_value_t *a JL_MAYBE_UNROOTED, const jl_value_t *b JL_MAYBE_UNROOTED, jl_datatype_t *dt) JL_NOTSAFEPOINT;
JL_DLLEXPORT uintptr_t jl_object_id(jl_value_t *v) JL_NOTSAFEPOINT;

STATIC_INLINE int jl_egal_(jl_value_t *a JL_MAYBE_UNROOTED, jl_value_t *b JL_MAYBE_UNROOTED) JL_NOTSAFEPOINT
STATIC_INLINE int jl_egal_(const jl_value_t *a JL_MAYBE_UNROOTED, const jl_value_t *b JL_MAYBE_UNROOTED) JL_NOTSAFEPOINT
{
if (a == b)
return 1;
Expand Down Expand Up @@ -1405,7 +1405,7 @@ JL_DLLEXPORT jl_value_t *jl_atomic_new_bits(jl_value_t *dt, const char *src);
JL_DLLEXPORT void jl_atomic_store_bits(char *dst, const jl_value_t *src, int nb);
JL_DLLEXPORT jl_value_t *jl_atomic_swap_bits(jl_value_t *dt, char *dst, const jl_value_t *src, int nb);
JL_DLLEXPORT int jl_atomic_bool_cmpswap_bits(char *dst, const jl_value_t *expected, const jl_value_t *src, int nb);
JL_DLLEXPORT jl_value_t *jl_atomic_cmpswap_bits(jl_value_t *dt, char *dst, const jl_value_t *expected, const jl_value_t *src, int nb);
JL_DLLEXPORT jl_value_t *jl_atomic_cmpswap_bits(jl_datatype_t *dt, char *dst, const jl_value_t *expected, const jl_value_t *src, int nb);
JL_DLLEXPORT jl_value_t *jl_new_struct(jl_datatype_t *type, ...);
JL_DLLEXPORT jl_value_t *jl_new_structv(jl_datatype_t *type, jl_value_t **args, uint32_t na);
JL_DLLEXPORT jl_value_t *jl_new_structt(jl_datatype_t *type, jl_value_t *tup);
Expand Down
55 changes: 36 additions & 19 deletions src/runtime_intrinsics.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ JL_DLLEXPORT jl_value_t *jl_pointerref(jl_value_t *p, jl_value_t *i, jl_value_t
jl_value_t *ety = jl_tparam0(jl_typeof(p));
if (ety == (jl_value_t*)jl_any_type) {
jl_value_t **pp = (jl_value_t**)(jl_unbox_long(p) + (jl_unbox_long(i)-1)*sizeof(void*));
return *pp;
return *pp; // TODO: change alignment assumption?
}
else {
if (!jl_is_datatype(ety))
Expand All @@ -60,7 +60,7 @@ JL_DLLEXPORT jl_value_t *jl_pointerset(jl_value_t *p, jl_value_t *x, jl_value_t
jl_value_t *ety = jl_tparam0(jl_typeof(p));
if (ety == (jl_value_t*)jl_any_type) {
jl_value_t **pp = (jl_value_t**)(jl_unbox_long(p) + (jl_unbox_long(i)-1)*sizeof(void*));
*pp = x;
*pp = x; // TODO: change alignment assumption?
}
else {
if (!jl_is_datatype(ety))
Expand Down Expand Up @@ -192,7 +192,7 @@ JL_DLLEXPORT jl_value_t *jl_atomic_swap_bits(jl_value_t *dt, char *dst, const jl
JL_DLLEXPORT int jl_atomic_bool_cmpswap_bits(char *dst, const jl_value_t *expected, const jl_value_t *src, int nb)
{
// dst must have the required alignment for an atomic of the given size
// TODO: this can spuriously fail if there are padding bits, the caller should deal with that
// n.b.: this can spuriously fail if there are padding bits, the caller should deal with that
int success;
switch (nb) {
case 1: {
Expand Down Expand Up @@ -232,18 +232,18 @@ JL_DLLEXPORT int jl_atomic_bool_cmpswap_bits(char *dst, const jl_value_t *expect
return success;
}

JL_DLLEXPORT jl_value_t *jl_atomic_cmpswap_bits(jl_value_t *dt, char *dst, const jl_value_t *expected, const jl_value_t *src, int nb)
JL_DLLEXPORT jl_value_t *jl_atomic_cmpswap_bits(jl_datatype_t *dt, char *dst, const jl_value_t *expected, const jl_value_t *src, int nb)
{
// dst must have the required alignment for an atomic of the given size
// TODO: this can spuriously fail if there are padding bits, the caller should deal with that
// n.b.: this does not spuriously fail if there are padding bits
jl_value_t *params[2];
params[0] = dt;
params[0] = (jl_value_t*)dt;
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)
int isptr = jl_field_isptr(tuptyp, 0);
jl_ptls_t ptls = jl_get_ptls_states();
jl_value_t *y = jl_gc_alloc(ptls, isptr ? nb : tuptyp->size, isptr ? dt : (jl_value_t*)tuptyp);
jl_value_t *y = jl_gc_alloc(ptls, isptr ? nb : tuptyp->size, isptr ? dt : tuptyp);
int success;
switch (nb) {
case 1: {
Expand All @@ -255,28 +255,45 @@ JL_DLLEXPORT jl_value_t *jl_atomic_cmpswap_bits(jl_value_t *dt, char *dst, const
case 2: {
uint16_t *y16 = (uint16_t*)y;
*y16 = *(uint16_t*)expected;
success = jl_atomic_cmpswap((uint16_t*)dst, y16, *(uint16_t*)src);
while (1) {
success = jl_atomic_cmpswap((uint16_t*)dst, y16, *(uint16_t*)src);
if (success || !dt->layout->haspadding || !jl_egal__bits(y, expected, dt))
break;
}
break;
}
case 4: {
uint32_t *y32 = (uint32_t*)y;
*y32 = *(uint32_t*)expected;
success = jl_atomic_cmpswap((uint32_t*)dst, y32, *(uint32_t*)src);
while (1) {
success = jl_atomic_cmpswap((uint32_t*)dst, y32, *(uint32_t*)src);
if (success || !dt->layout->haspadding || !jl_egal__bits(y, expected, dt))
break;
}
// TODO: while loop
break;
}
#if MAX_POINTERATOMIC_SIZE > 4
case 8: {
uint64_t *y64 = (uint64_t*)y;
*y64 = *(uint64_t*)expected;
success = jl_atomic_cmpswap((uint64_t*)dst, y64, *(uint64_t*)src);
while (1) {
success = jl_atomic_cmpswap((uint64_t*)dst, y64, *(uint64_t*)src);
if (success || !dt->layout->haspadding || !jl_egal__bits(y, expected, dt))
break;
}
break;
}
#endif
#if MAX_POINTERATOMIC_SIZE > 8
case 16: {
uint128_t *y128 = (uint128_t*)y;
*y128 = *(uint128_t*)expected;
success = jl_atomic_cmpswap((uint128_t*)dst, y128, *(uint128_t*)src);
while (1) {
success = jl_atomic_cmpswap((uint128_t*)dst, y128, *(uint128_t*)src);
if (success || !dt->layout->haspadding || !jl_egal__bits(y, expected, dt))
break;
}
break;
}
#else
Expand Down Expand Up @@ -375,12 +392,11 @@ JL_DLLEXPORT jl_value_t *jl_atomic_pointermodify(jl_value_t *p, jl_value_t *f, j
JL_GC_PUSHARGS(args, 2);
args[0] = expected;
while (1) {
// TODO: atomic ordering fence required here if hasptr?
args[1] = x;
jl_value_t *y = jl_apply_generic(f, args, 2);
args[1] = y;
if (ety == (jl_value_t*)jl_any_type) {
// TODO: this should loop, not just check once
// TODO: may need barrier before jl_egal
if (jl_atomic_cmpswap((jl_value_t**)pp, &expected, y))
break;
}
Expand Down Expand Up @@ -415,11 +431,12 @@ JL_DLLEXPORT jl_value_t *jl_atomic_pointercmpswap(jl_value_t *p, jl_value_t *exp
jl_value_t **result;
JL_GC_PUSHARGS(result, 2);
result[0] = expected;
// TODO: this should loop, not just check once
int success = jl_atomic_cmpswap((jl_value_t**)pp, &result[0], x);
// TODO: may need barrier before jl_egal
if (!success && jl_egal(expected, result[0])) {
success = jl_atomic_cmpswap((jl_value_t**)pp, &result[0], x);
int success;
while (1) {
success = jl_atomic_cmpswap((jl_value_t**)pp, &result[0], x);
// TODO: may need barrier before jl_egal
if (success || !jl_egal(result[0], expected))
break;
}
result[1] = success ? jl_true : jl_false;
result[0] = jl_f_tuple(NULL, result, 2);
Expand All @@ -436,7 +453,7 @@ JL_DLLEXPORT jl_value_t *jl_atomic_pointercmpswap(jl_value_t *p, jl_value_t *exp
size_t nb = jl_datatype_size(ety);
if ((nb & (nb - 1)) != 0 || nb > MAX_POINTERATOMIC_SIZE)
jl_error("pointercmpswap: invalid atomic operation");
return jl_atomic_cmpswap_bits(ety, pp, expected, x, nb);
return jl_atomic_cmpswap_bits((jl_datatype_t*)ety, pp, expected, x, nb);
}
}

Expand Down
26 changes: 19 additions & 7 deletions test/intrinsics.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,15 +162,18 @@ primitive type Int512 <: Signed 512 end
Int512(i::Int) = Core.Intrinsics.sext_int(Int512, i)
function add(i::T, j)::T where {T}; return i + j; end
swap(i, j) = j
for T in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Complex{Int512})
r = Ref{T}(10)
for TT in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Complex{Int512}, Any)
T(x) = convert(TT, x)
r = Ref{TT}(10)
p = Base.unsafe_convert(Ptr{eltype(r)}, r)
GC.@preserve r begin
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_pointercmpswap(p, T(100), S(2), :sequentially_consistent, :sequentially_consistent)
@test_throws TypeError Core.Intrinsics.atomic_pointercmpswap(p, S(100), T(2), :sequentially_consistent, :sequentially_consistent)
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_pointercmpswap(p, T(100), S(2), :sequentially_consistent, :sequentially_consistent)
@test_throws TypeError Core.Intrinsics.atomic_pointercmpswap(p, S(100), T(2), :sequentially_consistent, :sequentially_consistent)
end
@test Core.Intrinsics.pointerref(p, 1, 1) === T(10) === r[]
if sizeof(r) > 2 * sizeof(Int)
@test_throws ErrorException Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent)
Expand All @@ -181,7 +184,7 @@ for T in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Com
@test_throws ErrorException Core.Intrinsics.atomic_pointercmpswap(p, T(100), T(2), :sequentially_consistent, :sequentially_consistent)
@test Core.Intrinsics.pointerref(p, 1, 1) === T(10) === r[]
else
@test_throws TypeError Core.Intrinsics.atomic_pointermodify(p, swap, S(1), :sequentially_consistent)
TT !== Any && @test_throws TypeError Core.Intrinsics.atomic_pointermodify(p, swap, S(1), :sequentially_consistent)
@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 @@ -195,5 +198,14 @@ for T in (Int8, Int16, Int32, Int64, Int128, Int256, Int512, Complex{Int32}, Com
@test Core.Intrinsics.atomic_pointerswap(p, T(103), :sequentially_consistent) === T(102)
@test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === T(103)
end
if TT === Any
@test Core.Intrinsics.atomic_pointermodify(p, swap, S(103), :sequentially_consistent) === T(103)
@test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === S(103)
@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_pointercmpswap(p, T(100), S(2), :sequentially_consistent, :sequentially_consistent) === (S(100), false)
@test Core.Intrinsics.atomic_pointercmpswap(p, S(100), T(2), :sequentially_consistent, :sequentially_consistent) === (S(100), true)
@test Core.Intrinsics.atomic_pointerref(p, :sequentially_consistent) === T(2)
end
end
end

0 comments on commit 8242c8d

Please sign in to comment.