| Message ID | 20200423154250.10973-1-elver@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | tsan: Add optional support for distinguishing volatiles | expand |
On Thu, Apr 23, 2020 at 5:43 PM Marco Elver <elver@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_=VqTxS4nnptQ@mail.gmail.com Hi Jakub, FWIW this is: Acked-by: Dmitry Vyukov <dvuykov@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 <elver@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 <elver@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.parvathaneni@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 <elver@google.com> > + > + * c-c++-common/tsan/volatile.c: New test. > + > 2020-04-23 Jakub Jelinek <jakub@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 >
On Tue, Apr 28, 2020 at 04:48:31PM +0200, Dmitry Vyukov wrote: > FWIW this is: > > Acked-by: Dmitry Vyukov <dvuykov@google.com> > > We just landed a similar change to llvm: > https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 > > Do you have any objections? I don't have objections or anything right now, we are just trying to finalize GCC 10 and once it branches, patches like this can be reviewed/committed for GCC11. Jakub
On Tue, Apr 28, 2020 at 4:55 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Tue, Apr 28, 2020 at 04:48:31PM +0200, Dmitry Vyukov wrote: > > FWIW this is: > > > > Acked-by: Dmitry Vyukov <dvuykov@google.com> > > > > We just landed a similar change to llvm: > > https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 > > > > Do you have any objections? > > I don't have objections or anything right now, we are just trying to > finalize GCC 10 and once it branches, patches like this can be > reviewed/committed for GCC11. Thanks for clarification! Then we will just wait.
Hello, Jakub, On Tue, 28 Apr 2020 at 16:58, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Tue, Apr 28, 2020 at 4:55 PM Jakub Jelinek <jakub@redhat.com> wrote: > > > > On Tue, Apr 28, 2020 at 04:48:31PM +0200, Dmitry Vyukov wrote: > > > FWIW this is: > > > > > > Acked-by: Dmitry Vyukov <dvuykov@google.com> > > > > > > We just landed a similar change to llvm: > > > https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 > > > > > > Do you have any objections? > > > > I don't have objections or anything right now, we are just trying to > > finalize GCC 10 and once it branches, patches like this can be > > reviewed/committed for GCC11. > > Thanks for clarification! > Then we will just wait. Just saw the announcement that GCC11 is in development stage 1 [1]. In case it is still too early, do let us know what time window we shall follow up. Would it be useful to rebase and resend the patch? Many thanks, -- Marco [1] https://gcc.gnu.org/pipermail/gcc/2020-April/000505.html
On Wed, 6 May 2020 at 16:33, Marco Elver <elver@google.com> wrote: > > Hello, Jakub, > > On Tue, 28 Apr 2020 at 16:58, Dmitry Vyukov <dvyukov@google.com> wrote: > > > > On Tue, Apr 28, 2020 at 4:55 PM Jakub Jelinek <jakub@redhat.com> wrote: > > > > > > On Tue, Apr 28, 2020 at 04:48:31PM +0200, Dmitry Vyukov wrote: > > > > FWIW this is: > > > > > > > > Acked-by: Dmitry Vyukov <dvuykov@google.com> > > > > > > > > We just landed a similar change to llvm: > > > > https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 > > > > > > > > Do you have any objections? > > > > > > I don't have objections or anything right now, we are just trying to > > > finalize GCC 10 and once it branches, patches like this can be > > > reviewed/committed for GCC11. > > > > Thanks for clarification! > > Then we will just wait. > > Just saw the announcement that GCC11 is in development stage 1 [1]. In > case it is still too early, do let us know what time window we shall > follow up. > > Would it be useful to rebase and resend the patch? So, it's starting to look like we're really going to need this sooner than later. Given the feature is guarded behind a flag, and otherwise does not affect anything else, would it be possible to take this for GCC11? What do we need to do to make this happen? Thanks, -- Marco > [1] https://gcc.gnu.org/pipermail/gcc/2020-April/000505.html
On Wed, May 13, 2020 at 12:48 PM Marco Elver <elver@google.com> wrote: > > Hello, Jakub, > > > > On Tue, 28 Apr 2020 at 16:58, Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > On Tue, Apr 28, 2020 at 4:55 PM Jakub Jelinek <jakub@redhat.com> wrote: > > > > > > > > On Tue, Apr 28, 2020 at 04:48:31PM +0200, Dmitry Vyukov wrote: > > > > > FWIW this is: > > > > > > > > > > Acked-by: Dmitry Vyukov <dvuykov@google.com> > > > > > > > > > > We just landed a similar change to llvm: > > > > > https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814 > > > > > > > > > > Do you have any objections? > > > > > > > > I don't have objections or anything right now, we are just trying to > > > > finalize GCC 10 and once it branches, patches like this can be > > > > reviewed/committed for GCC11. > > > > > > Thanks for clarification! > > > Then we will just wait. > > > > Just saw the announcement that GCC11 is in development stage 1 [1]. In > > case it is still too early, do let us know what time window we shall > > follow up. > > > > Would it be useful to rebase and resend the patch? > > So, it's starting to look like we're really going to need this sooner > than later. Given the feature is guarded behind a flag, and otherwise > does not affect anything else, would it be possible to take this for > GCC11? What do we need to do to make this happen? > > Thanks, > -- Marco > > > [1] https://gcc.gnu.org/pipermail/gcc/2020-April/000505.html Jakub, could you please give some update. Do we just wait? That's fine, just want to understand because there are some interesting discussions in the kernel re bumping compiler requirements. Thanks
On 5/18/20 1:52 PM, Dmitry Vyukov via Gcc-patches wrote: > Jakub, could you please give some update. Do we just wait? That's > fine, just want to understand because there are some interesting > discussions in the kernel re bumping compiler requirements. > Thanks Hello. We switched to stage1 and we're currently working on some infrastructure changes related ChangeLog entries, -std= default change and others. If you need the patches for your fuzzing, please install them locally to a GCC 10.1.0, or to the current master. We'll review the changes as soon as possible. Martin
On 4/23/20 5:42 PM, Marco Elver via Gcc-patches wrote: Hello. Not being a maintainer of libsanitizer but I can provide a feedback: > 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'). Do you have a follow up patch that will introduce such an attribute? Does clang already have the attribute? > > [1] https://github.com/google/ktsan/wiki/KCSAN > [2] https://lkml.kernel.org/r/CANpmjNOfXNE-Zh3MNP=-gmnhvKbsfUfTtWkyg_=VqTxS4nnptQ@mail.gmail.com > > 2020-04-23 Marco Elver <elver@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 <elver@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.parvathaneni@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. You want to add 'Optimization' keyword as the parameter can be different per-TU (in LTO mode). > + > -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 <elver@google.com> > + > + * c-c++-common/tsan/volatile.c: New test. > + > 2020-04-23 Jakub Jelinek <jakub@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 Can you please add a run-time test-case that will check gd-output for TSAN error messages? > @@ -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); > And please check coding style, 8 spares are not expanded with a tab. Martin
On Wed, 20 May 2020 at 15:30, Martin Liška <mliska@suse.cz> wrote: > > On 4/23/20 5:42 PM, Marco Elver via Gcc-patches wrote: > > Hello. > > Not being a maintainer of libsanitizer but I can provide a feedback: Thank you for the review! Note, this is not touching libsanitizer or user-space TSAN runtime, only the compiler. Alternative runtimes may enable the option where required (particularly, kernel space runtimes). > > 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'). > > Do you have a follow up patch that will introduce such an attribute? Does clang > already have the attribute? Ah, sorry I wasn't clear enough here. As far as the compiler is aware, no extra attribute, so no patch for the compilers for that. It's an extra use-case, but not the main reason we need this. Re attribute, we may do: #ifdef __SANITIZE_THREAD__ #define no_data_race volatile #else #define no_data_race #endif in the kernel. It's something that was expressed by kernel maintainers, as some people want to just have a blanket annotation to make the data race detector ignore or treat certain variables as if they were atomic, even though they're not. But for all intents and purposes, please ignore the 'no_data_race' comment. The main use-case, of actually distinguishing volatile accesses is now required for KCSAN in the kernel, as without it the race detector won't work anymore after some {READ,WRITE}_ONCE() rework. Right now, KCSAN in the kernel is therefore Clang only: https://lore.kernel.org/lkml/20200515150338.190344-1-elver@google.com/ Getting this patch into GCC gets us one step closer to being able to re-enable KCSAN for GCC in the kernel, but there are some other loose ends that I don't know how to resolve (independent of this patch). [...] > > +-param=tsan-distinguish-volatile= > > +Common Joined UInteger Var(param_tsan_distinguish_volatile) IntegerRange(0, 1) Param > > +Emit special instrumentation for accesses to volatiles. > > You want to add 'Optimization' keyword as the parameter can be different > per-TU (in LTO mode). Will add in v2. > > + > > -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 <elver@google.com> > > + > > + * c-c++-common/tsan/volatile.c: New test. > > + > > 2020-04-23 Jakub Jelinek <jakub@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 > > Can you please add a run-time test-case that will check gd-output for TSAN > error messages? What do you mean? The user-space TSAN runtime itself does not make use of the option, and therefore will and should never implement __tsan_volatile*. As stated in the commit message, it's an option for alternative runtimes. Recently, the KCSAN runtime in the Linux kernel (there are also "CSAN" ports to NetBSD and FreeBSD kernels, which also had the same problem that default TSAN instrumentation doesn't distinguish volatiles). Note, we chose "CSAN" instead of "TSAN" for naming the different runtime, to avoid confusion since the runtimes function very very differently, just use the same instrumentation. (There was also a KTSAN for the kernel, but it turned out to be too complex in kernel space -- still, very little in common with the user-space runtime, just similar algorithm.) FWIW we have a test in the Linux kernel that checks the runtime, since that's where the runtime is implemented. > > @@ -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; [...] > > 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); > > > > And please check coding style, 8 spares are not expanded with a tab. Will fix for v2. Thanks, -- Marco
On 5/20/20 4:04 PM, Marco Elver wrote: > On Wed, 20 May 2020 at 15:30, Martin Liška <mliska@suse.cz> wrote: >> >> On 4/23/20 5:42 PM, Marco Elver via Gcc-patches wrote: >> >> Hello. >> >> Not being a maintainer of libsanitizer but I can provide a feedback: > > Thank you for the review! > > Note, this is not touching libsanitizer or user-space TSAN runtime, > only the compiler. Alternative runtimes may enable the option where > required (particularly, kernel space runtimes). You are right ;) Anyway, a maintainer will be needed, but Jakub promised to make a review once I'm done. > >>> 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'). >> >> Do you have a follow up patch that will introduce such an attribute? Does clang >> already have the attribute? > > Ah, sorry I wasn't clear enough here. As far as the compiler is aware, > no extra attribute, so no patch for the compilers for that. It's an > extra use-case, but not the main reason we need this. Re attribute, we > may do: > > #ifdef __SANITIZE_THREAD__ > #define no_data_race volatile > #else > #define no_data_race > #endif > > in the kernel. It's something that was expressed by kernel > maintainers, as some people want to just have a blanket annotation to > make the data race detector ignore or treat certain variables as if > they were atomic, even though they're not. But for all intents and > purposes, please ignore the 'no_data_race' comment. That's a reasonable approach for now! > > The main use-case, of actually distinguishing volatile accesses is now > required for KCSAN in the kernel, as without it the race detector > won't work anymore after some {READ,WRITE}_ONCE() rework. Right now, > KCSAN in the kernel is therefore Clang only: > https://lore.kernel.org/lkml/20200515150338.190344-1-elver@google.com/ > > Getting this patch into GCC gets us one step closer to being able to > re-enable KCSAN for GCC in the kernel, but there are some other loose > ends that I don't know how to resolve (independent of this patch). Ok. > > [...] >>> +-param=tsan-distinguish-volatile= >>> +Common Joined UInteger Var(param_tsan_distinguish_volatile) IntegerRange(0, 1) Param >>> +Emit special instrumentation for accesses to volatiles. >> >> You want to add 'Optimization' keyword as the parameter can be different >> per-TU (in LTO mode). > > Will add in v2. > >>> + >>> -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 <elver@google.com> >>> + >>> + * c-c++-common/tsan/volatile.c: New test. >>> + >>> 2020-04-23 Jakub Jelinek <jakub@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 >> >> Can you please add a run-time test-case that will check gd-output for TSAN >> error messages? > > What do you mean? The user-space TSAN runtime itself does not make use > of the option, and therefore will and should never implement > __tsan_volatile*. I've got it. So at least please add scanning of assembly or a tree dump for the expected __tsan_* calls. Martin > > As stated in the commit message, it's an option for alternative > runtimes. Recently, the KCSAN runtime in the Linux kernel (there are > also "CSAN" ports to NetBSD and FreeBSD kernels, which also had the > same problem that default TSAN instrumentation doesn't distinguish > volatiles). Note, we chose "CSAN" instead of "TSAN" for naming the > different runtime, to avoid confusion since the runtimes function very > very differently, just use the same instrumentation. (There was also a > KTSAN for the kernel, but it turned out to be too complex in kernel > space -- still, very little in common with the user-space runtime, > just similar algorithm.) > > FWIW we have a test in the Linux kernel that checks the runtime, since > that's where the runtime is implemented. > >>> @@ -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; > [...] >>> 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); >>> >> >> And please check coding style, 8 spares are not expanded with a tab. > > Will fix for v2. > > Thanks, > -- Marco >
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 <elver@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.parvathaneni@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 <elver@google.com> + + * c-c++-common/tsan/volatile.c: New test. + 2020-04-23 Jakub Jelinek <jakub@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);