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_=vqtxs4nn...@mail.gmail.com
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.
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 <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?
@@ -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
