[Mlir-commits] [mlir] [mlir] Fix DistinctAttributeUniquer deleting attribute storage when crash reproduction is enabled (PR #128566)

[Emilio Cota]

cota wrote:

@abulavin this is causing TSAN failures in downstream projects that we have at Google.
The problem is that the bitfields added here (`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
```

An obvious fix is to also access the bit fields with `allocatorMutex` being locked. However from reading the thread above it seems that your intent was to avoid contention if possible. You *could* define an std::atomic<int> to keep these bits in there, but you would still have to refactor the code to only read the flag once per call to `allocate`.

Given that this is introducing a race, I'd normally revert the PR. However, since it is also fixing a crash, I'd suggest we apply the obvious, non-scalable fix first, then you can work on making this more performant. If that sounds good, I've uploaded #132935 for review.

https://github.com/llvm/llvm-project/pull/128566