[TSAN] Add option to allow instrumenting reads of reads-before-writes

[melver]

Summary

Add -tsan-instrument-read-before-write which allows instrumenting reads
of reads-before-writes.

This is required for KCSAN [1], where under certain configurations plain
writes behave differently (e.g. aligned writes up to word size may be
treated as atomic). In order to avoid missing potential data races due
to plain RMW operations ("x++" etc.), we will require instrumenting
reads of reads-before-writes.

[1] https://github.com/google/ktsan/wiki/KCSAN

[dvyukov]

Maybe this flag should turn off instrumentation of the "atomic" writes? Then the "reads-before-write" are not before writes anymore and will be naturally instrumented. And we also get faster execution. What do you think?

[melver]

In D79983#2037942, @dvyukov wrote:

Maybe this flag should turn off instrumentation of the "atomic" writes? Then the "reads-before-write" are not before writes anymore and will be naturally instrumented. And we also get faster execution. What do you think?

Do you mean putting some of KCSAN's logic to determine what are "atomic" writes here? For one, I don't think it's reliable because the logic in KCSAN may change again and we shouldn't make any code here be KCSAN-specific.

The other thing is that conflicts between "atomic write" and "plain read" will be missed.

Or did I misunderstand what you meant?

[dvyukov]

In D79983#2038041, @melver wrote:

In D79983#2037942, @dvyukov wrote:

Maybe this flag should turn off instrumentation of the "atomic" writes? Then the "reads-before-write" are not before writes anymore and will be naturally instrumented. And we also get faster execution. What do you think?

Do you mean putting some of KCSAN's logic to determine what are "atomic" writes here? For one, I don't think it's reliable because the logic in KCSAN may change again and we shouldn't make any code here be KCSAN-specific.

The other thing is that conflicts between "atomic write" and "plain read" will be missed.

Or did I misunderstand what you meant?

Based on offline discussion my comment is withdrawn.

[dvyukov]

Merged as https://github.com/llvm/llvm-project/commit/151ed6aa38a3ec6c01973b35f684586b6e1c0f7e

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

 //===-- ThreadSanitizer.cpp - race detector -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of ThreadSanitizer, a race detector.
static cl::opt<bool>  ClInstrumentAtomics(
     cl::desc("Instrument atomics"), cl::Hidden);
 static cl::opt<bool>  ClInstrumentMemIntrinsics(
     "tsan-instrument-memintrinsics", cl::init(true),
     cl::desc("Instrument memintrinsics (memset/memcpy/memmove)"), cl::Hidden);
 static cl::opt<bool>  ClDistinguishVolatile(
     "tsan-distinguish-volatile", cl::init(false),
     cl::desc("Emit special instrumentation for accesses to volatiles"),
     cl::Hidden);
+static cl::opt<bool>  ClInstrumentReadBeforeWrite(
+    "tsan-instrument-read-before-write", cl::init(false),
+    cl::desc("Do not eliminate read instrumentation for read-before-writes"),
+    cl::Hidden);
 
 STATISTIC(NumInstrumentedReads, "Number of instrumented reads");
 STATISTIC(NumInstrumentedWrites, "Number of instrumented writes");
 STATISTIC(NumOmittedReadsBeforeWrite,
           "Number of reads ignored due to following writes");
 STATISTIC(NumAccessesWithBadSize, "Number of accesses with bad size");
 STATISTIC(NumInstrumentedVtableWrites, "Number of vtable ptr writes");
 STATISTIC(NumInstrumentedVtableReads, "Number of vtable ptr reads");
if (StoreInst *Store = dyn_cast<StoreInst>(I)) {
       if (!shouldInstrumentReadWriteFromAddress(I->getModule(), Addr))
         continue;
       WriteTargets.insert(Addr);
     } else {
       LoadInst *Load = cast<LoadInst>(I);
       Value *Addr = Load->getPointerOperand();
       if (!shouldInstrumentReadWriteFromAddress(I->getModule(), Addr))
         continue;
-      if (WriteTargets.count(Addr)) {
+      if (!ClInstrumentReadBeforeWrite && WriteTargets.count(Addr)) {
         // We will write to this temp, so no reason to analyze the read.
         NumOmittedReadsBeforeWrite++;
         continue;
       }
       if (addrPointsToConstantData(Addr)) {
         // Addr points to some constant data -- it can not race with any writes.
         continue;
       }

llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll

 ; RUN: opt < %s -tsan -S | FileCheck %s
+; RUN: opt < %s -tsan -tsan-instrument-read-before-write -S | FileCheck %s --check-prefixes=CHECK,CHECK-UNOPT
 
 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"
 
 define void @IncrementMe(i32* nocapture %ptr) nounwind uwtable sanitize_thread {
 entry:
   %0 = load i32, i32* %ptr, align 4
   %inc = add nsw i32 %0, 1
   store i32 %inc, i32* %ptr, align 4
   ret void
 }
 ; CHECK: define void @IncrementMe
 ; CHECK-NOT: __tsan_read
+; CHECK-UNOPT: __tsan_read
 ; CHECK: __tsan_write
 ; CHECK: ret void
 
 define void @IncrementMeWithCallInBetween(i32* nocapture %ptr) nounwind uwtable sanitize_thread {
 entry:
   %0 = load i32, i32* %ptr, align 4
   %inc = add nsw i32 %0, 1
   call void @foo()