From 203382461b43db30977c17f51929a565c1f3a229 Mon Sep 17 00:00:00 2001 From: gingerBill Date: Thu, 31 Mar 2022 00:14:49 +0100 Subject: Replace the atomic intrinsics Matching C11 in style --- src/check_builtin.cpp | 191 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 132 insertions(+), 59 deletions(-) (limited to 'src/check_builtin.cpp') diff --git a/src/check_builtin.cpp b/src/check_builtin.cpp index e480704e3..3fe0f1048 100644 --- a/src/check_builtin.cpp +++ b/src/check_builtin.cpp @@ -379,6 +379,32 @@ bool check_builtin_objc_procedure(CheckerContext *c, Operand *operand, Ast *call } } +bool check_atomic_memory_order_argument(CheckerContext *c, Ast *expr, String const &builtin_name, char const *extra_message = nullptr) { + Operand x = {}; + check_expr_with_type_hint(c, &x, expr, t_atomic_memory_order); + if (x.mode == Addressing_Invalid) { + return false; + } + if (!are_types_identical(x.type, t_atomic_memory_order) || x.mode != Addressing_Constant) { + gbString str = type_to_string(x.type); + if (extra_message) { + error(x.expr, "Expected a constant Atomic_Memory_Order value for the %s of '%.*s', got %s", extra_message, LIT(builtin_name), str); + } else { + error(x.expr, "Expected a constant Atomic_Memory_Order value for '%.*s', got %s", LIT(builtin_name), str); + } + gb_string_free(str); + return false; + } + i64 value = exact_value_to_i64(x.value); + if (value < 0 || value >= OdinAtomicMemoryOrder_COUNT) { + error(x.expr, "Illegal Atomic_Memory_Order value, got %lld", cast(long long)value); + return false; + } + + return true; + +} + bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 id, Type *type_hint) { ast_node(ce, CallExpr, call); if (ce->inlining != ProcInlining_none) { @@ -423,6 +449,11 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 // NOTE(bill): The first arg may be a Type, this will be checked case by case break; + case BuiltinProc_atomic_thread_fence: + case BuiltinProc_atomic_signal_fence: + // NOTE(bill): first type will require a type hint + break; + case BuiltinProc_DIRECTIVE: { ast_node(bd, BasicDirective, ce->proc); String name = bd->name.string; @@ -3198,10 +3229,12 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 break; - case BuiltinProc_atomic_fence: - case BuiltinProc_atomic_fence_acq: - case BuiltinProc_atomic_fence_rel: - case BuiltinProc_atomic_fence_acqrel: + + case BuiltinProc_atomic_thread_fence: + case BuiltinProc_atomic_signal_fence: + if (!check_atomic_memory_order_argument(c, ce->args[0], builtin_name)) { + return false; + } operand->mode = Addressing_NoValue; break; @@ -3210,9 +3243,6 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 case BuiltinProc_unaligned_store: /*fallthrough*/ case BuiltinProc_atomic_store: - case BuiltinProc_atomic_store_rel: - case BuiltinProc_atomic_store_relaxed: - case BuiltinProc_atomic_store_unordered: { Type *elem = nullptr; if (!is_type_normal_pointer(operand->type, &elem)) { @@ -3228,14 +3258,32 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 break; } + case BuiltinProc_atomic_store_explicit: + { + Type *elem = nullptr; + if (!is_type_normal_pointer(operand->type, &elem)) { + error(operand->expr, "Expected a pointer for '%.*s'", LIT(builtin_name)); + return false; + } + Operand x = {}; + check_expr_with_type_hint(c, &x, ce->args[1], elem); + check_assignment(c, &x, elem, builtin_name); + + if (!check_atomic_memory_order_argument(c, ce->args[2], builtin_name)) { + return false; + } + + operand->type = nullptr; + operand->mode = Addressing_NoValue; + break; + } + + case BuiltinProc_volatile_load: /*fallthrough*/ case BuiltinProc_unaligned_load: /*fallthrough*/ case BuiltinProc_atomic_load: - case BuiltinProc_atomic_load_acq: - case BuiltinProc_atomic_load_relaxed: - case BuiltinProc_atomic_load_unordered: { Type *elem = nullptr; if (!is_type_normal_pointer(operand->type, &elem)) { @@ -3247,41 +3295,52 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 break; } + case BuiltinProc_atomic_load_explicit: + { + Type *elem = nullptr; + if (!is_type_normal_pointer(operand->type, &elem)) { + error(operand->expr, "Expected a pointer for '%.*s'", LIT(builtin_name)); + return false; + } + + if (!check_atomic_memory_order_argument(c, ce->args[1], builtin_name)) { + return false; + } + + operand->type = elem; + operand->mode = Addressing_Value; + break; + } + case BuiltinProc_atomic_add: - case BuiltinProc_atomic_add_acq: - case BuiltinProc_atomic_add_rel: - case BuiltinProc_atomic_add_acqrel: - case BuiltinProc_atomic_add_relaxed: case BuiltinProc_atomic_sub: - case BuiltinProc_atomic_sub_acq: - case BuiltinProc_atomic_sub_rel: - case BuiltinProc_atomic_sub_acqrel: - case BuiltinProc_atomic_sub_relaxed: case BuiltinProc_atomic_and: - case BuiltinProc_atomic_and_acq: - case BuiltinProc_atomic_and_rel: - case BuiltinProc_atomic_and_acqrel: - case BuiltinProc_atomic_and_relaxed: case BuiltinProc_atomic_nand: - case BuiltinProc_atomic_nand_acq: - case BuiltinProc_atomic_nand_rel: - case BuiltinProc_atomic_nand_acqrel: - case BuiltinProc_atomic_nand_relaxed: case BuiltinProc_atomic_or: - case BuiltinProc_atomic_or_acq: - case BuiltinProc_atomic_or_rel: - case BuiltinProc_atomic_or_acqrel: - case BuiltinProc_atomic_or_relaxed: case BuiltinProc_atomic_xor: - case BuiltinProc_atomic_xor_acq: - case BuiltinProc_atomic_xor_rel: - case BuiltinProc_atomic_xor_acqrel: - case BuiltinProc_atomic_xor_relaxed: - case BuiltinProc_atomic_xchg: - case BuiltinProc_atomic_xchg_acq: - case BuiltinProc_atomic_xchg_rel: - case BuiltinProc_atomic_xchg_acqrel: - case BuiltinProc_atomic_xchg_relaxed: + case BuiltinProc_atomic_exchange: + { + Type *elem = nullptr; + if (!is_type_normal_pointer(operand->type, &elem)) { + error(operand->expr, "Expected a pointer for '%.*s'", LIT(builtin_name)); + return false; + } + Operand x = {}; + check_expr_with_type_hint(c, &x, ce->args[1], elem); + check_assignment(c, &x, elem, builtin_name); + + operand->type = elem; + operand->mode = Addressing_Value; + break; + } + + case BuiltinProc_atomic_add_explicit: + case BuiltinProc_atomic_sub_explicit: + case BuiltinProc_atomic_and_explicit: + case BuiltinProc_atomic_nand_explicit: + case BuiltinProc_atomic_or_explicit: + case BuiltinProc_atomic_xor_explicit: + case BuiltinProc_atomic_exchange_explicit: { Type *elem = nullptr; if (!is_type_normal_pointer(operand->type, &elem)) { @@ -3292,30 +3351,18 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 check_expr_with_type_hint(c, &x, ce->args[1], elem); check_assignment(c, &x, elem, builtin_name); + + if (!check_atomic_memory_order_argument(c, ce->args[2], builtin_name)) { + return false; + } + operand->type = elem; operand->mode = Addressing_Value; break; } - case BuiltinProc_atomic_cxchg: - case BuiltinProc_atomic_cxchg_acq: - case BuiltinProc_atomic_cxchg_rel: - case BuiltinProc_atomic_cxchg_acqrel: - case BuiltinProc_atomic_cxchg_relaxed: - case BuiltinProc_atomic_cxchg_failrelaxed: - case BuiltinProc_atomic_cxchg_failacq: - case BuiltinProc_atomic_cxchg_acq_failrelaxed: - case BuiltinProc_atomic_cxchg_acqrel_failrelaxed: - - case BuiltinProc_atomic_cxchgweak: - case BuiltinProc_atomic_cxchgweak_acq: - case BuiltinProc_atomic_cxchgweak_rel: - case BuiltinProc_atomic_cxchgweak_acqrel: - case BuiltinProc_atomic_cxchgweak_relaxed: - case BuiltinProc_atomic_cxchgweak_failrelaxed: - case BuiltinProc_atomic_cxchgweak_failacq: - case BuiltinProc_atomic_cxchgweak_acq_failrelaxed: - case BuiltinProc_atomic_cxchgweak_acqrel_failrelaxed: + case BuiltinProc_atomic_compare_exchange_strong: + case BuiltinProc_atomic_compare_exchange_weak: { Type *elem = nullptr; if (!is_type_normal_pointer(operand->type, &elem)) { @@ -3333,7 +3380,33 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 operand->type = elem; break; } - break; + + case BuiltinProc_atomic_compare_exchange_strong_explicit: + case BuiltinProc_atomic_compare_exchange_weak_explicit: + { + Type *elem = nullptr; + if (!is_type_normal_pointer(operand->type, &elem)) { + error(operand->expr, "Expected a pointer for '%.*s'", LIT(builtin_name)); + return false; + } + Operand x = {}; + Operand y = {}; + check_expr_with_type_hint(c, &x, ce->args[1], elem); + check_expr_with_type_hint(c, &y, ce->args[2], elem); + check_assignment(c, &x, elem, builtin_name); + check_assignment(c, &y, elem, builtin_name); + + if (!check_atomic_memory_order_argument(c, ce->args[3], builtin_name, "success ordering")) { + return false; + } + if (!check_atomic_memory_order_argument(c, ce->args[4], builtin_name, "failure ordering")) { + return false; + } + + operand->mode = Addressing_OptionalOk; + operand->type = elem; + break; + } case BuiltinProc_fixed_point_mul: case BuiltinProc_fixed_point_div: -- cgit v1.2.3 From 6bc0c611abf9426815207d06c8f1d922533b278b Mon Sep 17 00:00:00 2001 From: gingerBill Date: Thu, 31 Mar 2022 00:49:53 +0100 Subject: Enforce success failure pairings of `compare_exchange_*_explicit` at compile time --- core/sync/primitives_atomic.odin | 2 +- src/check_builtin.cpp | 92 +++++++++++++++++++++++++++++++++++++--- src/types.cpp | 13 +++++- 3 files changed, 97 insertions(+), 10 deletions(-) (limited to 'src/check_builtin.cpp') diff --git a/core/sync/primitives_atomic.odin b/core/sync/primitives_atomic.odin index 8cec36602..dd432745f 100644 --- a/core/sync/primitives_atomic.odin +++ b/core/sync/primitives_atomic.odin @@ -77,7 +77,7 @@ atomic_mutex_unlock :: proc(m: ^Atomic_Mutex) { // atomic_mutex_try_lock tries to lock m, will return true on success, and false on failure atomic_mutex_try_lock :: proc(m: ^Atomic_Mutex) -> bool { - _, ok := atomic_compare_exchange_strong_explicit(&m.state, .Unlocked, .Locked, .acquire, .acquire) + _, ok := atomic_compare_exchange_strong_explicit(&m.state, .Unlocked, .Locked, .acquire, .consume) return ok } diff --git a/src/check_builtin.cpp b/src/check_builtin.cpp index 3fe0f1048..a82577b31 100644 --- a/src/check_builtin.cpp +++ b/src/check_builtin.cpp @@ -379,7 +379,7 @@ bool check_builtin_objc_procedure(CheckerContext *c, Operand *operand, Ast *call } } -bool check_atomic_memory_order_argument(CheckerContext *c, Ast *expr, String const &builtin_name, char const *extra_message = nullptr) { +bool check_atomic_memory_order_argument(CheckerContext *c, Ast *expr, String const &builtin_name, OdinAtomicMemoryOrder *memory_order_, char const *extra_message = nullptr) { Operand x = {}; check_expr_with_type_hint(c, &x, expr, t_atomic_memory_order); if (x.mode == Addressing_Invalid) { @@ -400,6 +400,9 @@ bool check_atomic_memory_order_argument(CheckerContext *c, Ast *expr, String con error(x.expr, "Illegal Atomic_Memory_Order value, got %lld", cast(long long)value); return false; } + if (memory_order_) { + *memory_order_ = cast(OdinAtomicMemoryOrder)value; + } return true; @@ -3232,7 +3235,7 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 case BuiltinProc_atomic_thread_fence: case BuiltinProc_atomic_signal_fence: - if (!check_atomic_memory_order_argument(c, ce->args[0], builtin_name)) { + if (!check_atomic_memory_order_argument(c, ce->args[0], builtin_name, nullptr)) { return false; } operand->mode = Addressing_NoValue; @@ -3269,9 +3272,17 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 check_expr_with_type_hint(c, &x, ce->args[1], elem); check_assignment(c, &x, elem, builtin_name); - if (!check_atomic_memory_order_argument(c, ce->args[2], builtin_name)) { + OdinAtomicMemoryOrder memory_order = {}; + if (!check_atomic_memory_order_argument(c, ce->args[2], builtin_name, &memory_order)) { return false; } + switch (memory_order) { + case OdinAtomicMemoryOrder_consume: + case OdinAtomicMemoryOrder_acquire: + case OdinAtomicMemoryOrder_acq_rel: + error(ce->args[2], "Illegal memory order .%s for %.*s", OdinAtomicMemoryOrder_strings[memory_order], LIT(builtin_name)); + break; + } operand->type = nullptr; operand->mode = Addressing_NoValue; @@ -3303,10 +3314,18 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 return false; } - if (!check_atomic_memory_order_argument(c, ce->args[1], builtin_name)) { + OdinAtomicMemoryOrder memory_order = {}; + if (!check_atomic_memory_order_argument(c, ce->args[1], builtin_name, &memory_order)) { return false; } + switch (memory_order) { + case OdinAtomicMemoryOrder_release: + case OdinAtomicMemoryOrder_acq_rel: + error(ce->args[1], "Illegal memory order .%s for %.*s", OdinAtomicMemoryOrder_strings[memory_order], LIT(builtin_name)); + break; + } + operand->type = elem; operand->mode = Addressing_Value; break; @@ -3352,7 +3371,7 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 check_assignment(c, &x, elem, builtin_name); - if (!check_atomic_memory_order_argument(c, ce->args[2], builtin_name)) { + if (!check_atomic_memory_order_argument(c, ce->args[2], builtin_name, nullptr)) { return false; } @@ -3396,13 +3415,72 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 check_assignment(c, &x, elem, builtin_name); check_assignment(c, &y, elem, builtin_name); - if (!check_atomic_memory_order_argument(c, ce->args[3], builtin_name, "success ordering")) { + OdinAtomicMemoryOrder success_memory_order = {}; + OdinAtomicMemoryOrder failure_memory_order = {}; + if (!check_atomic_memory_order_argument(c, ce->args[3], builtin_name, &success_memory_order, "success ordering")) { return false; } - if (!check_atomic_memory_order_argument(c, ce->args[4], builtin_name, "failure ordering")) { + if (!check_atomic_memory_order_argument(c, ce->args[4], builtin_name, &failure_memory_order, "failure ordering")) { return false; } + bool invalid_combination = false; + + switch (success_memory_order) { + case OdinAtomicMemoryOrder_relaxed: + case OdinAtomicMemoryOrder_release: + if (failure_memory_order != OdinAtomicMemoryOrder_relaxed) { + invalid_combination = true; + } + break; + case OdinAtomicMemoryOrder_consume: + switch (failure_memory_order) { + case OdinAtomicMemoryOrder_relaxed: + case OdinAtomicMemoryOrder_consume: + break; + default: + invalid_combination = true; + break; + } + break; + case OdinAtomicMemoryOrder_acquire: + case OdinAtomicMemoryOrder_acq_rel: + switch (failure_memory_order) { + case OdinAtomicMemoryOrder_relaxed: + case OdinAtomicMemoryOrder_consume: + case OdinAtomicMemoryOrder_acquire: + break; + default: + invalid_combination = true; + break; + } + break; + case OdinAtomicMemoryOrder_seq_cst: + switch (failure_memory_order) { + case OdinAtomicMemoryOrder_relaxed: + case OdinAtomicMemoryOrder_consume: + case OdinAtomicMemoryOrder_acquire: + case OdinAtomicMemoryOrder_seq_cst: + break; + default: + invalid_combination = true; + break; + } + break; + default: + invalid_combination = true; + break; + } + + + if (invalid_combination) { + error(ce->args[3], "Illegal memory order pairing for %.*s, success = .%s, failure = .%s", + LIT(builtin_name), + OdinAtomicMemoryOrder_strings[success_memory_order], + OdinAtomicMemoryOrder_strings[failure_memory_order] + ); + } + operand->mode = Addressing_OptionalOk; operand->type = elem; break; diff --git a/src/types.cpp b/src/types.cpp index 25e29820c..16d07d392 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -693,8 +693,8 @@ gb_global Type *t_objc_SEL = nullptr; gb_global Type *t_objc_Class = nullptr; enum OdinAtomicMemoryOrder : i32 { - OdinAtomicMemoryOrder_relaxed = 0, - OdinAtomicMemoryOrder_consume = 1, + OdinAtomicMemoryOrder_relaxed = 0, // unordered + OdinAtomicMemoryOrder_consume = 1, // monotonic OdinAtomicMemoryOrder_acquire = 2, OdinAtomicMemoryOrder_release = 3, OdinAtomicMemoryOrder_acq_rel = 4, @@ -702,6 +702,15 @@ enum OdinAtomicMemoryOrder : i32 { OdinAtomicMemoryOrder_COUNT, }; +char const *OdinAtomicMemoryOrder_strings[OdinAtomicMemoryOrder_COUNT] = { + "relaxed", + "consume", + "acquire", + "release", + "acq_rel", + "seq_cst", +}; + gb_global Type *t_atomic_memory_order = nullptr; -- cgit v1.2.3 From 1eac3482a68b66978a62fd397e0b9d53ca32d053 Mon Sep 17 00:00:00 2001 From: gingerBill Date: Thu, 31 Mar 2022 01:01:51 +0100 Subject: Add checks for memory ordering on fences --- core/c/libc/stdatomic.odin | 12 ++++++------ src/check_builtin.cpp | 26 ++++++++++++++++++++------ 2 files changed, 26 insertions(+), 12 deletions(-) (limited to 'src/check_builtin.cpp') diff --git a/core/c/libc/stdatomic.odin b/core/c/libc/stdatomic.odin index a5dd78afc..26ea0db5e 100644 --- a/core/c/libc/stdatomic.odin +++ b/core/c/libc/stdatomic.odin @@ -47,9 +47,9 @@ kill_dependency :: #force_inline proc(value: $T) -> T { // 7.17.4 Fences atomic_thread_fence :: #force_inline proc(order: memory_order) { - switch order { - case .relaxed: intrinsics.atomic_thread_fence(.relaxed) - case .consume: intrinsics.atomic_thread_fence(.consume) + assert(order != .relaxed) + assert(order != .consume) + #partial switch order { case .acquire: intrinsics.atomic_thread_fence(.acquire) case .release: intrinsics.atomic_thread_fence(.release) case .acq_rel: intrinsics.atomic_thread_fence(.acq_rel) @@ -58,9 +58,9 @@ atomic_thread_fence :: #force_inline proc(order: memory_order) { } atomic_signal_fence :: #force_inline proc(order: memory_order) { - switch order { - case .relaxed: intrinsics.atomic_signal_fence(.relaxed) - case .consume: intrinsics.atomic_signal_fence(.consume) + assert(order != .relaxed) + assert(order != .consume) + #partial switch order { case .acquire: intrinsics.atomic_signal_fence(.acquire) case .release: intrinsics.atomic_signal_fence(.release) case .acq_rel: intrinsics.atomic_signal_fence(.acq_rel) diff --git a/src/check_builtin.cpp b/src/check_builtin.cpp index a82577b31..8b8814176 100644 --- a/src/check_builtin.cpp +++ b/src/check_builtin.cpp @@ -3235,10 +3235,24 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 case BuiltinProc_atomic_thread_fence: case BuiltinProc_atomic_signal_fence: - if (!check_atomic_memory_order_argument(c, ce->args[0], builtin_name, nullptr)) { - return false; + { + OdinAtomicMemoryOrder memory_order = {}; + if (!check_atomic_memory_order_argument(c, ce->args[0], builtin_name, &memory_order)) { + return false; + } + switch (memory_order) { + case OdinAtomicMemoryOrder_acquire: + case OdinAtomicMemoryOrder_release: + case OdinAtomicMemoryOrder_acq_rel: + case OdinAtomicMemoryOrder_seq_cst: + break; + default: + error(ce->args[0], "Illegal memory ordering for '%.*s', got .%s", LIT(builtin_name), OdinAtomicMemoryOrder_strings[memory_order]); + break; + } + + operand->mode = Addressing_NoValue; } - operand->mode = Addressing_NoValue; break; case BuiltinProc_volatile_store: @@ -3280,7 +3294,7 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 case OdinAtomicMemoryOrder_consume: case OdinAtomicMemoryOrder_acquire: case OdinAtomicMemoryOrder_acq_rel: - error(ce->args[2], "Illegal memory order .%s for %.*s", OdinAtomicMemoryOrder_strings[memory_order], LIT(builtin_name)); + error(ce->args[2], "Illegal memory order .%s for '%.*s'", OdinAtomicMemoryOrder_strings[memory_order], LIT(builtin_name)); break; } @@ -3322,7 +3336,7 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 switch (memory_order) { case OdinAtomicMemoryOrder_release: case OdinAtomicMemoryOrder_acq_rel: - error(ce->args[1], "Illegal memory order .%s for %.*s", OdinAtomicMemoryOrder_strings[memory_order], LIT(builtin_name)); + error(ce->args[1], "Illegal memory order .%s for '%.*s'", OdinAtomicMemoryOrder_strings[memory_order], LIT(builtin_name)); break; } @@ -3474,7 +3488,7 @@ bool check_builtin_procedure(CheckerContext *c, Operand *operand, Ast *call, i32 if (invalid_combination) { - error(ce->args[3], "Illegal memory order pairing for %.*s, success = .%s, failure = .%s", + error(ce->args[3], "Illegal memory order pairing for '%.*s', success = .%s, failure = .%s", LIT(builtin_name), OdinAtomicMemoryOrder_strings[success_memory_order], OdinAtomicMemoryOrder_strings[failure_memory_order] -- cgit v1.2.3