Skip to content

Commit

Permalink
[BOLT] Fix data race in MCPlusBuilder::getOrCreateAnnotationIndex (#6…
Browse files Browse the repository at this point in the history
…7004)

MCPlusBuilder::getOrCreateAnnotationIndex(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?

---------

Co-authored-by: Amir Ayupov <fads93@gmail.com>
  • Loading branch information
kbeyls and aaupov authored Sep 21, 2023
1 parent 3769aaa commit 8fb28e4
Showing 1 changed file with 9 additions and 3 deletions.
12 changes: 9 additions & 3 deletions bolt/include/bolt/Core/MCPlusBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <cassert>
#include <cstdint>
#include <map>
Expand Down Expand Up @@ -166,6 +167,10 @@ class MCPlusBuilder {
/// Names of non-standard annotations.
SmallVector<std::string, 8> 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);
Expand Down Expand Up @@ -1775,6 +1780,7 @@ class MCPlusBuilder {

/// Return annotation index matching the \p Name.
std::optional<unsigned> getAnnotationIndex(StringRef Name) const {
std::shared_lock<llvm::sys::RWMutex> Lock(AnnotationNameMutex);
auto AI = AnnotationNameIndexMap.find(Name);
if (AI != AnnotationNameIndexMap.end())
return AI->second;
Expand All @@ -1784,10 +1790,10 @@ 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;
if (std::optional<unsigned> Index = getAnnotationIndex(Name))
return *Index;

std::unique_lock<llvm::sys::RWMutex> Lock(AnnotationNameMutex);
const unsigned Index =
AnnotationNameIndexMap.size() + MCPlus::MCAnnotation::kGeneric;
AnnotationNameIndexMap.insert(std::make_pair(Name, Index));
Expand Down

0 comments on commit 8fb28e4

Please sign in to comment.