authorgravatar for jahe788@gmail.comIntegratedQuantum <jahe788@gmail.com> 2023-01-19 15:57:29+01:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2023-01-19 16:57:29+02:00
logf38fd388f8446b62945df4d2bfbcff350955bbce
tree4020c7590990bfc418fd5a01be6e66f475ce1f1c
parent116b7708099846e9ad789514504341b29bcce99c
signaturebadge-question-mark Signed by PGP key 4AEE18F83AFDEB23

Mutex deadlock detection in debug

Add a debug implementation to Mutex that detects deadlocks caused by calling lock twice in a single thread.

2 files changed, 64 insertions(+), 22 deletions(-)

lib/std/Thread/Condition.zig+9-1
...@@ -161,12 +161,20 @@ const WindowsImpl = struct {...@@ -161,12 +161,20 @@ const WindowsImpl = struct {
161 }161 }
162 }162 }
163163
164 if (comptime builtin.mode == .Debug) {
165 // The internal state of the DebugMutex needs to be handled here as well.
166 mutex.impl.locking_thread.store(0, .Unordered);
167 }
164 const rc = os.windows.kernel32.SleepConditionVariableSRW(168 const rc = os.windows.kernel32.SleepConditionVariableSRW(
165 &self.condition,169 &self.condition,
166 &mutex.impl.srwlock,170 if (comptime builtin.mode == .Debug) &mutex.impl.impl.srwlock else &mutex.impl.srwlock,
167 timeout_ms,171 timeout_ms,
168 0, // the srwlock was assumed to acquired in exclusive mode not shared172 0, // the srwlock was assumed to acquired in exclusive mode not shared
169 );173 );
174 if (comptime builtin.mode == .Debug) {
175 // The internal state of the DebugMutex needs to be handled here as well.
176 mutex.impl.locking_thread.store(std.Thread.getCurrentId(), .Unordered);
177 }
170178
171 // Return error.Timeout if we know the timeout elapsed correctly.179 // Return error.Timeout if we know the timeout elapsed correctly.
172 if (rc == os.windows.FALSE) {180 if (rc == os.windows.FALSE) {
lib/std/Thread/Mutex.zig+55-21
...@@ -27,7 +27,8 @@ const os = std.os;...@@ -27,7 +27,8 @@ const os = std.os;
27const assert = std.debug.assert;27const assert = std.debug.assert;
28const testing = std.testing;28const testing = std.testing;
29const Atomic = std.atomic.Atomic;29const Atomic = std.atomic.Atomic;
30const Futex = std.Thread.Futex;30const Thread = std.Thread;
31const Futex = Thread.Futex;
3132
32impl: Impl = .{},33impl: Impl = .{},
3334
...@@ -51,7 +52,12 @@ pub fn unlock(self: *Mutex) void {...@@ -51,7 +52,12 @@ pub fn unlock(self: *Mutex) void {
51 self.impl.unlock();52 self.impl.unlock();
52}53}
5354
54const Impl = if (builtin.single_threaded)55const Impl = if (builtin.mode == .Debug and !builtin.single_threaded)
56 DebugImpl
57else
58 ReleaseImpl;
59
60const ReleaseImpl = if (builtin.single_threaded)
55 SingleThreadedImpl61 SingleThreadedImpl
56else if (builtin.os.tag == .windows)62else if (builtin.os.tag == .windows)
57 WindowsImpl63 WindowsImpl
...@@ -60,22 +66,50 @@ else if (builtin.os.tag.isDarwin())...@@ -60,22 +66,50 @@ else if (builtin.os.tag.isDarwin())
60else66else
61 FutexImpl;67 FutexImpl;
6268
69const DebugImpl = struct {
70 locking_thread: Atomic(Thread.Id) = Atomic(Thread.Id).init(0), // 0 means it's not locked.
71 impl: ReleaseImpl = .{},
72
73 inline fn tryLock(self: *@This()) bool {
74 const locking = self.impl.tryLock();
75 if (locking) {
76 self.locking_thread.store(Thread.getCurrentId(), .Unordered);
77 }
78 return locking;
79 }
80
81 inline fn lock(self: *@This()) void {
82 const current_id = Thread.getCurrentId();
83 if (self.locking_thread.load(.Unordered) == current_id and current_id != 0) {
84 @panic("Deadlock detected");
85 }
86 self.impl.lock();
87 self.locking_thread.store(current_id, .Unordered);
88 }
89
90 inline fn unlock(self: *@This()) void {
91 assert(self.locking_thread.load(.Unordered) == Thread.getCurrentId());
92 self.locking_thread.store(0, .Unordered);
93 self.impl.unlock();
94 }
95};
96
63const SingleThreadedImpl = struct {97const SingleThreadedImpl = struct {
64 is_locked: bool = false,98 is_locked: bool = false,
6599
66 fn tryLock(self: *Impl) bool {100 fn tryLock(self: *@This()) bool {
67 if (self.is_locked) return false;101 if (self.is_locked) return false;
68 self.is_locked = true;102 self.is_locked = true;
69 return true;103 return true;
70 }104 }
71105
72 fn lock(self: *Impl) void {106 fn lock(self: *@This()) void {
73 if (!self.tryLock()) {107 if (!self.tryLock()) {
74 unreachable; // deadlock detected108 unreachable; // deadlock detected
75 }109 }
76 }110 }
77111
78 fn unlock(self: *Impl) void {112 fn unlock(self: *@This()) void {
79 assert(self.is_locked);113 assert(self.is_locked);
80 self.is_locked = false;114 self.is_locked = false;
81 }115 }
...@@ -86,15 +120,15 @@ const SingleThreadedImpl = struct {...@@ -86,15 +120,15 @@ const SingleThreadedImpl = struct {
86const WindowsImpl = struct {120const WindowsImpl = struct {
87 srwlock: os.windows.SRWLOCK = .{},121 srwlock: os.windows.SRWLOCK = .{},
88122
89 fn tryLock(self: *Impl) bool {123 fn tryLock(self: *@This()) bool {
90 return os.windows.kernel32.TryAcquireSRWLockExclusive(&self.srwlock) != os.windows.FALSE;124 return os.windows.kernel32.TryAcquireSRWLockExclusive(&self.srwlock) != os.windows.FALSE;
91 }125 }
92126
93 fn lock(self: *Impl) void {127 fn lock(self: *@This()) void {
94 os.windows.kernel32.AcquireSRWLockExclusive(&self.srwlock);128 os.windows.kernel32.AcquireSRWLockExclusive(&self.srwlock);
95 }129 }
96130
97 fn unlock(self: *Impl) void {131 fn unlock(self: *@This()) void {
98 os.windows.kernel32.ReleaseSRWLockExclusive(&self.srwlock);132 os.windows.kernel32.ReleaseSRWLockExclusive(&self.srwlock);
99 }133 }
100};134};
...@@ -103,15 +137,15 @@ const WindowsImpl = struct {...@@ -103,15 +137,15 @@ const WindowsImpl = struct {
103const DarwinImpl = struct {137const DarwinImpl = struct {
104 oul: os.darwin.os_unfair_lock = .{},138 oul: os.darwin.os_unfair_lock = .{},
105139
106 fn tryLock(self: *Impl) bool {140 fn tryLock(self: *@This()) bool {
107 return os.darwin.os_unfair_lock_trylock(&self.oul);141 return os.darwin.os_unfair_lock_trylock(&self.oul);
108 }142 }
109143
110 fn lock(self: *Impl) void {144 fn lock(self: *@This()) void {
111 os.darwin.os_unfair_lock_lock(&self.oul);145 os.darwin.os_unfair_lock_lock(&self.oul);
112 }146 }
113147
114 fn unlock(self: *Impl) void {148 fn unlock(self: *@This()) void {
115 os.darwin.os_unfair_lock_unlock(&self.oul);149 os.darwin.os_unfair_lock_unlock(&self.oul);
116 }150 }
117};151};
...@@ -123,19 +157,19 @@ const FutexImpl = struct {...@@ -123,19 +157,19 @@ const FutexImpl = struct {
123 const locked = 0b01;157 const locked = 0b01;
124 const contended = 0b11; // must contain the `locked` bit for x86 optimization below158 const contended = 0b11; // must contain the `locked` bit for x86 optimization below
125159
126 fn tryLock(self: *Impl) bool {160 fn tryLock(self: *@This()) bool {
127 // Lock with compareAndSwap instead of tryCompareAndSwap to avoid reporting spurious CAS failure.161 // Lock with compareAndSwap instead of tryCompareAndSwap to avoid reporting spurious CAS failure.
128 return self.lockFast("compareAndSwap");162 return self.lockFast("compareAndSwap");
129 }163 }
130164
131 fn lock(self: *Impl) void {165 fn lock(self: *@This()) void {
132 // Lock with tryCompareAndSwap instead of compareAndSwap due to being more inline-able on LL/SC archs like ARM.166 // Lock with tryCompareAndSwap instead of compareAndSwap due to being more inline-able on LL/SC archs like ARM.
133 if (!self.lockFast("tryCompareAndSwap")) {167 if (!self.lockFast("tryCompareAndSwap")) {
134 self.lockSlow();168 self.lockSlow();
135 }169 }
136 }170 }
137171
138 inline fn lockFast(self: *Impl, comptime casFn: []const u8) bool {172 inline fn lockFast(self: *@This(), comptime casFn: []const u8) bool {
139 // On x86, use `lock bts` instead of `lock cmpxchg` as:173 // On x86, use `lock bts` instead of `lock cmpxchg` as:
140 // - they both seem to mark the cache-line as modified regardless: https://stackoverflow.com/a/63350048174 // - they both seem to mark the cache-line as modified regardless: https://stackoverflow.com/a/63350048
141 // - `lock bts` is smaller instruction-wise which makes it better for inlining175 // - `lock bts` is smaller instruction-wise which makes it better for inlining
...@@ -149,7 +183,7 @@ const FutexImpl = struct {...@@ -149,7 +183,7 @@ const FutexImpl = struct {
149 return @field(self.state, casFn)(unlocked, locked, .Acquire, .Monotonic) == null;183 return @field(self.state, casFn)(unlocked, locked, .Acquire, .Monotonic) == null;
150 }184 }
151185
152 fn lockSlow(self: *Impl) void {186 fn lockSlow(self: *@This()) void {
153 @setCold(true);187 @setCold(true);
154188
155 // Avoid doing an atomic swap below if we already know the state is contended.189 // Avoid doing an atomic swap below if we already know the state is contended.
...@@ -172,7 +206,7 @@ const FutexImpl = struct {...@@ -172,7 +206,7 @@ const FutexImpl = struct {
172 }206 }
173 }207 }
174208
175 fn unlock(self: *Impl) void {209 fn unlock(self: *@This()) void {
176 // Unlock the mutex and wake up a waiting thread if any.210 // Unlock the mutex and wake up a waiting thread if any.
177 //211 //
178 // A waiting thread will acquire with `contended` instead of `locked`212 // A waiting thread will acquire with `contended` instead of `locked`
...@@ -228,7 +262,7 @@ test "Mutex - many uncontended" {...@@ -228,7 +262,7 @@ test "Mutex - many uncontended" {
228262
229 const Runner = struct {263 const Runner = struct {
230 mutex: Mutex = .{},264 mutex: Mutex = .{},
231 thread: std.Thread = undefined,265 thread: Thread = undefined,
232 counter: NonAtomicCounter = .{},266 counter: NonAtomicCounter = .{},
233267
234 fn run(self: *@This()) void {268 fn run(self: *@This()) void {
...@@ -243,7 +277,7 @@ test "Mutex - many uncontended" {...@@ -243,7 +277,7 @@ test "Mutex - many uncontended" {
243 };277 };
244278
245 var runners = [_]Runner{.{}} ** num_threads;279 var runners = [_]Runner{.{}} ** num_threads;
246 for (runners) |*r| r.thread = try std.Thread.spawn(.{}, Runner.run, .{r});280 for (runners) |*r| r.thread = try Thread.spawn(.{}, Runner.run, .{r});
247 for (runners) |r| r.thread.join();281 for (runners) |r| r.thread.join();
248 for (runners) |r| try testing.expectEqual(r.counter.get(), num_increments);282 for (runners) |r| try testing.expectEqual(r.counter.get(), num_increments);
249}283}
...@@ -265,7 +299,7 @@ test "Mutex - many contended" {...@@ -265,7 +299,7 @@ test "Mutex - many contended" {
265 var i: usize = num_increments;299 var i: usize = num_increments;
266 while (i > 0) : (i -= 1) {300 while (i > 0) : (i -= 1) {
267 // Occasionally hint to let another thread run.301 // Occasionally hint to let another thread run.
268 defer if (i % 100 == 0) std.Thread.yield() catch {};302 defer if (i % 100 == 0) Thread.yield() catch {};
269303
270 self.mutex.lock();304 self.mutex.lock();
271 defer self.mutex.unlock();305 defer self.mutex.unlock();
...@@ -277,8 +311,8 @@ test "Mutex - many contended" {...@@ -277,8 +311,8 @@ test "Mutex - many contended" {
277311
278 var runner = Runner{};312 var runner = Runner{};
279313
280 var threads: [num_threads]std.Thread = undefined;314 var threads: [num_threads]Thread = undefined;
281 for (threads) |*t| t.* = try std.Thread.spawn(.{}, Runner.run, .{&runner});315 for (threads) |*t| t.* = try Thread.spawn(.{}, Runner.run, .{&runner});
282 for (threads) |t| t.join();316 for (threads) |t| t.join();
283317
284 try testing.expectEqual(runner.counter.get(), num_increments * num_threads);318 try testing.expectEqual(runner.counter.get(), num_increments * num_threads);