[clang] 5624e86 - [tsan] Respect !nosanitize metadata and remove gcov special case
Fangrui Song via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 24 22:31:16 PDT 2023
Author: Fangrui Song
Date: 2023-08-24T22:31:11-07:00
New Revision: 5624e86ae0fb259c6069d1cc7d5eaa24402f9134
URL: https://github.com/llvm/llvm-project/commit/5624e86ae0fb259c6069d1cc7d5eaa24402f9134
DIFF: https://github.com/llvm/llvm-project/commit/5624e86ae0fb259c6069d1cc7d5eaa24402f9134.diff
LOG: [tsan] Respect !nosanitize metadata and remove gcov special case
Certain instrumentations set the !nosanitize metadata for inserted
instructions, which are generally not interested for sanitizers. Skip
tsan instrumentation like we do for asan (D126294)/msan/hwasan.
-fprofile-arcs instrumentation has data race unless
-fprofile-update=atomic is specified. Let's remove the the `__llvm_gcov`
special case from commit 0222adbcd25779a156399bcc16fde9f6d083a809 (2016)
as the racy instructions have the !nosanitize metadata.
(-fprofile-arcs instrumentation does not use `__llvm_gcda` as global variables.)
```
std::atomic<int> c;
void foo() { c++; }
int main() {
std::thread th(foo);
c++;
th.join();
}
```
Tested that `clang++ --coverage -fsanitize=thread a.cc && ./a.out` does
not report spurious tsan errors.
Also remove the default CC1 option -fprofile-update=atomic for
-fsanitize=thread to make options more orthogonal.
Reviewed By: Enna1
Differential Revision: https://reviews.llvm.org/D158385
Added:
Modified:
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/CodeGen/code-coverage-tsan.c
clang/test/Driver/fprofile-update.c
llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
llvm/test/Instrumentation/ThreadSanitizer/do-not-instrument-memory-access.ll
Removed:
################################################################################
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 488838b1ff4bb7..aaed70a1461ef4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -869,8 +869,6 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
else if (Val != "single")
D.Diag(diag::err_drv_unsupported_option_argument)
<< A->getSpelling() << Val;
- } else if (SanArgs.needsTsanRt()) {
- CmdArgs.push_back("-fprofile-update=atomic");
}
int FunctionGroups = 1;
diff --git a/clang/test/CodeGen/code-coverage-tsan.c b/clang/test/CodeGen/code-coverage-tsan.c
index 8b0676dd454626..f6cf7f73f173ee 100644
--- a/clang/test/CodeGen/code-coverage-tsan.c
+++ b/clang/test/CodeGen/code-coverage-tsan.c
@@ -1,5 +1,4 @@
-/// -fprofile-update=atomic (implied by -fsanitize=thread) requires the
-/// (potentially concurrent) counter updates to be atomic.
+/// -fprofile-update=atomic requires the (potentially concurrent) counter updates to be atomic.
// RUN: %clang_cc1 %s -triple x86_64 -emit-llvm -fprofile-update=atomic \
// RUN: -coverage-notes-file=/dev/null -coverage-data-file=/dev/null -o - | FileCheck %s
diff --git a/clang/test/Driver/fprofile-update.c b/clang/test/Driver/fprofile-update.c
index 35fd830cee861a..2cad883cd81717 100644
--- a/clang/test/Driver/fprofile-update.c
+++ b/clang/test/Driver/fprofile-update.c
@@ -1,6 +1,5 @@
/// For -fprofile-instr-generate and -fprofile-arcs, increment counters atomically
-/// if -fprofile-update={atomic,prefer-atomic} or -fsanitize=thread is specified.
-// RUN: %clang -### %s -c -target x86_64-linux -fsanitize=thread %s 2>&1 | FileCheck %s
+/// if -fprofile-update={atomic,prefer-atomic} is specified.
// RUN: %clang -### %s -c -fprofile-update=atomic 2>&1 | FileCheck %s
// RUN: %clang -### %s -c -fprofile-update=prefer-atomic 2>&1 | FileCheck %s
diff --git a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
index 229b9438057028..65081ac5348451 100644
--- a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
@@ -364,11 +364,6 @@ static bool shouldInstrumentReadWriteFromAddress(const Module *M, Value *Addr) {
getInstrProfSectionName(IPSK_cnts, OF, /*AddSegmentInfo=*/false)))
return false;
}
-
- // Check if the global is private gcov data.
- if (GV->getName().startswith("__llvm_gcov") ||
- GV->getName().startswith("__llvm_gcda"))
- return false;
}
// Do not instrument accesses from
diff erent address spaces; we cannot deal
@@ -522,6 +517,9 @@ bool ThreadSanitizer::sanitizeFunction(Function &F,
// Traverse all instructions, collect loads/stores/returns, check for calls.
for (auto &BB : F) {
for (auto &Inst : BB) {
+ // Skip instructions inserted by another instrumentation.
+ if (Inst.hasMetadata(LLVMContext::MD_nosanitize))
+ continue;
if (isTsanAtomic(&Inst))
AtomicAccesses.push_back(&Inst);
else if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst))
diff --git a/llvm/test/Instrumentation/ThreadSanitizer/do-not-instrument-memory-access.ll b/llvm/test/Instrumentation/ThreadSanitizer/do-not-instrument-memory-access.ll
index c0efa250fd50aa..3e7527cce76c64 100644
--- a/llvm/test/Instrumentation/ThreadSanitizer/do-not-instrument-memory-access.ll
+++ b/llvm/test/Instrumentation/ThreadSanitizer/do-not-instrument-memory-access.ll
@@ -1,6 +1,6 @@
; This test checks that we are not instrumenting unwanted acesses to globals:
+; - Instructions with the !nosanitize metadata (e.g. -fprofile-arcs instrumented counter accesses)
; - Instruction profiler counter instrumentation has known intended races.
-; - The gcov counters array has a known intended race.
;
; RUN: opt < %s -passes='function(tsan),module(tsan-module)' -S | FileCheck %s
@@ -18,42 +18,44 @@ target triple = "x86_64-apple-macosx10.9"
define i32 @test_gep() sanitize_thread {
entry:
- %pgocount = load i64, ptr @__profc_test_gep
+ %pgocount = load i64, ptr @__profc_test_gep, !nosanitize !0
%0 = add i64 %pgocount, 1
- store i64 %0, ptr @__profc_test_gep
+ store i64 %0, ptr @__profc_test_gep, !nosanitize !0
- %gcovcount = load i64, ptr @__llvm_gcov_ctr
+ %gcovcount = load i64, ptr @__llvm_gcov_ctr, !nosanitize !0
%1 = add i64 %gcovcount, 1
- store i64 %1, ptr @__llvm_gcov_ctr
+ store i64 %1, ptr @__llvm_gcov_ctr, !nosanitize !0
- %gcovcount.1 = load i64, ptr @__llvm_gcov_ctr.1
+ %gcovcount.1 = load i64, ptr @__llvm_gcov_ctr.1, !nosanitize !0
%2 = add i64 %gcovcount.1, 1
- store i64 %2, ptr @__llvm_gcov_ctr.1
+ store i64 %2, ptr @__llvm_gcov_ctr.1, !nosanitize !0
ret i32 1
}
define i32 @test_bitcast() sanitize_thread {
entry:
- %0 = load <2 x i64>, ptr @__profc_test_bitcast, align 8
- %.promoted5 = load i64, ptr @__profc_test_bitcast_foo, align 8
+ %0 = load <2 x i64>, ptr @__profc_test_bitcast, align 8, !nosanitize !0
+ %.promoted5 = load i64, ptr @__profc_test_bitcast_foo, align 8, !nosanitize !0
%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
+ store <2 x i64> %2, ptr @__profc_test_bitcast, align 8, !nosanitize !0
+ store i64 %1, ptr @__profc_test_bitcast_foo, align 8, !nosanitize !0
ret i32 undef
}
define void @test_load() sanitize_thread {
entry:
- %0 = load i32, ptr @__llvm_gcov_global_state_pred
- store i32 1, ptr @__llvm_gcov_global_state_pred
+ %0 = load i32, ptr @__llvm_gcov_global_state_pred, !nosanitize !0
+ store i32 1, ptr @__llvm_gcov_global_state_pred, !nosanitize !0
- %1 = load i32, ptr @__llvm_gcda_foo
- store i32 1, ptr @__llvm_gcda_foo
+ %1 = load i32, ptr @__llvm_gcda_foo, !nosanitize !0
+ store i32 1, ptr @__llvm_gcda_foo, !nosanitize !0
ret void
}
+!0 = !{}
+
; CHECK-NOT: {{call void @__tsan_write}}
; CHECK: __tsan_init
More information about the cfe-commits
mailing list