mirror/Odin
1
0 Fork 0
mirror of https://github.com/odin-lang/Odin.git synced 2026-10-15 00:31:58 -04:00
Code Issues Packages Projects Releases Wiki Activity

Warn on atomics on a local whose address is only taken by them, suggesting volatile_load/volatile_store

Browse Source
This commit is contained in:
gingerBill committed 2026-10-08 10:09:06 +01:00
1 parent 8dee146bf3
commit 9db7f49441
1 file changed
+131
Download Patch File Download Diff File Expand all files Collapse all files
src/check_atomics.cpp
+131
View File
@@ -13,6 +13,8 @@
// matches; a futex woken before the procedure waking it writes it, as what waits on it may sleep again; and with an
// unpaired ordering, an `atomic_signal_fence` where an `atomic_thread_fence` would pair, as it only orders against a
// signal handler on the same thread.
// Atomics on a local whose address is only taken by them are warned about too, as nothing else can access it, so they
// order nothing; `volatile_*` is probably what is meant, to keep the accesses, or it is a copy of what is shared.
// With -vet-atomic-access, a plain read of a location accessed atomically is an error, unless a lock is taken.
struct AtomicUses {
@@ -40,6 +42,13 @@ struct AtomicProcedure {
PtrMap<Ast *, AtomicPlace> places; // of each call within it
};
struct AtomicLocal {
DeclInfo *decl;
i32 count;
i32 addressed;
Ast * first;
};
struct AtomicSite {
Entity * location;
Ast * call;
@@ -51,6 +60,7 @@ enum AtomicReportKind {
AtomicReport_Acquire, // read with acquire ordering, which nothing releases
AtomicReport_WeakIgnored, // a weak compare-exchange whose second result is not used
AtomicReport_WakeEarly, // a futex woken before it is written
AtomicReport_Local, // atomics on a local which nothing else can access
};
struct AtomicReport {
@@ -65,6 +75,8 @@ struct AtomicScan {
PtrMap<Ast *, AtomicPlace> *places; // NULL when not used
AtomicPlace place;
PtrMap<Entity *, AtomicLocal> *locals; // NULL when not used
};
gb_global PtrMap<Entity *, AtomicUses> atomic_uses;
@@ -132,6 +144,62 @@ gb_internal Entity *check_atomic_location(Ast *expr) {
}
}
gb_internal Entity *check_atomic_local(Ast *expr) {
for (;;) {
expr = unparen_expr(expr);
switch (expr->kind) {
case_ast_node(i, Ident, expr);
Entity *e = entity_of_node(expr);
if (e == nullptr || e->kind != Entity_Variable) {
return nullptr;
}
if (e->using_parent != nullptr) {
// a field brought in by `using`, within what it was applied to, unless that is a pointer
if (is_type_pointer(e->using_parent->type)) {
return nullptr;
}
e = e->using_parent;
}
if ((e->flags & EntityFlag_Param) != 0 || !is_entity_local_variable(e)) {
return nullptr;
}
return e;
case_end;
case_ast_node(se, SelectorExpr, expr);
if (is_type_pointer(se->expr->tav.type)) {
return nullptr;
}
expr = se->expr;
case_end;
case_ast_node(ie, IndexExpr, expr);
Type *t = base_type(ie->expr->tav.type);
if (t == nullptr || (t->kind != Type_Array && t->kind != Type_EnumeratedArray && t->kind != Type_FixedCapacityDynamicArray)) {
return nullptr;
}
expr = ie->expr;
case_end;
default:
return nullptr;
}
}
}
gb_internal void check_atomic_address_taken(AtomicScan *s, Ast *expr) {
// by `&`, slicing, or iterating by reference
// which a call like `x->f()` is too, as `&x` is its first argument
if (s->locals == nullptr) {
return;
}
if (Entity *l = check_atomic_local(expr)) {
if (AtomicLocal *local = map_get(s->locals, l)) {
local->addressed += 1;
}
}
}
gb_internal void check_atomic_first(Ast **site, Ast *call) {
if (*site == nullptr || token_pos_cmp(ast_token(call).pos, ast_token(*site).pos) < 0) {
*site = call;
@@ -266,6 +334,9 @@ gb_internal void check_atomic_scan(AtomicScan *s, Ast *node, bool addr) {
case_end;
case_ast_node(ue, UnaryExpr, node);
if (ue->op.kind == Token_And) {
check_atomic_address_taken(s, ue->expr);
}
check_atomic_scan(s, ue->expr, ue->op.kind == Token_And);
case_end;
@@ -308,6 +379,9 @@ gb_internal void check_atomic_scan(AtomicScan *s, Ast *node, bool addr) {
case_end;
case_ast_node(se, SliceExpr, node);
if (is_type_array_like(se->expr->tav.type)) {
check_atomic_address_taken(s, se->expr);
}
check_atomic_scan(s, se->expr, is_type_array_like(se->expr->tav.type));
check_atomic_scan(s, se->low, false);
check_atomic_scan(s, se->high, false);
@@ -336,6 +410,10 @@ gb_internal void check_atomic_scan(AtomicScan *s, Ast *node, bool addr) {
check_atomic_scan(s, sce->call, false);
case_end;
case_ast_node(el, Ellipsis, node);
check_atomic_scan(s, el->expr, false);
case_end;
case_ast_node(cl, CompoundLit, node);
for (Ast *elem : cl->elems) {
check_atomic_scan(s, elem, false);
@@ -460,6 +538,9 @@ gb_internal void check_atomic_scan(AtomicScan *s, Ast *node, bool addr) {
for (Ast *val : rs->vals) {
by_ref |= val->kind == Ast_UnaryExpr && val->UnaryExpr.op.kind == Token_And;
}
if (by_ref && is_type_array_like(rs->expr->tav.type)) {
check_atomic_address_taken(s, rs->expr);
}
check_atomic_scan(s, rs->expr, by_ref && is_type_array_like(rs->expr->tav.type));
check_atomic_scan(s, rs->body, false);
case_end;
@@ -470,6 +551,9 @@ gb_internal void check_atomic_scan(AtomicScan *s, Ast *node, bool addr) {
for (Ast *val : vals) {
by_ref |= val != nullptr && val->kind == Ast_UnaryExpr && val->UnaryExpr.op.kind == Token_And;
}
if (by_ref && is_type_array_like(rs->expr->tav.type)) {
check_atomic_address_taken(s, rs->expr);
}
check_atomic_scan(s, rs->init, false);
check_atomic_scan(s, rs->expr, by_ref && is_type_array_like(rs->expr->tav.type));
check_atomic_scan(s, rs->body, false);
@@ -628,6 +712,10 @@ gb_internal void check_atomics(Checker *c) {
ptr_set_init(&operated, atomics.count);
defer (ptr_set_destroy(&operated));
PtrMap<Entity *, AtomicLocal> locals = {};
map_init(&locals, 0);
defer (map_destroy(&locals));
for (CheckedAtomic const &a : atomics) {
if (a.id == BuiltinProc_atomic_thread_fence || a.id == BuiltinProc_atomic_signal_fence) {
continue;
@@ -638,6 +726,23 @@ gb_internal void check_atomics(Checker *c) {
continue;
}
ptr_set_add(&operated, ptr);
// NOTE(bill): this means it not a "true" futex
// as waiting with a timeout on what nothing wakes is deliberate
Entity *l = nullptr;
if (a.futex == ProcedureFutex_None) {
l = check_atomic_local(ptr->UnaryExpr.expr);
}
if (l != nullptr && a.decl != nullptr) {
AtomicLocal local = {};
local.decl = a.decl;
if (AtomicLocal *found = map_get(&locals, l)) {
local = *found;
}
local.count += 1;
check_atomic_first(&local.first, call);
map_set(&locals, l, local);
}
Entity *e = check_atomic_location(ptr->UnaryExpr.expr);
if (e == nullptr) {
continue;
@@ -832,6 +937,24 @@ gb_internal void check_atomics(Checker *c) {
}
}
PtrSet<DeclInfo *> walked = {};
for (auto const &entry : locals) {
DeclInfo *decl = entry.value.decl;
if (decl->proc_info == nullptr || ptr_set_update(&walked, decl)) {
continue;
}
AtomicScan s = {};
s.reads = array_make<Ast *>(temporary_allocator(), 0, 0);
s.locals = &locals;
check_atomic_scan(&s, decl->proc_info->body, false);
}
ptr_set_destroy(&walked);
for (auto const &entry : locals) {
if (entry.value.addressed == entry.value.count && ast_file_analysis(entry.value.first->file(), AnalysisFlag_Atomic)) {
array_add(&reports, AtomicReport{AtomicReport_Local, entry.value.first, nullptr});
}
}
// NOTE(bill): In order and once for what is at the same place in each instantiation of a polymorphic procedure
array_sort(reports, check_atomic_report_cmp);
for_array(i, reports) {
@@ -848,6 +971,13 @@ gb_internal void check_atomics(Checker *c) {
gb_string_free(name);
continue;
}
if (r.kind == AtomicReport_Local) {
Entity *l = check_atomic_local(check_atomic_address_of(r.site->CallExpr.args[0])->UnaryExpr.expr);
warning(r.site, "The address of '%.*s' is only taken by atomic operations, so nothing else can access it, and they order nothing", LIT(l->token.string));
error_line("\tSuggestion: To keep its accesses from being optimized away, use 'volatile_load' and 'volatile_store', which order nothing between threads\n");
error_line("\t If it is meant to be shared, it may be a copy of what is\n");
continue;
}
gbString str = expr_to_string(check_atomic_address_of(r.site->CallExpr.args[0])->UnaryExpr.expr);
char const *other = token_pos_to_string(ast_token(r.other).pos);
@@ -886,6 +1016,7 @@ gb_internal void check_atomics(Checker *c) {
error_line("\tSuggestion: Wake it after the write at %s\n", other);
break;
case AtomicReport_WeakIgnored:
case AtomicReport_Local:
break;
}
gb_string_free(str);