Skip to content

Commit 1e50a99

Browse files
authored
Utilize bitshifts correctly in signals-mach.c when storing/reading the previous GC state (#53868)
I have not succeed in writing a test for this, but this was found on a CI hang together with @Keno and @vtjnash. In essence if we hit a safepoint while GC_SAFE things can go wrong <img width="748" alt="image" src="https://github.com/JuliaLang/julia/assets/28694980/7d8170ee-11ab-43de-9bb1-9219aa5a2d80">
1 parent 3530c8f commit 1e50a99

File tree

1 file changed

+23
-5
lines changed

1 file changed

+23
-5
lines changed

src/signals-mach.c

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <mach/task.h>
1010
#include <mach/mig_errors.h>
1111
#include <AvailabilityMacros.h>
12+
#include <stdint.h>
1213
#include "mach_excServer.c"
1314

1415
#ifdef MAC_OS_X_VERSION_10_9
@@ -47,14 +48,31 @@ static arraylist_t suspended_threads;
4748
extern uv_mutex_t safepoint_lock;
4849
extern uv_cond_t safepoint_cond_begin;
4950

51+
#define GC_STATE_SHIFT 8*sizeof(int16_t)
52+
static inline int8_t decode_gc_state(uintptr_t item)
53+
{
54+
return (int8_t)(item >> GC_STATE_SHIFT);
55+
}
56+
57+
static inline int16_t decode_tid(uintptr_t item)
58+
{
59+
return (int16_t)item;
60+
}
61+
62+
static inline uintptr_t encode_item(int16_t tid, int8_t gc_state)
63+
{
64+
return (uintptr_t)tid | ((uintptr_t)gc_state << GC_STATE_SHIFT);
65+
}
66+
5067
// see jl_safepoint_wait_thread_resume
5168
void jl_safepoint_resume_thread_mach(jl_ptls_t ptls2, int16_t tid2)
5269
{
5370
// must be called with uv_mutex_lock(&safepoint_lock) and uv_mutex_lock(&ptls2->sleep_lock) held (in that order)
5471
for (size_t i = 0; i < suspended_threads.len; i++) {
5572
uintptr_t item = (uintptr_t)suspended_threads.items[i];
56-
int16_t tid = (int16_t)item;
57-
int8_t gc_state = (int8_t)(item >> 8);
73+
74+
int16_t tid = decode_tid(item);
75+
int8_t gc_state = decode_gc_state(item);
5876
if (tid != tid2)
5977
continue;
6078
jl_atomic_store_release(&ptls2->gc_state, gc_state);
@@ -71,8 +89,8 @@ void jl_mach_gc_end(void)
7189
size_t j = 0;
7290
for (size_t i = 0; i < suspended_threads.len; i++) {
7391
uintptr_t item = (uintptr_t)suspended_threads.items[i];
74-
int16_t tid = (int16_t)item;
75-
int8_t gc_state = (int8_t)(item >> 8);
92+
int16_t tid = decode_tid(item);
93+
int8_t gc_state = decode_gc_state(item);
7694
jl_ptls_t ptls2 = jl_atomic_load_relaxed(&jl_all_tls_states)[tid];
7795
uv_mutex_lock(&ptls2->sleep_lock);
7896
if (jl_atomic_load_relaxed(&ptls2->suspend_count) == 0) {
@@ -117,7 +135,7 @@ static void jl_mach_gc_wait(jl_ptls_t ptls2, mach_port_t thread, int16_t tid)
117135
// triggers a SIGSEGV and gets handled by the usual codepath for unix.
118136
int8_t gc_state = jl_atomic_load_acquire(&ptls2->gc_state);
119137
jl_atomic_store_release(&ptls2->gc_state, JL_GC_STATE_WAITING);
120-
uintptr_t item = tid | (((uintptr_t)gc_state) << 16);
138+
uintptr_t item = encode_item(tid, gc_state);
121139
arraylist_push(&suspended_threads, (void*)item);
122140
thread_suspend(thread);
123141
}

0 commit comments

Comments
 (0)