MaskRay created this revision.
MaskRay added reviewers: Sanitizers, dvyukov, melver, vitalybuka, Enna1.
Herald added a subscriber: hiraditya.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.
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
<https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D158385
Files:
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
Index: llvm/test/Instrumentation/ThreadSanitizer/do-not-instrument-memory-access.ll
===================================================================
--- llvm/test/Instrumentation/ThreadSanitizer/do-not-instrument-memory-access.ll
+++ 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 @@
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
Index: llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
===================================================================
--- llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
@@ -364,11 +364,6 @@
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 different address spaces; we cannot deal
@@ -522,6 +517,9 @@
// 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))
Index: clang/test/Driver/fprofile-update.c
===================================================================
--- clang/test/Driver/fprofile-update.c
+++ 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
Index: clang/test/CodeGen/code-coverage-tsan.c
===================================================================
--- clang/test/CodeGen/code-coverage-tsan.c
+++ 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
Index: clang/lib/Driver/ToolChains/Clang.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -869,8 +869,6 @@
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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
