On Thu, Apr 23, 2020 at 5:43 PM Marco Elver <el...@google.com> wrote: > > Add support to optionally emit different instrumentation for accesses to > volatile variables. While the default TSAN runtime likely will never > require this feature, other runtimes for different environments that > have subtly different memory models or assumptions may require > distinguishing volatiles. > > One such environment are OS kernels, where volatile is still used in > various places for various reasons, and often declare volatile to be > "safe enough" even in multi-threaded contexts. One such example is the > Linux kernel, which implements various synchronization primitives using > volatile (READ_ONCE(), WRITE_ONCE()). Here the Kernel Concurrency > Sanitizer (KCSAN) [1], is a runtime that uses TSAN instrumentation but > otherwise implements a very different approach to race detection from > TSAN. > > While in the Linux kernel it is generally discouraged to use volatiles > explicitly, the topic will likely come up again, and we will eventually > need to distinguish volatile accesses [2]. The other use-case is > ignoring data races on specially marked variables in the kernel, for > example bit-flags (here we may hide 'volatile' behind a different name > such as 'no_data_race'). > > [1] https://github.com/google/ktsan/wiki/KCSAN > [2] > https://lkml.kernel.org/r/CANpmjNOfXNE-Zh3MNP=-gmnhvKbsfUfTtWkyg_=vqtxs4nn...@mail.gmail.com
Hi Jakub, FWIW this is: Acked-by: Dmitry Vyukov <dvuy...@google.com> We just landed a similar change to llvm: https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 Do you have any objections? Yes, I know volatile is not related to threading :) But 5 years we have a similar patch for gcc for another race detector prototype: https://gist.github.com/xairy/862ba3260348efe23a37decb93aa79e9 So this is not the first time this comes up. Thanks > 2020-04-23 Marco Elver <el...@google.com> > > gcc/ > * params.opt: Define --param=tsan-distinguish-volatile=[0,1]. > * sanitizer.def (BUILT_IN_TSAN_VOLATILE_READ1): Define new > builtin for volatile instrumentation of reads/writes. > (BUILT_IN_TSAN_VOLATILE_READ2): Likewise. > (BUILT_IN_TSAN_VOLATILE_READ4): Likewise. > (BUILT_IN_TSAN_VOLATILE_READ8): Likewise. > (BUILT_IN_TSAN_VOLATILE_READ16): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE1): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE2): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE4): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE8): Likewise. > (BUILT_IN_TSAN_VOLATILE_WRITE16): Likewise. > * tsan.c (get_memory_access_decl): Argument if access is > volatile. If param tsan-distinguish-volatile is non-zero, and > access if volatile, return volatile instrumentation decl. > (instrument_expr): Check if access is volatile. > > gcc/testsuite/ > * c-c++-common/tsan/volatile.c: New test. > --- > gcc/ChangeLog | 19 +++++++ > gcc/params.opt | 4 ++ > gcc/sanitizer.def | 21 ++++++++ > gcc/testsuite/ChangeLog | 4 ++ > gcc/testsuite/c-c++-common/tsan/volatile.c | 62 ++++++++++++++++++++++ > gcc/tsan.c | 53 ++++++++++++------ > 6 files changed, 146 insertions(+), 17 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/tsan/volatile.c > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog > index 5f299e463db..aa2bb98ae05 100644 > --- a/gcc/ChangeLog > +++ b/gcc/ChangeLog > @@ -1,3 +1,22 @@ > +2020-04-23 Marco Elver <el...@google.com> > + > + * params.opt: Define --param=tsan-distinguish-volatile=[0,1]. > + * sanitizer.def (BUILT_IN_TSAN_VOLATILE_READ1): Define new > + builtin for volatile instrumentation of reads/writes. > + (BUILT_IN_TSAN_VOLATILE_READ2): Likewise. > + (BUILT_IN_TSAN_VOLATILE_READ4): Likewise. > + (BUILT_IN_TSAN_VOLATILE_READ8): Likewise. > + (BUILT_IN_TSAN_VOLATILE_READ16): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE1): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE2): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE4): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE8): Likewise. > + (BUILT_IN_TSAN_VOLATILE_WRITE16): Likewise. > + * tsan.c (get_memory_access_decl): Argument if access is > + volatile. If param tsan-distinguish-volatile is non-zero, and > + access if volatile, return volatile instrumentation decl. > + (instrument_expr): Check if access is volatile. > + > 2020-04-23 Srinath Parvathaneni <srinath.parvathan...@arm.com> > > * config/arm/arm_mve.h (__arm_vbicq_n_u16): Modify function > parameter's > diff --git a/gcc/params.opt b/gcc/params.opt > index 4aec480798b..9b564bb046c 100644 > --- a/gcc/params.opt > +++ b/gcc/params.opt > @@ -908,6 +908,10 @@ Stop reverse growth if the reverse probability of best > edge is less than this th > Common Joined UInteger Var(param_tree_reassoc_width) Param Optimization > Set the maximum number of instructions executed in parallel in reassociated > tree. If 0, use the target dependent heuristic. > > +-param=tsan-distinguish-volatile= > +Common Joined UInteger Var(param_tsan_distinguish_volatile) IntegerRange(0, > 1) Param > +Emit special instrumentation for accesses to volatiles. > + > -param=uninit-control-dep-attempts= > Common Joined UInteger Var(param_uninit_control_dep_attempts) Init(1000) > IntegerRange(1, 65536) Param Optimization > Maximum number of nested calls to search for control dependencies during > uninitialized variable analysis. > diff --git a/gcc/sanitizer.def b/gcc/sanitizer.def > index 11eb6467eba..a32715ddb92 100644 > --- a/gcc/sanitizer.def > +++ b/gcc/sanitizer.def > @@ -214,6 +214,27 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ_RANGE, > "__tsan_read_range", > DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE_RANGE, "__tsan_write_range", > BT_FN_VOID_PTR_PTRMODE, ATTR_NOTHROW_LEAF_LIST) > > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ1, "__tsan_volatile_read1", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ2, "__tsan_volatile_read2", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ4, "__tsan_volatile_read4", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ8, "__tsan_volatile_read8", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ16, > "__tsan_volatile_read16", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE1, > "__tsan_volatile_write1", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE2, > "__tsan_volatile_write2", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE4, > "__tsan_volatile_write4", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE8, > "__tsan_volatile_write8", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE16, > "__tsan_volatile_write16", > + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) > + > DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_ATOMIC8_LOAD, > "__tsan_atomic8_load", > BT_FN_I1_CONST_VPTR_INT, ATTR_NOTHROW_LEAF_LIST) > diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog > index 245c1512c76..f1d3e236b86 100644 > --- a/gcc/testsuite/ChangeLog > +++ b/gcc/testsuite/ChangeLog > @@ -1,3 +1,7 @@ > +2020-04-23 Marco Elver <el...@google.com> > + > + * c-c++-common/tsan/volatile.c: New test. > + > 2020-04-23 Jakub Jelinek <ja...@redhat.com> > > PR target/94707 > diff --git a/gcc/testsuite/c-c++-common/tsan/volatile.c > b/gcc/testsuite/c-c++-common/tsan/volatile.c > new file mode 100644 > index 00000000000..d51d1e3ce8d > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/tsan/volatile.c > @@ -0,0 +1,62 @@ > +/* { dg-additional-options "--param=tsan-distinguish-volatile=1" } */ > + > +#include <assert.h> > +#include <stdint.h> > +#include <stdio.h> > + > +int32_t Global4; > +volatile int32_t VolatileGlobal4; > +volatile int64_t VolatileGlobal8; > + > +static int nvolatile_reads; > +static int nvolatile_writes; > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +__attribute__((no_sanitize_thread)) > +void __tsan_volatile_read4(void *addr) { > + assert(addr == &VolatileGlobal4); > + nvolatile_reads++; > +} > +__attribute__((no_sanitize_thread)) > +void __tsan_volatile_write4(void *addr) { > + assert(addr == &VolatileGlobal4); > + nvolatile_writes++; > +} > +__attribute__((no_sanitize_thread)) > +void __tsan_volatile_read8(void *addr) { > + assert(addr == &VolatileGlobal8); > + nvolatile_reads++; > +} > +__attribute__((no_sanitize_thread)) > +void __tsan_volatile_write8(void *addr) { > + assert(addr == &VolatileGlobal8); > + nvolatile_writes++; > +} > + > +#ifdef __cplusplus > +} > +#endif > + > +__attribute__((no_sanitize_thread)) > +static void check() { > + assert(nvolatile_reads == 4); > + assert(nvolatile_writes == 4); > +} > + > +int main() { > + Global4 = 1; > + > + VolatileGlobal4 = 1; > + Global4 = VolatileGlobal4; > + VolatileGlobal4 = 1 + VolatileGlobal4; > + > + VolatileGlobal8 = 1; > + Global4 = (int32_t)VolatileGlobal8; > + VolatileGlobal8 = 1 + VolatileGlobal8; > + > + check(); > + return 0; > +} > diff --git a/gcc/tsan.c b/gcc/tsan.c > index 8d22a776377..04e92559584 100644 > --- a/gcc/tsan.c > +++ b/gcc/tsan.c > @@ -52,25 +52,41 @@ along with GCC; see the file COPYING3. If not see > void __tsan_read/writeX (void *addr); */ > > static tree > -get_memory_access_decl (bool is_write, unsigned size) > +get_memory_access_decl (bool is_write, unsigned size, bool volatilep) > { > enum built_in_function fcode; > > - if (size <= 1) > - fcode = is_write ? BUILT_IN_TSAN_WRITE1 > - : BUILT_IN_TSAN_READ1; > - else if (size <= 3) > - fcode = is_write ? BUILT_IN_TSAN_WRITE2 > - : BUILT_IN_TSAN_READ2; > - else if (size <= 7) > - fcode = is_write ? BUILT_IN_TSAN_WRITE4 > - : BUILT_IN_TSAN_READ4; > - else if (size <= 15) > - fcode = is_write ? BUILT_IN_TSAN_WRITE8 > - : BUILT_IN_TSAN_READ8; > + if (param_tsan_distinguish_volatile && volatilep) > + { > + if (size <= 1) > + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE1 > + : BUILT_IN_TSAN_VOLATILE_READ1; > + else if (size <= 3) > + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE2 > + : BUILT_IN_TSAN_VOLATILE_READ2; > + else if (size <= 7) > + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE4 > + : BUILT_IN_TSAN_VOLATILE_READ4; > + else if (size <= 15) > + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE8 > + : BUILT_IN_TSAN_VOLATILE_READ8; > + else > + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE16 > + : BUILT_IN_TSAN_VOLATILE_READ16; > + } > else > - fcode = is_write ? BUILT_IN_TSAN_WRITE16 > - : BUILT_IN_TSAN_READ16; > + { > + if (size <= 1) > + fcode = is_write ? BUILT_IN_TSAN_WRITE1 : BUILT_IN_TSAN_READ1; > + else if (size <= 3) > + fcode = is_write ? BUILT_IN_TSAN_WRITE2 : BUILT_IN_TSAN_READ2; > + else if (size <= 7) > + fcode = is_write ? BUILT_IN_TSAN_WRITE4 : BUILT_IN_TSAN_READ4; > + else if (size <= 15) > + fcode = is_write ? BUILT_IN_TSAN_WRITE8 : BUILT_IN_TSAN_READ8; > + else > + fcode = is_write ? BUILT_IN_TSAN_WRITE16 : BUILT_IN_TSAN_READ16; > + } > > return builtin_decl_implicit (fcode); > } > @@ -204,8 +220,11 @@ instrument_expr (gimple_stmt_iterator gsi, tree expr, > bool is_write) > g = gimple_build_call (builtin_decl, 2, expr_ptr, size_int (size)); > } > else if (rhs == NULL) > - g = gimple_build_call (get_memory_access_decl (is_write, size), > - 1, expr_ptr); > + { > + builtin_decl = get_memory_access_decl (is_write, size, > + TREE_THIS_VOLATILE(expr)); > + g = gimple_build_call (builtin_decl, 1, expr_ptr); > + } > else > { > builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE); > -- > 2.26.1.301.g55bc3eb7cb9-goog >
