[Lldb-commits] [lldb] [lldb] Make TypeSystem::m_sym_file atomic to fix data race (PR #198923)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

<details>
<summary>Changes</summary>

SymbolFileCommon::GetTypeSystemForLanguage unconditionally writes this pointer with `ts->SetSymbolFile(this)` on every lookup, which races with concurrent reads from other threads.

The race is benign in practice: there is exactly (one SymbolFile per Module, so every writer stores the same pointer) but it is still undefined behavior under the C++ memory model.

Make the field std::atomic<SymbolFile *> and turn SetSymbolFile into a compare-exchange that asserts a TypeSystem is never rebound to a different SymbolFile, documenting the invariant that lets us get away with this.

The alternative is to have the SymbolFile pointer passed in through the constructor, but that would require updating a bunch of call sites, including various plugin interfaces.

Found by ThreadSanitizer as part of #<!-- -->197792.

---
Full diff: https://github.com/llvm/llvm-project/pull/198923.diff


1 Files Affected:

- (modified) lldb/include/lldb/Symbol/TypeSystem.h (+27-5) 


``````````diff
diff --git a/lldb/include/lldb/Symbol/TypeSystem.h b/lldb/include/lldb/Symbol/TypeSystem.h
index ee5a27b157490..6899d71688431 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -9,6 +9,8 @@
 #ifndef LLDB_SYMBOL_TYPESYSTEM_H
 #define LLDB_SYMBOL_TYPESYSTEM_H
 
+#include <atomic>
+#include <cassert>
 #include <functional>
 #include <mutex>
 #include <optional>
@@ -91,10 +93,6 @@ class TypeSystem : public PluginInterface,
   virtual PDBASTParser *GetPDBParser() { return nullptr; }
   virtual npdb::PdbAstBuilder *GetNativePDBParser() { return nullptr; }
 
-  virtual SymbolFile *GetSymbolFile() const { return m_sym_file; }
-
-  virtual void SetSymbolFile(SymbolFile *sym_file) { m_sym_file = sym_file; }
-
   // CompilerDecl functions
   virtual ConstString DeclGetName(void *opaque_decl) = 0;
 
@@ -559,13 +557,37 @@ class TypeSystem : public PluginInterface,
 
   virtual std::optional<llvm::json::Value> ReportStatistics();
 
+  SymbolFile *GetSymbolFile() const { return m_sym_file.load(); }
+
+  /// Bind this TypeSystem to its owning SymbolFile.
+  ///
+  /// A TypeSystem is created by its Module's TypeSystemMap before that
+  /// Module has an associated SymbolFile. The SymbolFile installs this
+  /// back-pointer when it first hands out the TypeSystem so that lazy
+  /// type-completion callbacks can route through it.
+  ///
+  /// Each Module owns at most one SymbolFile and one TypeSystem per
+  /// language, so the binding is expected to be established exactly once.
+  /// Repeated calls with the same pointer are no-ops. Rebinding to a
+  /// different non-null SymbolFile is a programming error.
+  void SetSymbolFile(SymbolFile *sym_file) {
+    SymbolFile *expected = nullptr;
+    if (m_sym_file.compare_exchange_strong(expected, sym_file))
+      return;
+    assert(expected == sym_file &&
+           "TypeSystem rebound to a different SymbolFile");
+  }
+
   bool GetHasForcefullyCompletedTypes() const {
     return m_has_forcefully_completed_types;
   }
+
 protected:
-  SymbolFile *m_sym_file = nullptr;
   /// Used for reporting statistics.
   bool m_has_forcefully_completed_types = false;
+
+private:
+  std::atomic<SymbolFile *> m_sym_file = nullptr;
 };
 
 class TypeSystemMap {

``````````

</details>


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