commit aa806c5cb792a9826ad5075f80c2ba15e1e8943d
author Lokbondo Kung <lokokung@google.com> Fri Nov 01 03:25:37 2024 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Nov 01 03:25:37 2024 +0000
tree abc0c08e6849806d4ac192d17a90f51544ee21f1
parent 0eee090c2b8adca4142c352b6668a9b72435c538

[dawn][native] Fixes TSAN race from GetCompilationInfo.

- The race happens when multiple threads Complete the
  CompilationInfoEvent. The call to
  OwnedCompilationMessages::GetCompilationInfo was writing
  to the owned copy of CompilationInfo in both threads. Note
  that in practice, the competing threads would have written
  the same things.
- Adds an atomic bool to gate writing to the internal
  CompilationInfo struct that should only ever be written
  once.
- Updates some of the asserts to use the new bool to be
  explicit.

Bug: 373845830
Change-Id: I3998a276bc210bc5bf5bccfe31c44b4e01de4472
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/211947
Reviewed-by: Shrek Shao <shrekshao@google.com>
Auto-Submit: Loko Kung <lokokung@google.com>
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Commit-Queue: Geoff Lang <geofflang@chromium.org>

src/dawn/native/CompilationMessages.cpp

diff --git a/src/dawn/native/CompilationMessages.cpp b/src/dawn/native/CompilationMessages.cpp
index 91c3b59..3fe0be5 100644
--- a/src/dawn/native/CompilationMessages.cpp
+++ b/src/dawn/native/CompilationMessages.cpp
@@ -83,11 +83,7 @@
     return numberOfUTF16CodeUnits;
 }
 
-OwnedCompilationMessages::OwnedCompilationMessages() {
-    mCompilationInfo.nextInChain = 0;
-    mCompilationInfo.messageCount = 0;
-    mCompilationInfo.messages = nullptr;
-}
+OwnedCompilationMessages::OwnedCompilationMessages() = default;
 
 OwnedCompilationMessages::~OwnedCompilationMessages() = default;
 
@@ -193,7 +189,7 @@
 
 void OwnedCompilationMessages::AddMessage(const CompilationMessage& message) {
     // Cannot add messages after GetCompilationInfo has been called.
-    DAWN_ASSERT(mCompilationInfo.messages == nullptr);
+    DAWN_ASSERT(!mCompilationInfo->has_value());
 
     DAWN_ASSERT(message.nextInChain == nullptr);
 
@@ -208,7 +204,7 @@
 
 MaybeError OwnedCompilationMessages::AddMessages(const tint::diag::List& diagnostics) {
     // Cannot add messages after GetCompilationInfo has been called.
-    DAWN_ASSERT(mCompilationInfo.messages == nullptr);
+    DAWN_ASSERT(!mCompilationInfo->has_value());
 
     for (const auto& diag : diagnostics) {
         DAWN_TRY(AddMessage(diag));
@@ -221,17 +217,23 @@
 
 void OwnedCompilationMessages::ClearMessages() {
     // Cannot clear messages after GetCompilationInfo has been called.
-    DAWN_ASSERT(mCompilationInfo.messages == nullptr);
+    DAWN_ASSERT(!mCompilationInfo->has_value());
 
     mMessageStrings.clear();
     mMessages.clear();
 }
 
 const CompilationInfo* OwnedCompilationMessages::GetCompilationInfo() {
-    mCompilationInfo.messageCount = mMessages.size();
-    mCompilationInfo.messages = mMessages.data();
+    return mCompilationInfo.Use([&](auto info) {
+        if (info->has_value()) {
+            return &info->value();
+        }
 
-    return &mCompilationInfo;
+        (*info).emplace();
+        (*info)->messageCount = mMessages.size();
+        (*info)->messages = mMessages.data();
+        return &info->value();
+    });
 }
 
 const std::vector<std::string>& OwnedCompilationMessages::GetFormattedTintMessages() const {

src/dawn/native/CompilationMessages.h

diff --git a/src/dawn/native/CompilationMessages.h b/src/dawn/native/CompilationMessages.h
index 8a0280d..48d79d9 100644
--- a/src/dawn/native/CompilationMessages.h
+++ b/src/dawn/native/CompilationMessages.h
@@ -32,11 +32,11 @@
 #include <string>
 #include <vector>
 
+#include "dawn/common/MutexProtected.h"
+#include "dawn/common/NonCopyable.h"
 #include "dawn/native/Error.h"
 #include "dawn/native/dawn_platform.h"
 
-#include "dawn/common/NonCopyable.h"
-
 namespace tint::diag {
 class Diagnostic;
 class List;
@@ -76,7 +76,7 @@
     void AddMessage(const CompilationMessage& message);
     void AddFormattedTintMessages(const tint::diag::List& diagnostics);
 
-    CompilationInfo mCompilationInfo;
+    MutexProtected<std::optional<CompilationInfo>> mCompilationInfo = std::nullopt;
     std::vector<std::unique_ptr<std::string>> mMessageStrings;
     std::vector<CompilationMessage> mMessages;
     std::vector<std::string> mFormattedTintMessages;