[SanitizerBinaryMetadata] Pretend compiler-generated loads/stores are atomic

[melver]

Summary

Profiling and GCOV generate known data-racy loads/stores. Pretend they
are atomic so that analysis using PC-keyed metadata for atomics do not
report them as data races (which would look like false positives).

[dvyukov]

We also discussed that msan/asan shadow accesses need to be ignored.
MSan accesses can lead to false positives, ASan just to unnecessary checking.
It can also make sense to exclude accesses to global const objects.

[melver]

In D142982#4093842, @dvyukov wrote:

We also discussed that msan/asan shadow accesses need to be ignored.
MSan accesses can lead to false positives, ASan just to unnecessary checking.
It can also make sense to exclude accesses to global const objects.

Good points - will add in a future patch. Identifying sanitizer accesses is not obvious, probably need to add some more metadata in sanitizer passes.

[dvyukov]

TSan pass has these checks, probably can just copy-paste these:

  if (addrPointsToConstantData(Addr)) {
    // Addr points to some constant data -- it can not race with any writes.
    continue;
  }

if (isa<AllocaInst>(getUnderlyingObject(Addr)) &&
    !PointerMayBeCaptured(Addr, true, true)) {
  // The variable is addressable but not captured, so it cannot be
  // referenced from a different thread and participate in a data race
  // (see llvm/Analysis/CaptureTracking.h for details).
  NumOmittedNonCaptured++;
  continue;
}

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp

 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Value.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
+#include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 #include <array>
 #include <cstdint>
 
private:
   StringRef getSectionName(StringRef SectionSuffix);
 
   // Returns the section start marker name.
   Twine getSectionStart(StringRef SectionSuffix);
 
   // Returns the section end marker name.
   Twine getSectionEnd(StringRef SectionSuffix);
 
