From 3987fd2297ee9f13ef88c659294e08a514d62a80 Mon Sep 17 00:00:00 2001 From: Kristof Beyls Date: Thu, 21 Sep 2023 12:05:06 +0200 Subject: [PATCH 1/3] [BOLT] Fix data race in MCPlusBuilder::getOrCreateAnnotationIndex MCPlusBuilder::getAnnotationIndex(Name) can be called from different threads, for example when making use of ParallelUtilities::runOnEachFunctionWithUniqueAllocId. The race occurs when an Index for a particular annotation Name needs to be created for the first time. For example, this can easily happen when multiple "copies" of an analysis pass run on different BinaryFunctions, and the analysis pass creates a new Annotation Index to be able to store analysis results as annotations. This was found by using the ThreadSanitizer. No regression test was added; I don't think there is good way to write regression tests that verify the absence of data races? --- bolt/include/bolt/Core/MCPlusBuilder.h | 27 ++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index f48cf21852dcb..b67e172d43a38 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -29,6 +29,7 @@ #include "llvm/Support/Allocator.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/ErrorOr.h" +#include "llvm/Support/RWMutex.h" #include #include #include @@ -166,6 +167,10 @@ class MCPlusBuilder { /// Names of non-standard annotations. SmallVector AnnotationNames; + /// A mutex that is used to control parallel accesses to + /// AnnotationNameIndexMap and AnnotationsNames. + mutable llvm::sys::RWMutex AnnotationNameMutex; + /// Allocate the TailCall annotation value. Clients of the target-specific /// MCPlusBuilder classes must use convert/lower/create* interfaces instead. void setTailCall(MCInst &Inst); @@ -1775,6 +1780,7 @@ class MCPlusBuilder { /// Return annotation index matching the \p Name. std::optional getAnnotationIndex(StringRef Name) const { + std::shared_lock Lock(AnnotationNameMutex); auto AI = AnnotationNameIndexMap.find(Name); if (AI != AnnotationNameIndexMap.end()) return AI->second; @@ -1784,15 +1790,20 @@ class MCPlusBuilder { /// Return annotation index matching the \p Name. Create a new index if the /// \p Name wasn't registered previously. unsigned getOrCreateAnnotationIndex(StringRef Name) { - auto AI = AnnotationNameIndexMap.find(Name); - if (AI != AnnotationNameIndexMap.end()) - return AI->second; + { + std::optional Index = getAnnotationIndex(Name); + if (Index.has_value()) + return *Index; + } - const unsigned Index = - AnnotationNameIndexMap.size() + MCPlus::MCAnnotation::kGeneric; - AnnotationNameIndexMap.insert(std::make_pair(Name, Index)); - AnnotationNames.emplace_back(std::string(Name)); - return Index; + { + std::unique_lock Lock(AnnotationNameMutex); + const unsigned Index = + AnnotationNameIndexMap.size() + MCPlus::MCAnnotation::kGeneric; + AnnotationNameIndexMap.insert(std::make_pair(Name, Index)); + AnnotationNames.emplace_back(std::string(Name)); + return Index; + } } /// Store an annotation value on an MCInst. This assumes the annotation From faad7c7d61accc9cb93d25a456bc63600267d8fa Mon Sep 17 00:00:00 2001 From: Kristof Beyls Date: Thu, 21 Sep 2023 19:34:13 +0200 Subject: [PATCH 2/3] Update bolt/include/bolt/Core/MCPlusBuilder.h Co-authored-by: Amir Ayupov --- bolt/include/bolt/Core/MCPlusBuilder.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index b67e172d43a38..4116424fffe23 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -1791,8 +1791,7 @@ class MCPlusBuilder { /// \p Name wasn't registered previously. unsigned getOrCreateAnnotationIndex(StringRef Name) { { - std::optional Index = getAnnotationIndex(Name); - if (Index.has_value()) + if (std::optional Index = getAnnotationIndex(Name)) return *Index; } From 7c316757ecb4540d4c215c2608fb39dd8d6637fe Mon Sep 17 00:00:00 2001 From: Kristof Beyls Date: Thu, 21 Sep 2023 19:40:23 +0200 Subject: [PATCH 3/3] Another minor improvement to coding style. --- bolt/include/bolt/Core/MCPlusBuilder.h | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index 4116424fffe23..7eb45d2a91405 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -1790,19 +1790,15 @@ class MCPlusBuilder { /// Return annotation index matching the \p Name. Create a new index if the /// \p Name wasn't registered previously. unsigned getOrCreateAnnotationIndex(StringRef Name) { - { - if (std::optional Index = getAnnotationIndex(Name)) - return *Index; - } - - { - std::unique_lock Lock(AnnotationNameMutex); - const unsigned Index = - AnnotationNameIndexMap.size() + MCPlus::MCAnnotation::kGeneric; - AnnotationNameIndexMap.insert(std::make_pair(Name, Index)); - AnnotationNames.emplace_back(std::string(Name)); - return Index; - } + if (std::optional Index = getAnnotationIndex(Name)) + return *Index; + + std::unique_lock Lock(AnnotationNameMutex); + const unsigned Index = + AnnotationNameIndexMap.size() + MCPlus::MCAnnotation::kGeneric; + AnnotationNameIndexMap.insert(std::make_pair(Name, Index)); + AnnotationNames.emplace_back(std::string(Name)); + return Index; } /// Store an annotation value on an MCInst. This assumes the annotation