From e9b54d9adac3c0624441fc406201d89bddccb4c2 Mon Sep 17 00:00:00 2001 From: Jaremy Creechley Date: Mon, 6 Jan 2025 04:29:04 -0700 Subject: [PATCH 1/2] make tsan happy with running(thr: Thread): bool --- lib/std/private/threadtypes.nim | 5 +++-- lib/std/typedthreads.nim | 12 +++++++----- lib/system/threadimpl.nim | 16 ++++++++++------ 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/lib/std/private/threadtypes.nim b/lib/std/private/threadtypes.nim index a1cdf21dc0f77..f68a939bb6617 100644 --- a/lib/std/private/threadtypes.nim +++ b/lib/std/private/threadtypes.nim @@ -1,4 +1,5 @@ include system/inclrtl +import std/atomics const hasSharedHeap* = defined(boehmgc) or defined(gogc) # don't share heaps; every thread has its own @@ -166,9 +167,9 @@ type core*: PGcThread sys*: SysThread when TArg is void: - dataFn*: proc () {.nimcall, gcsafe.} + dataFn*: Atomic[proc () {.nimcall, gcsafe.}] else: - dataFn*: proc (m: TArg) {.nimcall, gcsafe.} + dataFn*: Atomic[proc (m: TArg) {.nimcall, gcsafe.}] data*: TArg when hasAllocStack: rawStack*: pointer diff --git a/lib/std/typedthreads.nim b/lib/std/typedthreads.nim index 494baa8abf9f3..808ea04af632f 100644 --- a/lib/std/typedthreads.nim +++ b/lib/std/typedthreads.nim @@ -78,6 +78,7 @@ deinitLock(l) import std/private/[threadtypes] export Thread +import std/atomics import system/ansi_c @@ -151,7 +152,8 @@ else: proc running*[TArg](t: Thread[TArg]): bool {.inline.} = ## Returns true if `t` is running. - result = t.dataFn != nil + let dataFn = t.dataFn.load() + result = dataFn != nil proc handle*[TArg](t: Thread[TArg]): SysThread {.inline.} = ## Returns the thread handle of `t`. @@ -202,7 +204,7 @@ when false: else: discard pthread_cancel(t.sys) when declared(registerThread): unregisterThread(addr(t)) - t.dataFn = nil + t.dataFn.store nil ## if thread `t` already exited, `t.core` will be `null`. if not isNil(t.core): deallocThreadStorage(t.core) @@ -220,7 +222,7 @@ when hostOS == "windows": t.core = cast[PGcThread](allocThreadStorage(sizeof(GcThread))) when TArg isnot void: t.data = param - t.dataFn = tp + t.dataFn.store tp when hasSharedHeap: t.core.stackSize = ThreadStackSize var dummyThreadId: int32 = 0'i32 t.sys = createThread(nil, ThreadStackSize, threadProcWrapper[TArg], @@ -245,7 +247,7 @@ elif defined(genode): t.core = cast[PGcThread](allocThreadStorage(sizeof(GcThread))) when TArg isnot void: t.data = param - t.dataFn = tp + t.dataFn.store(tp) when hasSharedHeap: t.stackSize = ThreadStackSize t.sys.initThread( runtimeEnv, @@ -269,7 +271,7 @@ else: t.core = cast[PGcThread](allocThreadStorage(sizeof(GcThread))) when TArg isnot void: t.data = param - t.dataFn = tp + t.dataFn.store(tp) when hasSharedHeap: t.core.stackSize = ThreadStackSize var a {.noinit.}: Pthread_attr doAssert pthread_attr_init(a) == 0 diff --git a/lib/system/threadimpl.nim b/lib/system/threadimpl.nim index 093a920a1d536..95fb70f27b39e 100644 --- a/lib/system/threadimpl.nim +++ b/lib/system/threadimpl.nim @@ -1,3 +1,5 @@ +import std/atomics + var nimThreadDestructionHandlers* {.rtlThreadVar.}: seq[proc () {.closure, gcsafe, raises: [].}] when not defined(boehmgc) and not hasSharedHeap and not defined(gogc) and not defined(gcRegions): @@ -50,10 +52,11 @@ when defined(boehmgc): boehmGC_register_my_thread(sb) try: let thrd = cast[ptr Thread[TArg]](thrd) + let dataFn = thrd.dataFn.load() when TArg is void: - thrd.dataFn() + dataFn() else: - thrd.dataFn(thrd.data) + dataFn(thrd.data) except: threadTrouble() finally: @@ -62,15 +65,16 @@ when defined(boehmgc): else: proc threadProcWrapDispatch[TArg](thrd: ptr Thread[TArg]) {.raises: [].} = try: + let dataFn = thrd.dataFn.load() when TArg is void: - thrd.dataFn() + dataFn() else: when defined(nimV2): - thrd.dataFn(thrd.data) + dataFn(thrd.data) else: var x: TArg = default(TArg) deepCopy(x, thrd.data) - thrd.dataFn(x) + dataFn(x) except: threadTrouble() finally: @@ -107,5 +111,5 @@ template nimThreadProcWrapperBody*(closure: untyped): untyped = # mark as not running anymore: thrd.core = nil - thrd.dataFn = nil + thrd.dataFn.store(nil) deallocThreadStorage(cast[pointer](core)) From 2e8d39caa97649fa31e1390e431cf4212bb4af86 Mon Sep 17 00:00:00 2001 From: Jaremy Creechley Date: Wed, 8 Jan 2025 03:03:37 -0700 Subject: [PATCH 2/2] change running to take var, otherwise we might get a copy? --- lib/std/typedthreads.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/std/typedthreads.nim b/lib/std/typedthreads.nim index 808ea04af632f..51cf82374ffb9 100644 --- a/lib/std/typedthreads.nim +++ b/lib/std/typedthreads.nim @@ -150,7 +150,7 @@ else: nimThreadProcWrapperBody(closure) {.pop.} -proc running*[TArg](t: Thread[TArg]): bool {.inline.} = +proc running*[TArg](t: var Thread[TArg]): bool {.inline.} = ## Returns true if `t` is running. let dataFn = t.dataFn.load() result = dataFn != nil