From 94d298755a6036c3dbbf95aed3299e021c3fb9c8 Mon Sep 17 00:00:00 2001 From: gingerBill Date: Mon, 16 Aug 2021 15:33:26 +0100 Subject: [PATCH 1/5] Fix race condition when adding a dependency --- src/check_expr.cpp | 9 ++++++--- src/checker.cpp | 10 ++++++---- src/checker.hpp | 3 ++- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/check_expr.cpp b/src/check_expr.cpp index 30f44a59f..e0eb5a954 100644 --- a/src/check_expr.cpp +++ b/src/check_expr.cpp @@ -4117,20 +4117,23 @@ bool check_identifier_exists(Scope *s, Ast *node, bool nested = false, Scope **o } isize add_dependencies_from_unpacking(CheckerContext *c, Entity **lhs, isize lhs_count, isize tuple_index, isize tuple_count) { - if (lhs != nullptr) { + if (lhs != nullptr && c->decl != nullptr) { + mutex_lock(&c->info->deps_mutex); + for (isize j = 0; (tuple_index + j) < lhs_count && j < tuple_count; j++) { Entity *e = lhs[tuple_index + j]; if (e != nullptr) { DeclInfo *decl = decl_info_of_entity(e); if (decl != nullptr) { - c->decl = decl; // will be reset by the 'defer' any way for_array(k, decl->deps.entries) { Entity *dep = decl->deps.entries[k].ptr; - add_declaration_dependency(c, dep); // TODO(bill): Should this be here? + ptr_set_add(&c->decl->deps, dep); } } } } + + mutex_unlock(&c->info->deps_mutex); } return tuple_count; } diff --git a/src/checker.cpp b/src/checker.cpp index 79a60f896..11d706403 100644 --- a/src/checker.cpp +++ b/src/checker.cpp @@ -629,14 +629,16 @@ void check_scope_usage(Checker *c, Scope *scope) { } -void add_dependency(DeclInfo *d, Entity *e) { +void add_dependency(CheckerInfo *info, DeclInfo *d, Entity *e) { + mutex_lock(&info->deps_mutex); ptr_set_add(&d->deps, e); + mutex_unlock(&info->deps_mutex); } void add_type_info_dependency(DeclInfo *d, Type *type) { if (d == nullptr) { - // GB_ASSERT(type == t_invalid); return; } + // NOTE(bill): no mutex is required here because the only procedure calling it is wrapped in a mutex already ptr_set_add(&d->type_info_deps, type); } @@ -657,7 +659,7 @@ void add_package_dependency(CheckerContext *c, char const *package_name, char co e->flags |= EntityFlag_Used; GB_ASSERT_MSG(e != nullptr, "%s", name); GB_ASSERT(c->decl != nullptr); - ptr_set_add(&c->decl->deps, e); + add_dependency(c->info, c->decl, e); } void add_declaration_dependency(CheckerContext *c, Entity *e) { @@ -665,7 +667,7 @@ void add_declaration_dependency(CheckerContext *c, Entity *e) { return; } if (c->decl != nullptr) { - add_dependency(c->decl, e); + add_dependency(c->info, c->decl, e); } } diff --git a/src/checker.hpp b/src/checker.hpp index bf462de71..dea24ae53 100644 --- a/src/checker.hpp +++ b/src/checker.hpp @@ -300,7 +300,8 @@ struct CheckerInfo { BlockingMutex global_untyped_mutex; BlockingMutex builtin_mutex; - // NOT recursive & Only used at the end of `check_proc_body` + // NOT recursive & only used at the end of `check_proc_body` + // and in `add_dependency`. // This is a possible source of contention but probably not // too much of a problem in practice BlockingMutex deps_mutex; From df159dbae73cf983f061a57aee3c6774ce105351 Mon Sep 17 00:00:00 2001 From: gingerBill Date: Mon, 16 Aug 2021 15:48:54 +0100 Subject: [PATCH 2/5] Add some missing files to sync2 for linux and darwin --- core/sync/sync2/primitives_darwin.odin | 143 +++++++++++++++++++++++ core/sync/sync2/primitives_linux.odin | 15 +++ core/sync/sync2/primitives_pthreads.odin | 4 +- 3 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 core/sync/sync2/primitives_darwin.odin create mode 100644 core/sync/sync2/primitives_linux.odin diff --git a/core/sync/sync2/primitives_darwin.odin b/core/sync/sync2/primitives_darwin.odin new file mode 100644 index 000000000..29e00f028 --- /dev/null +++ b/core/sync/sync2/primitives_darwin.odin @@ -0,0 +1,143 @@ +//+build darwin +//+private +package sync2 + +import "core:time" +import "core:c" +import "intrinsics" + +foreign import pthread "System.framework" + +_current_thread_id :: proc "contextless" () -> int { + tid: u64; + // NOTE(Oskar): available from OSX 10.6 and iOS 3.2. + // For older versions there is `syscall(SYS_thread_selfid)`, but not really + // the same thing apparently. + foreign pthread { pthread_threadid_np :: proc "c" (rawptr, ^u64) -> c.int ---; } + pthread_threadid_np(nil, &tid); + return int(tid); +} + +foreign { + @(link_name="usleep") + _darwin_usleep :: proc "c" (us: uint) -> i32 --- + @(link_name="sched_yield") + _darwin_sched_yield :: proc "c" () -> i32 --- +} + +_atomic_try_wait_slow :: proc(ptr: ^u32, val: u32) { + history: uint = 10; + for { + // Exponential wait + _darwin_usleep(history >> 2); + history += history >> 2; + if history > (1 << 10) { + history = 1 << 10; + } + + if atomic_load(ptr) != val { + break; + } + } +} + +_atomic_wait :: proc(ptr: ^u32, val: u32) { + if intrinsics.expect(atomic_load(ptr) != val, true) { + return; + } + for i in 0..<16 { + if atomic_load(ptr) != val { + return; + } + if i < 12 { + intrinsics.cpu_relax(); + } else { + _darwin_sched_yield(); + } + } + + for val == atomic_load(ptr) { + _atomic_try_wait_slow(ptr, val); + } +} + + +_Mutex :: struct { + +} + +_mutex_lock :: proc(m: ^Mutex) { +} + +_mutex_unlock :: proc(m: ^Mutex) { +} + +_mutex_try_lock :: proc(m: ^Mutex) -> bool { + return false; +} + +_RW_Mutex :: struct { +} + +_rw_mutex_lock :: proc(rw: ^RW_Mutex) { +} + +_rw_mutex_unlock :: proc(rw: ^RW_Mutex) { +} + +_rw_mutex_try_lock :: proc(rw: ^RW_Mutex) -> bool { + return false; +} + +_rw_mutex_shared_lock :: proc(rw: ^RW_Mutex) { +} + +_rw_mutex_shared_unlock :: proc(rw: ^RW_Mutex) { +} + +_rw_mutex_try_shared_lock :: proc(rw: ^RW_Mutex) -> bool { + return false; +} + + +_Recursive_Mutex :: struct { +} + +_recursive_mutex_lock :: proc(m: ^Recursive_Mutex) { +} + +_recursive_mutex_unlock :: proc(m: ^Recursive_Mutex) { +} + +_recursive_mutex_try_lock :: proc(m: ^Recursive_Mutex) -> bool { + return false; +} + + + + +_Cond :: struct { +} + +_cond_wait :: proc(c: ^Cond, m: ^Mutex) { +} + +_cond_wait_with_timeout :: proc(c: ^Cond, m: ^Mutex, timeout: time.Duration) -> bool { + return false; +} + +_cond_signal :: proc(c: ^Cond) { +} + +_cond_broadcast :: proc(c: ^Cond) { +} + + +_Sema :: struct { +} + +_sema_wait :: proc(s: ^Sema) { +} + +_sema_post :: proc(s: ^Sema, count := 1) { +} diff --git a/core/sync/sync2/primitives_linux.odin b/core/sync/sync2/primitives_linux.odin new file mode 100644 index 000000000..0a5da0880 --- /dev/null +++ b/core/sync/sync2/primitives_linux.odin @@ -0,0 +1,15 @@ +//+build linux +//+private +package sync2 + +// TODO(bill): remove libc +foreign import libc "system:c" + +_current_thread_id :: proc "contextless" () -> int { + foreign libc { + syscall :: proc(number: i32, #c_vararg args: ..any) -> i32 --- + } + + SYS_GETTID :: 186; + return int(syscall(SYS_GETTID)); +} diff --git a/core/sync/sync2/primitives_pthreads.odin b/core/sync/sync2/primitives_pthreads.odin index d0484e09e..98270b739 100644 --- a/core/sync/sync2/primitives_pthreads.odin +++ b/core/sync/sync2/primitives_pthreads.odin @@ -139,7 +139,7 @@ _recursive_mutex_lock :: proc(m: ^Recursive_Mutex) { } _recursive_mutex_unlock :: proc(m: ^Recursive_Mutex) { - tid := runtime.current_thread_id(); + tid := _current_thread_id(); assert(tid == m.impl.owner); m.impl.recursion -= 1; recursion := m.impl.recursion; @@ -154,7 +154,7 @@ _recursive_mutex_unlock :: proc(m: ^Recursive_Mutex) { } _recursive_mutex_try_lock :: proc(m: ^Recursive_Mutex) -> bool { - tid := runtime.current_thread_id(); + tid := _current_thread_id(); if m.impl.owner == tid { return mutex_try_lock(&m.impl.mutex); } From 0051cd12e22d03b5442df98d82d9b8a586362082 Mon Sep 17 00:00:00 2001 From: gingerBill Date: Mon, 16 Aug 2021 16:30:49 +0100 Subject: [PATCH 3/5] Make flags atomic for `Entity` and `Type` --- src/check_expr.cpp | 2 +- src/entity.cpp | 4 ++-- src/types.cpp | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/check_expr.cpp b/src/check_expr.cpp index e0eb5a954..581021ac7 100644 --- a/src/check_expr.cpp +++ b/src/check_expr.cpp @@ -923,7 +923,7 @@ bool is_polymorphic_type_assignable(CheckerContext *c, Type *poly, Type *source, poly->kind = Type_EnumeratedArray; poly->cached_size = -1; poly->cached_align = -1; - poly->flags = source->flags; + poly->flags.exchange(source->flags); poly->failure = false; poly->EnumeratedArray.elem = source->EnumeratedArray.elem; poly->EnumeratedArray.index = source->EnumeratedArray.index; diff --git a/src/entity.cpp b/src/entity.cpp index e7b888365..fb65bc3a8 100644 --- a/src/entity.cpp +++ b/src/entity.cpp @@ -115,8 +115,8 @@ enum ProcedureOptimizationMode : u32 { struct Entity { EntityKind kind; u64 id; - u64 flags; - EntityState state; + std::atomic flags; + std::atomic state; Token token; Scope * scope; Type * type; diff --git a/src/types.cpp b/src/types.cpp index f497e9509..fd5bdbc6d 100644 --- a/src/types.cpp +++ b/src/types.cpp @@ -306,9 +306,9 @@ struct Type { }; // NOTE(bill): These need to be at the end to not affect the unionized data - i64 cached_size; - i64 cached_align; - u32 flags; // TypeFlag + std::atomic cached_size; + std::atomic cached_align; + std::atomic flags; // TypeFlag bool failure; }; From fce86ff3d58dd38a6343053a76186e58a977bd72 Mon Sep 17 00:00:00 2001 From: gingerBill Date: Mon, 16 Aug 2021 18:17:26 +0100 Subject: [PATCH 4/5] Correct struct tag bug --- src/check_type.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/check_type.cpp b/src/check_type.cpp index d2a216c93..7dbd72612 100644 --- a/src/check_type.cpp +++ b/src/check_type.cpp @@ -159,7 +159,12 @@ void check_struct_fields(CheckerContext *ctx, Ast *node, Array *fields Entity *field = alloc_entity_field(ctx->scope, name_token, type, is_using, field_src_index); add_entity(ctx, ctx->scope, name, field); array_add(fields, field); - array_add(tags, p->tag.string); + String tag = p->tag.string; + if (tag.len != 0 && !unquote_string(permanent_allocator(), &tag, 0, tag.text[0] == '`')) { + error(p->tag, "Invalid string literal"); + tag = {}; + } + array_add(tags, tag); field_src_index += 1; } From 9ab94650c80363568b2361c57ca203a5eebd2d8c Mon Sep 17 00:00:00 2001 From: gingerBill Date: Mon, 16 Aug 2021 18:21:58 +0100 Subject: [PATCH 5/5] Allow `+` in import paths --- src/parser.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/parser.cpp b/src/parser.cpp index 8f3ffb8cc..2db8384a7 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -5018,10 +5018,9 @@ AstPackage *try_add_import_path(Parser *p, String const &path, String const &rel gb_global Rune illegal_import_runes[] = { '"', '\'', '`', - // ' ', '\t', '\r', '\n', '\v', '\f', '\\', // NOTE(bill): Disallow windows style filepaths - '!', '$', '%', '^', '&', '*', '(', ')', '=', '+', + '!', '$', '%', '^', '&', '*', '(', ')', '=', '[', ']', '{', '}', ';', ':', // NOTE(bill): Disallow windows style absolute filepaths