Skip to content

Commit 5624e86

Browse files
committed
[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 0222adb (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
1 parent 4235bc0 commit 5624e86

File tree

5 files changed

+22
-26
lines changed

5 files changed

+22
-26
lines changed

clang/lib/Driver/ToolChains/Clang.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -869,8 +869,6 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
869869
else if (Val != "single")
870870
D.Diag(diag::err_drv_unsupported_option_argument)
871871
<< A->getSpelling() << Val;
872-
} else if (SanArgs.needsTsanRt()) {
873-
CmdArgs.push_back("-fprofile-update=atomic");
874872
}
875873

876874
int FunctionGroups = 1;

clang/test/CodeGen/code-coverage-tsan.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
/// -fprofile-update=atomic (implied by -fsanitize=thread) requires the
2-
/// (potentially concurrent) counter updates to be atomic.
1+
/// -fprofile-update=atomic requires the (potentially concurrent) counter updates to be atomic.
32
// RUN: %clang_cc1 %s -triple x86_64 -emit-llvm -fprofile-update=atomic \
43
// RUN: -coverage-notes-file=/dev/null -coverage-data-file=/dev/null -o - | FileCheck %s
54

clang/test/Driver/fprofile-update.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
/// For -fprofile-instr-generate and -fprofile-arcs, increment counters atomically
2-
/// if -fprofile-update={atomic,prefer-atomic} or -fsanitize=thread is specified.
3-
// RUN: %clang -### %s -c -target x86_64-linux -fsanitize=thread %s 2>&1 | FileCheck %s
2+
/// if -fprofile-update={atomic,prefer-atomic} is specified.
43
// RUN: %clang -### %s -c -fprofile-update=atomic 2>&1 | FileCheck %s
54
// RUN: %clang -### %s -c -fprofile-update=prefer-atomic 2>&1 | FileCheck %s
65

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -364,11 +364,6 @@ static bool shouldInstrumentReadWriteFromAddress(const Module *M, Value *Addr) {
364364
getInstrProfSectionName(IPSK_cnts, OF, /*AddSegmentInfo=*/false)))
365365
return false;
366366
}
367-
368-
// Check if the global is private gcov data.
369-
if (GV->getName().startswith("__llvm_gcov") ||
370-
GV->getName().startswith("__llvm_gcda"))
371-
return false;
372367
}
373368

374369
// Do not instrument accesses from different address spaces; we cannot deal
@@ -522,6 +517,9 @@ bool ThreadSanitizer::sanitizeFunction(Function &F,
522517
// Traverse all instructions, collect loads/stores/returns, check for calls.
523518
for (auto &BB : F) {
524519
for (auto &Inst : BB) {
520+
// Skip instructions inserted by another instrumentation.
521+
if (Inst.hasMetadata(LLVMContext::MD_nosanitize))
522+
continue;
525523
if (isTsanAtomic(&Inst))
526524
AtomicAccesses.push_back(&Inst);
527525
else if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst))
Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
; This test checks that we are not instrumenting unwanted acesses to globals:
2+
; - Instructions with the !nosanitize metadata (e.g. -fprofile-arcs instrumented counter accesses)
23
; - Instruction profiler counter instrumentation has known intended races.
3-
; - The gcov counters array has a known intended race.
44
;
55
; RUN: opt < %s -passes='function(tsan),module(tsan-module)' -S | FileCheck %s
66

@@ -18,42 +18,44 @@ target triple = "x86_64-apple-macosx10.9"
1818

1919
define i32 @test_gep() sanitize_thread {
2020
entry:
21-
%pgocount = load i64, ptr @__profc_test_gep
21+
%pgocount = load i64, ptr @__profc_test_gep, !nosanitize !0
2222
%0 = add i64 %pgocount, 1
23-
store i64 %0, ptr @__profc_test_gep
23+
store i64 %0, ptr @__profc_test_gep, !nosanitize !0
2424

25-
%gcovcount = load i64, ptr @__llvm_gcov_ctr
25+
%gcovcount = load i64, ptr @__llvm_gcov_ctr, !nosanitize !0
2626
%1 = add i64 %gcovcount, 1
27-
store i64 %1, ptr @__llvm_gcov_ctr
27+
store i64 %1, ptr @__llvm_gcov_ctr, !nosanitize !0
2828

29-
%gcovcount.1 = load i64, ptr @__llvm_gcov_ctr.1
29+
%gcovcount.1 = load i64, ptr @__llvm_gcov_ctr.1, !nosanitize !0
3030
%2 = add i64 %gcovcount.1, 1
31-
store i64 %2, ptr @__llvm_gcov_ctr.1
31+
store i64 %2, ptr @__llvm_gcov_ctr.1, !nosanitize !0
3232

3333
ret i32 1
3434
}
3535

3636
define i32 @test_bitcast() sanitize_thread {
3737
entry:
38-
%0 = load <2 x i64>, ptr @__profc_test_bitcast, align 8
39-
%.promoted5 = load i64, ptr @__profc_test_bitcast_foo, align 8
38+
%0 = load <2 x i64>, ptr @__profc_test_bitcast, align 8, !nosanitize !0
39+
%.promoted5 = load i64, ptr @__profc_test_bitcast_foo, align 8, !nosanitize !0
4040
%1 = add i64 %.promoted5, 10
4141
%2 = add <2 x i64> %0, <i64 1, i64 10>
42-
store <2 x i64> %2, ptr @__profc_test_bitcast, align 8
43-
store i64 %1, ptr @__profc_test_bitcast_foo, align 8
42+
store <2 x i64> %2, ptr @__profc_test_bitcast, align 8, !nosanitize !0
43+
store i64 %1, ptr @__profc_test_bitcast_foo, align 8, !nosanitize !0
4444
ret i32 undef
4545
}
4646

4747
define void @test_load() sanitize_thread {
4848
entry:
49-
%0 = load i32, ptr @__llvm_gcov_global_state_pred
50-
store i32 1, ptr @__llvm_gcov_global_state_pred
49+
%0 = load i32, ptr @__llvm_gcov_global_state_pred, !nosanitize !0
50+
store i32 1, ptr @__llvm_gcov_global_state_pred, !nosanitize !0
5151

52-
%1 = load i32, ptr @__llvm_gcda_foo
53-
store i32 1, ptr @__llvm_gcda_foo
52+
%1 = load i32, ptr @__llvm_gcda_foo, !nosanitize !0
53+
store i32 1, ptr @__llvm_gcda_foo, !nosanitize !0
5454

5555
ret void
5656
}
5757

58+
!0 = !{}
59+
5860
; CHECK-NOT: {{call void @__tsan_write}}
5961
; CHECK: __tsan_init

0 commit comments

Comments
 (0)