Skip to content

Commit 7fbb72b

Browse files
committed
[mlir] DistinctAttributeAllocator: fix race
The bitfields added in #128566 (`threadingIsEnabled : 1` and `useThreadLocalAllocator : 1`) are accessed without synchronization. Example tsan report: ``` WARNING: ThreadSanitizer: data race (pid=337) Write of size 1 at 0x7260001d0ff0 by thread T224: #0 disableThreadLocalStorage third_party/llvm/llvm-project/mlir/lib/IR/AttributeDetail.h:434:29 #1 mlir::MLIRContext::disableThreadLocalStorage(bool) third_party/llvm/llvm-project/mlir/lib/IR/MLIRContext.cpp:723:40 #2 mlir::PassManager::runWithCrashRecovery(mlir::Operation*, mlir::AnalysisManager) third_party/llvm/llvm-project/mlir/lib/Pass/PassCrashRecovery.cpp:423:8 [...] Previous write of size 1 at 0x7260001d0ff0 by thread T222: #0 disableThreadLocalStorage third_party/llvm/llvm-project/mlir/lib/IR/AttributeDetail.h:434:29 #1 mlir::MLIRContext::disableThreadLocalStorage(bool) third_party/llvm/llvm-project/mlir/lib/IR/MLIRContext.cpp:723:40 #2 mlir::PassManager::runWithCrashRecovery(mlir::Operation*, mlir::AnalysisManager) third_party/llvm/llvm-project/mlir/lib/Pass/PassCrashRecovery.cpp:423:8 ``` Fix the race by serializing accesses to these fields with the existing `allocatorMutex`.
1 parent b246943 commit 7fbb72b

File tree

1 file changed

+12
-6
lines changed

1 file changed

+12
-6
lines changed

mlir/lib/IR/AttributeDetail.h

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -413,16 +413,17 @@ class DistinctAttributeAllocator {
413413
/// Allocates a distinct attribute storage using a thread local bump pointer
414414
/// allocator to enable synchronization free parallel allocations.
415415
DistinctAttrStorage *allocate(Attribute referencedAttr) {
416+
std::unique_lock<std::mutex> lock(allocatorMutex);
416417
if (!useThreadLocalAllocator && threadingIsEnabled) {
417-
std::scoped_lock<std::mutex> lock(allocatorMutex);
418-
return allocateImpl(referencedAttr);
418+
return allocateImpl(referencedAttr, lock);
419419
}
420-
return allocateImpl(referencedAttr);
420+
return allocateImpl(referencedAttr, lock);
421421
}
422422

423423
/// Sets a flag that stores if multithreading is enabled. The flag is used to
424424
/// decide if locking is needed when using a non thread-safe allocator.
425425
void disableMultiThreading(bool disable = true) {
426+
std::scoped_lock<std::mutex> lock(allocatorMutex);
426427
threadingIsEnabled = !disable;
427428
}
428429

@@ -431,20 +432,25 @@ class DistinctAttributeAllocator {
431432
/// beyond the lifetime of a child thread calling this function while ensuring
432433
/// thread-safe allocation.
433434
void disableThreadLocalStorage(bool disable = true) {
435+
std::scoped_lock<std::mutex> lock(allocatorMutex);
434436
useThreadLocalAllocator = !disable;
435437
}
436438

437439
private:
438-
DistinctAttrStorage *allocateImpl(Attribute referencedAttr) {
439-
return new (getAllocatorInUse().Allocate<DistinctAttrStorage>())
440+
DistinctAttrStorage *allocateImpl(Attribute referencedAttr,
441+
const std::unique_lock<std::mutex>& lock) {
442+
assert(lock.owns_lock());
443+
return new (getAllocatorInUse(lock).Allocate<DistinctAttrStorage>())
440444
DistinctAttrStorage(referencedAttr);
441445
}
442446

443447
/// If threading is disabled on the owning MLIR context, a normal non
444448
/// thread-local, non-thread safe bump pointer allocator is used instead to
445449
/// prevent use-after-free errors whenever attribute storage created on a
446450
/// crash recover thread is accessed after the thread joins.
447-
llvm::BumpPtrAllocator &getAllocatorInUse() {
451+
llvm::BumpPtrAllocator &getAllocatorInUse(
452+
const std::unique_lock<std::mutex>& lock) {
453+
assert(lock.owns_lock());
448454
if (useThreadLocalAllocator)
449455
return allocatorCache.get();
450456
return allocator;

0 commit comments

Comments
 (0)