Wipe ptls->system_id when a thread exits.#50747
Conversation
vtjnash
left a comment
There was a problem hiding this comment.
Very sketchy that you are doing this without atomics or a lock, but otherwise SGTM
|
Are you suggesting to make the field atomic? |
vtjnash
left a comment
There was a problem hiding this comment.
Now it still looks sketchy, since you now have a lot of unsynchronized uses of an an atomic variable. Many of them happen behind a lock, so that is fine. But signals-mach.c:392 looks like a data race now, since the current_task load is not an acquire load and is in the wrong order with this. All of the other places could probably use a brief comment nearby that they are either inside the lock, or in a region where we know the target thread for lookup is pinned by the OS and currently stopped.
Should I make them regular loads then?
So what should I change? |
|
@vtjnash Bump. |
On macOS, we use the system thread ID to match against the list of known thread local states during signal handling. To prevent picking up the wrong entry, i.e. from when a thread was previously executing a different task, make sure to wipe the system ID when a thread exits. This manifested as the signal handler actually reporting a bus error when a thread touched safepoint memory during GC, because the matched thread local state had no current task attached to it. Fixes JuliaGPU/Metal.jl#225
|
Let's close this in favor of #50871, which is much simpler. |
On macOS, we use the system thread ID to match against the list of known thread local states during signal handling. To prevent picking up the wrong entry, i.e. from when a thread was previously executing a different task, make sure to wipe the system ID when a thread exits.
This manifested as the signal handler actually reporting a bus error when a thread touched safepoint memory during GC, because the matched thread local state had no current task attached to it.
Fixes JuliaGPU/Metal.jl#225