[v3] tsan: Add optional support for distinguishing volatiles

Message ID 20200609131539.180522-1-elver@google.com
State New
Headers show
Series
  • [v3] tsan: Add optional support for distinguishing volatiles
Related show

Commit Message

Bill Schmidt via Gcc-patches June 9, 2020, 1:15 p.m.
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, and often declare volatile to be appropriate 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), is a runtime that uses
TSAN instrumentation but otherwise implements a very different approach
to race detection from TSAN:

	https://github.com/google/ktsan/wiki/KCSAN

Due to recent changes in requirements by the Linux kernel, KCSAN
requires that the compiler supports tsan-distinguish-volatile (among
several new requirements):

	https://lore.kernel.org/lkml/20200521142047.169334-7-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.

Acked-by: Dmitry Vyukov <dvyukov@google.com>

---
v3:
* Remove Optimization from -param=tsan-distinguish-volatile.
* Simplify get_memory_access_decl.
* Avoid use of builtin_decl temporary.

v2:
* Add Optimization keyword to -param=tsan-distinguish-volatile= as the
  parameter can be different per TU.
* Add tree-dump check to test.
---
 gcc/params.opt                             |  4 ++
 gcc/sanitizer.def                          | 21 +++++++
 gcc/testsuite/c-c++-common/tsan/volatile.c | 67 ++++++++++++++++++++++
 gcc/tsan.c                                 | 31 +++++-----
 4 files changed, 110 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/tsan/volatile.c

-- 
2.27.0.278.ge193c7cf3a9-goog

Comments

Bill Schmidt via Gcc-patches June 9, 2020, 1:22 p.m. | #1
On Tue, Jun 09, 2020 at 03:15:39PM +0200, Marco Elver wrote:
> 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.

> 

> Acked-by: Dmitry Vyukov <dvyukov@google.com>


Ok, thanks.

	Jakub
Bill Schmidt via Gcc-patches June 9, 2020, 1:47 p.m. | #2
On Tue, 9 Jun 2020 at 15:22, Jakub Jelinek <jakub@redhat.com> wrote:
>

> On Tue, Jun 09, 2020 at 03:15:39PM +0200, Marco Elver wrote:

> > 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.

> >

> > Acked-by: Dmitry Vyukov <dvyukov@google.com>

>

> Ok, thanks.


I think one of you has to commit the patch, as we don't have access to
the GCC git repository.

Thank you!

-- Marco
Martin Liška June 9, 2020, 7:24 p.m. | #3
On 6/9/20 3:47 PM, Marco Elver wrote:
> I think one of you has to commit the patch, as we don't have access to

> the GCC git repository.


I've just done that.
I also fixed few wrong formatting issues, next time please use ./contrib/check_GNU_style.py.

You'll be preparing one another patch, please request a write access:
https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Thanks,
Martin

Patch

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/c-c++-common/tsan/volatile.c b/gcc/testsuite/c-c++-common/tsan/volatile.c
new file mode 100644
index 00000000000..68379921685
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/tsan/volatile.c
@@ -0,0 +1,67 @@ 
+/* { dg-options "--param=tsan-distinguish-volatile=1 -fdump-tree-optimized" } */
+
+#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;
+}
+
+// { dg-final { scan-tree-dump-times "__tsan_volatile_read4 \\(&VolatileGlobal4" 2 "optimized" } }
+// { dg-final { scan-tree-dump-times "__tsan_volatile_read8 \\(&VolatileGlobal8" 2 "optimized" } }
+// { dg-final { scan-tree-dump-times "__tsan_volatile_write4 \\(&VolatileGlobal4" 2 "optimized" } }
+// { dg-final { scan-tree-dump-times "__tsan_volatile_write8 \\(&VolatileGlobal8" 2 "optimized" } }
diff --git a/gcc/tsan.c b/gcc/tsan.c
index 8d22a776377..fcb2653ebbe 100644
--- a/gcc/tsan.c
+++ b/gcc/tsan.c
@@ -52,25 +52,29 @@  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;
+  int pos;
 
   if (size <= 1)
-    fcode = is_write ? BUILT_IN_TSAN_WRITE1
-		     : BUILT_IN_TSAN_READ1;
+    pos = 0;
   else if (size <= 3)
-    fcode = is_write ? BUILT_IN_TSAN_WRITE2
-		     : BUILT_IN_TSAN_READ2;
+    pos = 1;
   else if (size <= 7)
-    fcode = is_write ? BUILT_IN_TSAN_WRITE4
-		     : BUILT_IN_TSAN_READ4;
+    pos = 2;
   else if (size <= 15)
-    fcode = is_write ? BUILT_IN_TSAN_WRITE8
-		     : BUILT_IN_TSAN_READ8;
+    pos = 3;
+  else
+    pos = 4;
+
+  if (param_tsan_distinguish_volatile && volatilep)
+    fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE1
+                     : BUILT_IN_TSAN_VOLATILE_READ1;
   else
-    fcode = is_write ? BUILT_IN_TSAN_WRITE16
-		     : BUILT_IN_TSAN_READ16;
+    fcode = is_write ? BUILT_IN_TSAN_WRITE1
+                     : BUILT_IN_TSAN_READ1;
+  fcode = (built_in_function)(fcode + pos);
 
   return builtin_decl_implicit (fcode);
 }
@@ -204,8 +208,9 @@  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);
+    g = gimple_build_call (get_memory_access_decl (is_write, size,
+                                                   TREE_THIS_VOLATILE (expr)),
+                           1, expr_ptr);
   else
     {
       builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);