On 5/20/20 4:04 PM, Marco Elver wrote:
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).
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-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).
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 <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*.
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
