On Wed, 20 May 2020 at 15:30, Martin Liška <mli...@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-el...@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 <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
>
> 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