+  // Returns true if the access to the address should be considered "atomic".
+  bool pretendAtomicAccess(Value *Addr);
+
   Module &Mod;
   const SanitizerBinaryMetadataOptions Options;
   const Triple TargetTriple;
   IRBuilder<> IRB;
 };
 
 bool SanitizerBinaryMetadata::run() {
   MetadataInfoSet MIS;
bool useAfterReturnUnsafe(Instruction &I) {
   // Tail-called functions are not necessary intercepted
   // at runtime because there is no call instruction.
   // So conservatively mark the caller as requiring checking.
   else if (auto *CI = dyn_cast<CallInst>(&I))
     return CI->isTailCall() && !isUARSafeCall(CI);
   return false;
 }
 
+bool SanitizerBinaryMetadata::pretendAtomicAccess(Value *Addr) {
+  assert(Addr && "Expected non-null Addr");
+
+  Addr = Addr->stripInBoundsOffsets();
+  auto *GV = dyn_cast<GlobalVariable>(Addr);
+  if (!GV)
+    return false;
+
+  if (GV->hasSection()) {
+    const auto OF = Triple(Mod.getTargetTriple()).getObjectFormat();
+    const auto ProfSec =
+        getInstrProfSectionName(IPSK_cnts, OF, /*AddSegmentInfo=*/false);
+    if (GV->getSection().endswith(ProfSec))
+      return true;
+  }
+
+  if (GV->getName().startswith("__llvm_gcov") ||
+      GV->getName().startswith("__llvm_gcda"))
+    return true;
+
+  return false;
+}
+
 bool SanitizerBinaryMetadata::runOn(Instruction &I, MetadataInfoSet &MIS,
                                     MDBuilder &MDB, uint32_t &FeatureMask) {
   SmallVector<const MetadataInfo *, 1> InstMetadata;
   bool RequiresCovered = false;
 
   if (Options.UAR && !(FeatureMask & kSanitizerBinaryMetadataUAR)) {
     if (useAfterReturnUnsafe(I))
       FeatureMask |= kSanitizerBinaryMetadataUAR;
   }
 
   if (Options.Atomics && I.mayReadOrWriteMemory()) {
     auto SSID = getAtomicSyncScopeID(&I);
-    if (SSID.has_value() && *SSID != SyncScope::SingleThread) {
+    bool IsAtomic = SSID.has_value() && *SSID != SyncScope::SingleThread;
+
+    if (!IsAtomic) {
+      // Check to pretend some compiler-generated accesses are atomic, to avoid
+      // false positives in data-race analysis.
+      Value *Addr = nullptr;
+      if (auto *SI = dyn_cast<StoreInst>(&I))
+        Addr = SI->getPointerOperand();
+      else if (auto *LI = dyn_cast<LoadInst>(&I))
+        Addr = LI->getPointerOperand();
+      if (Addr)
+        IsAtomic = pretendAtomicAccess(Addr);
+    }
+
+    if (IsAtomic) {
       NumMetadataAtomics++;
       InstMetadata.push_back(&MetadataInfo::Atomics);
     }
     RequiresCovered = true;
   }
 
   // Attach MD_pcsections to instruction.
   if (!InstMetadata.empty()) {

llvm/test/Instrumentation/SanitizerBinaryMetadata/pretend-atomic-access.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes='module(sanmd-module)' -sanitizer-metadata-atomics -S | FileCheck %s
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@__profc_test_gep = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
+@__profc_test_bitcast = private global [2 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
+@__profc_test_bitcast_foo = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
+
+@__llvm_gcov_ctr = internal global [1 x i64] zeroinitializer
+@__llvm_gcov_ctr.1 = internal global [1 x i64] zeroinitializer
+@__llvm_gcov_global_state_pred = internal global i32 0
+@__llvm_gcda_foo = internal global i32 0
+
+define i32 @test_gep() sanitize_thread {
+; CHECK-LABEL: @test_gep(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[PGOCOUNT:%.*]] = load i64, ptr @__profc_test_gep, align 8, !pcsections !2
+; CHECK-NEXT:    [[TMP0:%.*]] = add i64 [[PGOCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP0]], ptr @__profc_test_gep, align 8, !pcsections !2
+; CHECK-NEXT:    [[GCOVCOUNT:%.*]] = load i64, ptr @__llvm_gcov_ctr, align 8, !pcsections !2
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[GCOVCOUNT]], 1
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__llvm_gcov_ctr, align 8, !pcsections !2
+; CHECK-NEXT:    [[GCOVCOUNT_1:%.*]] = load i64, ptr @__llvm_gcov_ctr.1, align 8, !pcsections !2
+; CHECK-NEXT:    [[TMP2:%.*]] = add i64 [[GCOVCOUNT_1]], 1
+; CHECK-NEXT:    store i64 [[TMP2]], ptr @__llvm_gcov_ctr.1, align 8, !pcsections !2
+; CHECK-NEXT:    ret i32 1
+;
+entry:
+  %pgocount = load i64, ptr @__profc_test_gep
+  %0 = add i64 %pgocount, 1
+  store i64 %0, ptr @__profc_test_gep
+
+  %gcovcount = load i64, ptr @__llvm_gcov_ctr
+  %1 = add i64 %gcovcount, 1
+  store i64 %1, ptr @__llvm_gcov_ctr
+
+  %gcovcount.1 = load i64, ptr @__llvm_gcov_ctr.1
+  %2 = add i64 %gcovcount.1, 1
+  store i64 %2, ptr @__llvm_gcov_ctr.1
+
+  ret i32 1
+}
+
+define i32 @test_bitcast() sanitize_thread {
+; CHECK-LABEL: @test_bitcast(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load <2 x i64>, ptr @__profc_test_bitcast, align 8, !pcsections !2
+; CHECK-NEXT:    [[DOTPROMOTED5:%.*]] = load i64, ptr @__profc_test_bitcast_foo, align 8, !pcsections !2
+; CHECK-NEXT:    [[TMP1:%.*]] = add i64 [[DOTPROMOTED5]], 10
+; CHECK-NEXT:    [[TMP2:%.*]] = add <2 x i64> [[TMP0]], <i64 1, i64 10>
+; CHECK-NEXT:    store <2 x i64> [[TMP2]], ptr @__profc_test_bitcast, align 8, !pcsections !2
+; CHECK-NEXT:    store i64 [[TMP1]], ptr @__profc_test_bitcast_foo, align 8, !pcsections !2
+; CHECK-NEXT:    ret i32 undef
+;
+entry:
+  %0 = load <2 x i64>, ptr @__profc_test_bitcast, align 8
+  %.promoted5 = load i64, ptr @__profc_test_bitcast_foo, align 8
+  %1 = add i64 %.promoted5, 10
+  %2 = add <2 x i64> %0, <i64 1, i64 10>
+  store <2 x i64> %2, ptr @__profc_test_bitcast, align 8
+  store i64 %1, ptr @__profc_test_bitcast_foo, align 8
+  ret i32 undef
+}
+
+define void @test_load() sanitize_thread {
+; CHECK-LABEL: @test_load(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr @__llvm_gcov_global_state_pred, align 4, !pcsections !2
+; CHECK-NEXT:    store i32 1, ptr @__llvm_gcov_global_state_pred, align 4, !pcsections !2
+; CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr @__llvm_gcda_foo, align 4, !pcsections !2
+; CHECK-NEXT:    store i32 1, ptr @__llvm_gcda_foo, align 4, !pcsections !2
+; CHECK-NEXT:    ret void
+;
+entry:
+  %0 = load i32, ptr @__llvm_gcov_global_state_pred
+  store i32 1, ptr @__llvm_gcov_global_state_pred
+
+  %1 = load i32, ptr @__llvm_gcda_foo
+  store i32 1, ptr @__llvm_gcda_foo
+
+  ret void
+}