[PATCH] Instrument bit field and unaligned accesses for TSAN

[PATCH] Instrument bit field and unaligned accesses for TSAN

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

Hi,

I noticed that currently the tsan pass does not instrument bit fileld accesses or unaligned data.
Thus some race conditions are not found by the sanitizer.

Furthermore the function __tsan_vptr_update is not called with the correct parameters.

This patch uses two already existing interfaces for unaligned or bit field accessses,
__tsan_read_range and __tsan_write_range, and fixes the parameters of the __tsan_vptr_update
function.

As a little surprise, the test case g++.dg/tsan/aligned_vs_unaligned_race.C started to fail at -O2
with this patch, but actually that was due to a combination of invalid C code and inverted logic
in the test case.  So actually that test was failing all the time, because TSAN cannot find
the race condition because __tsan_write8 should only be called with aligned addresses.


Boot-strapped and reggession tested on x86_64-linux-gnu.
OK for trunk?


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: changelog-tsan-bitfields.txt --]
[-- Type: text/plain, Size: 633 bytes --]

gcc/ChangeLog:
2014-12-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Instrument bit field and unaligned accesses for TSAN.
	* sanitizer.def (BUILT_IN_TSAN_READ_RANGE): New built-in function.
	(BUILT_IN_TSAN_WRITE_RANGE): New built-in function.
	* tsan.c (instrument_expr): Handle COMPONENT_REF and BIT_FIELD_REF.
	Use BUILT_IN_TSAN_READ_RANGE and BUILT_IN_TSAN_WRITE_RANGE for
	unaligned memory regions.  Pass "rhs" to BUILT_IN_TSAN_VPTR_UPDATE.

testsuite/ChangeLog:
2014-12-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/tsan/bitfield_race.c: New testcase.
	* g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed.


[-- Attachment #3: patch-tsan-bitfields.diff --]
[-- Type: application/octet-stream, Size: 7027 bytes --]

Index: gcc/sanitizer.def
===================================================================
--- gcc/sanitizer.def	(revision 218963)
+++ gcc/sanitizer.def	(working copy)
@@ -188,6 +188,10 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE8, "__tsa
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE16, "__tsan_write16",
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
+DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ_RANGE, "__tsan_read_range",
+		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
+DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE_RANGE, "__tsan_write_range",
+		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_ATOMIC8_LOAD,
 		      "__tsan_atomic8_load",
Index: gcc/tsan.c
===================================================================
--- gcc/tsan.c	(revision 218963)
+++ gcc/tsan.c	(working copy)
@@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-propagate.h"
 #include "tsan.h"
 #include "asan.h"
+#include "builtins.h"
 
 /* Number of instrumented memory accesses in the current function.  */
 
@@ -121,13 +122,12 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
   gimple stmt, g;
   gimple_seq seq;
   location_t loc;
+  unsigned int align;
 
   size = int_size_in_bytes (TREE_TYPE (expr));
-  if (size == -1)
+  if (size <= 0)
     return false;
 
-  /* For now just avoid instrumenting bit field acceses.
-     TODO: handle bit-fields as if touching the whole field.  */
   HOST_WIDE_INT bitsize, bitpos;
   tree offset;
   machine_mode mode;
@@ -155,17 +155,71 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
 	  && DECL_HARD_REGISTER (base)))
     return false;
 
-  if (size == 0
-      || bitpos % (size * BITS_PER_UNIT)
-      || bitsize != size * BITS_PER_UNIT)
-    return false;
-
   stmt = gsi_stmt (gsi);
   loc = gimple_location (stmt);
   rhs = is_vptr_store (stmt, expr, is_write);
-  gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
-  expr_ptr = build_fold_addr_expr (unshare_expr (expr));
   seq = NULL;
+
+  if ((TREE_CODE (expr) == COMPONENT_REF
+       && DECL_BIT_FIELD_TYPE (TREE_OPERAND (expr, 1)))
+      || TREE_CODE (expr) == BIT_FIELD_REF)
+    {
+      base = TREE_OPERAND (expr, 0);
+      if (TREE_CODE (expr) == COMPONENT_REF)
+	{
+	  expr = TREE_OPERAND (expr, 1);
+	  if (is_write && DECL_BIT_FIELD_REPRESENTATIVE (expr))
+	    expr = DECL_BIT_FIELD_REPRESENTATIVE (expr);
+	  if (!tree_fits_uhwi_p (DECL_FIELD_OFFSET (expr))
+	      || !tree_fits_uhwi_p (DECL_FIELD_BIT_OFFSET (expr))
+	      || !tree_fits_uhwi_p (DECL_SIZE (expr)))
+	    return false;
+	  bitpos = tree_to_uhwi (DECL_FIELD_OFFSET (expr)) * BITS_PER_UNIT
+		   + tree_to_uhwi (DECL_FIELD_BIT_OFFSET (expr));
+	  bitsize = tree_to_uhwi (DECL_SIZE (expr));
+	}
+      else
+	{
+	  if (!tree_fits_uhwi_p (TREE_OPERAND (expr, 2))
+	      || !tree_fits_uhwi_p (TREE_OPERAND (expr, 1)))
+	    return false;
+	  bitpos = tree_to_uhwi (TREE_OPERAND (expr, 2));
+	  bitsize = tree_to_uhwi (TREE_OPERAND (expr, 1));
+	}
+      if (bitpos < 0 || bitsize <= 0)
+	return false;
+      size = (bitpos % BITS_PER_UNIT + bitsize + BITS_PER_UNIT - 1)
+	     / BITS_PER_UNIT;
+      align = get_object_alignment (base);
+      if (align < BITS_PER_UNIT)
+	return false;
+      bitpos = bitpos & ~(BITS_PER_UNIT - 1);
+      if ((align - 1) & bitpos)
+	{
+	  align = (align - 1) & bitpos;
+	  align = align & -align;
+	}
+      gcc_checking_assert (is_gimple_addressable (base));
+      expr = build_fold_addr_expr (unshare_expr (base));
+      if (!is_gimple_mem_ref_addr (expr))
+	{
+	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr)), expr);
+	  expr = gimple_assign_lhs (g);
+	  gimple_set_location (g, loc);
+	  gimple_seq_add_stmt_without_update (&seq, g);
+	}
+      expr = build2 (MEM_REF, char_type_node, expr,
+		     build_int_cst (TREE_TYPE (expr), bitpos / BITS_PER_UNIT));
+      expr_ptr = build_fold_addr_expr (expr);
+    }
+  else
+    {
+      align = get_object_alignment (expr);
+      if (align < BITS_PER_UNIT)
+	return false;
+      gcc_checking_assert (is_gimple_addressable (expr));
+      expr_ptr = build_fold_addr_expr (unshare_expr (expr));
+    }
   if (!is_gimple_val (expr_ptr))
     {
       g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr_ptr)), expr_ptr);
@@ -173,13 +227,21 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
       gimple_set_location (g, loc);
       gimple_seq_add_stmt_without_update (&seq, g);
     }
-  if (rhs == NULL)
+  if ((size & -size) != size || size > 16
+      || align < MIN (size, 8) * BITS_PER_UNIT)
+    {
+      builtin_decl = builtin_decl_implicit (is_write
+					    ? BUILT_IN_TSAN_WRITE_RANGE
+					    : BUILT_IN_TSAN_READ_RANGE);
+      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);
   else
     {
       builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
-      g = gimple_build_call (builtin_decl, 1, expr_ptr);
+      g = gimple_build_call (builtin_decl, 2, expr_ptr, unshare_expr (rhs));
     }
   gimple_set_location (g, loc);
   gimple_seq_add_stmt_without_update (&seq, g);
Index: gcc/testsuite/c-c++-common/tsan/bitfield_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(revision 0)
+++ gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(working copy)
@@ -0,0 +1,26 @@
+/* { dg-shouldfail "tsan" } */
+
+#include <pthread.h>
+#include <unistd.h>
+
+struct bitfield
+{
+  int a:10;
+  int b:10;
+} Global;
+
+void *Thread1(void *x) {
+  sleep(1);
+  Global.a = 42;
+  return x;
+}
+
+int main() {
+  pthread_t t;
+  pthread_create(&t, 0, Thread1, 0);
+  Global.b = 43;
+  pthread_join(t, 0);
+  return Global.a;
+}
+
+/* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
Index: gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(revision 218963)
+++ gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(working copy)
@@ -1,3 +1,4 @@
+/* { dg-shouldfail "tsan" } */
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -11,8 +12,9 @@ void *Thread1(void *x) {
 
 void *Thread2(void *x) {
   char *p1 = reinterpret_cast<char *>(&Global[0]);
-  uint64_t *p4 = reinterpret_cast<uint64_t *>(p1 + 1);
-  (*p4)++;
+  struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
+  u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
+  (*p4).val++;
   return NULL;
 }
 
@@ -23,7 +25,7 @@ int main() {
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
   printf("Pass\n");
-  /* { dg-prune-output "ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
+  /* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
   /* { dg-output "Pass.*" } */
   return 0;
 }

Re: [PATCH] Instrument bit field and unaligned accesses for TSAN

[Jakub Jelinek]

On Mon, Dec 29, 2014 at 09:20:57PM +0100, Bernd Edlinger wrote:

> --- gcc/sanitizer.def	(revision 218963)
> +++ gcc/sanitizer.def	(working copy)
> @@ -188,6 +188,10 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE8, "__tsa
>  		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>  DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE16, "__tsan_write16",
>  		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ_RANGE, "__tsan_read_range",
> +		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE_RANGE, "__tsan_write_range",
> +		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>  
>  DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_ATOMIC8_LOAD,
>  		      "__tsan_atomic8_load",

For the BUILT_IN_VPTR_UPDATE builtin you also need to change
the prototype, to BT_FN_VOID_PTR_PTR.  Do you have a testcase for the
__tsan_vptr_update bug?  Can you submit it separately, because it
probably is desirable also for the 4.9 and 4.8 branches.

> @@ -173,13 +227,21 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
>        gimple_set_location (g, loc);
>        gimple_seq_add_stmt_without_update (&seq, g);
>      }
> -  if (rhs == NULL)
> +  if ((size & -size) != size || size > 16

Isn't (size & (size - 1)) == 0 a better check?

Otherwise LGTM.

	Jakub

RE: [PATCH] Instrument bit field and unaligned accesses for TSAN

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]



On Fri, 2 Jan 2015 20:01:02, Jakub Jelinke wrote:
>
> On Mon, Dec 29, 2014 at 09:20:57PM +0100, Bernd Edlinger wrote:
>
>> --- gcc/sanitizer.def (revision 218963)
>> +++ gcc/sanitizer.def (working copy)
>> @@ -188,6 +188,10 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE8, "__tsa
>> BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>> DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE16, "__tsan_write16",
>> BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ_RANGE, "__tsan_read_range",
>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE_RANGE, "__tsan_write_range",
>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>
>> DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_ATOMIC8_LOAD,
>> "__tsan_atomic8_load",
>
> For the BUILT_IN_VPTR_UPDATE builtin you also need to change
> the prototype, to BT_FN_VOID_PTR_PTR. Do you have a testcase for the
> __tsan_vptr_update bug? Can you submit it separately, because it
> probably is desirable also for the 4.9 and 4.8 branches.
>

OK, I will do that.
I removed the __tsan_vptr_update stuff from the patch, for now.

It will probably be difficult for me to find a test case for this,
because I am not really sure what __tsan_vptr_update is actually good for,
(i.e. the use case).


>> @@ -173,13 +227,21 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
>> gimple_set_location (g, loc);
>> gimple_seq_add_stmt_without_update (&seq, g);
>> }
>> - if (rhs == NULL)
>> + if ((size & -size) != size || size> 16
>
> Isn't (size & (size - 1)) == 0 a better check?
>

Yes, that's of course better, Thanks.

I think I should also change the function type of BUILT_IN_READ_RANGE
and BUILT_IN_WRITE_RANGE to BT_FN_VOID_PTR_SIZE?

I attached the updated version of the patch.

Is that OK too?

Thanks
Bernd.

> Otherwise LGTM.
>
> Jakub
 		 	   		  

[-- Attachment #2: changelog-tsan-bitfields.txt --]
[-- Type: text/plain, Size: 674 bytes --]

gcc/ChangeLog:
2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Instrument bit field and unaligned accesses for TSAN.
	* asan.c (initialize_sanitizer_builtins): New function type
	BT_FN_VOID_PTR_SIZE.
	* sanitizer.def (BUILT_IN_TSAN_READ_RANGE): New built-in function.
	(BUILT_IN_TSAN_WRITE_RANGE): New built-in function.
	* tsan.c (instrument_expr): Handle COMPONENT_REF and BIT_FIELD_REF.
	Use BUILT_IN_TSAN_READ_RANGE and BUILT_IN_TSAN_WRITE_RANGE for
	unaligned memory regions.

testsuite/ChangeLog:
2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/tsan/bitfield_race.c: New testcase.
	* g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed.


[-- Attachment #3: patch-tsan-bitfields.diff --]
[-- Type: application/octet-stream, Size: 7376 bytes --]

Index: gcc/asan.c
===================================================================
--- gcc/asan.c	(revision 219135)
+++ gcc/asan.c	(working copy)
@@ -2278,6 +2278,9 @@ initialize_sanitizer_builtins (void)
     = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE);
   tree BT_FN_VOID_CONST_PTR
     = build_function_type_list (void_type_node, const_ptr_type_node, NULL_TREE);
+  tree BT_FN_VOID_PTR_SIZE
+    = build_function_type_list (void_type_node, ptr_type_node,
+				size_type_node, NULL_TREE);
   tree BT_FN_VOID_PTR_PTR
     = build_function_type_list (void_type_node, ptr_type_node,
 				ptr_type_node, NULL_TREE);
Index: gcc/sanitizer.def
===================================================================
--- gcc/sanitizer.def	(revision 219135)
+++ gcc/sanitizer.def	(working copy)
@@ -188,6 +188,10 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE8, "__tsa
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE16, "__tsan_write16",
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
+DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ_RANGE, "__tsan_read_range",
+		      BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
+DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE_RANGE, "__tsan_write_range",
+		      BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST)
 
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_ATOMIC8_LOAD,
 		      "__tsan_atomic8_load",
Index: gcc/tsan.c
===================================================================
--- gcc/tsan.c	(revision 219135)
+++ gcc/tsan.c	(working copy)
@@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-propagate.h"
 #include "tsan.h"
 #include "asan.h"
+#include "builtins.h"
 
 /* Number of instrumented memory accesses in the current function.  */
 
@@ -121,13 +122,12 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
   gimple stmt, g;
   gimple_seq seq;
   location_t loc;
+  unsigned int align;
 
   size = int_size_in_bytes (TREE_TYPE (expr));
-  if (size == -1)
+  if (size <= 0)
     return false;
 
-  /* For now just avoid instrumenting bit field acceses.
-     TODO: handle bit-fields as if touching the whole field.  */
   HOST_WIDE_INT bitsize, bitpos;
   tree offset;
   machine_mode mode;
@@ -155,17 +155,71 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
 	  && DECL_HARD_REGISTER (base)))
     return false;
 
-  if (size == 0
-      || bitpos % (size * BITS_PER_UNIT)
-      || bitsize != size * BITS_PER_UNIT)
-    return false;
-
   stmt = gsi_stmt (gsi);
   loc = gimple_location (stmt);
   rhs = is_vptr_store (stmt, expr, is_write);
-  gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
-  expr_ptr = build_fold_addr_expr (unshare_expr (expr));
   seq = NULL;
+
+  if ((TREE_CODE (expr) == COMPONENT_REF
+       && DECL_BIT_FIELD_TYPE (TREE_OPERAND (expr, 1)))
+      || TREE_CODE (expr) == BIT_FIELD_REF)
+    {
+      base = TREE_OPERAND (expr, 0);
+      if (TREE_CODE (expr) == COMPONENT_REF)
+	{
+	  expr = TREE_OPERAND (expr, 1);
+	  if (is_write && DECL_BIT_FIELD_REPRESENTATIVE (expr))
+	    expr = DECL_BIT_FIELD_REPRESENTATIVE (expr);
+	  if (!tree_fits_uhwi_p (DECL_FIELD_OFFSET (expr))
+	      || !tree_fits_uhwi_p (DECL_FIELD_BIT_OFFSET (expr))
+	      || !tree_fits_uhwi_p (DECL_SIZE (expr)))
+	    return false;
+	  bitpos = tree_to_uhwi (DECL_FIELD_OFFSET (expr)) * BITS_PER_UNIT
+		   + tree_to_uhwi (DECL_FIELD_BIT_OFFSET (expr));
+	  bitsize = tree_to_uhwi (DECL_SIZE (expr));
+	}
+      else
+	{
+	  if (!tree_fits_uhwi_p (TREE_OPERAND (expr, 2))
+	      || !tree_fits_uhwi_p (TREE_OPERAND (expr, 1)))
+	    return false;
+	  bitpos = tree_to_uhwi (TREE_OPERAND (expr, 2));
+	  bitsize = tree_to_uhwi (TREE_OPERAND (expr, 1));
+	}
+      if (bitpos < 0 || bitsize <= 0)
+	return false;
+      size = (bitpos % BITS_PER_UNIT + bitsize + BITS_PER_UNIT - 1)
+	     / BITS_PER_UNIT;
+      align = get_object_alignment (base);
+      if (align < BITS_PER_UNIT)
+	return false;
+      bitpos = bitpos & ~(BITS_PER_UNIT - 1);
+      if ((align - 1) & bitpos)
+	{
+	  align = (align - 1) & bitpos;
+	  align = align & -align;
+	}
+      gcc_checking_assert (is_gimple_addressable (base));
+      expr = build_fold_addr_expr (unshare_expr (base));
+      if (!is_gimple_mem_ref_addr (expr))
+	{
+	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr)), expr);
+	  expr = gimple_assign_lhs (g);
+	  gimple_set_location (g, loc);
+	  gimple_seq_add_stmt_without_update (&seq, g);
+	}
+      expr = build2 (MEM_REF, char_type_node, expr,
+		     build_int_cst (TREE_TYPE (expr), bitpos / BITS_PER_UNIT));
+      expr_ptr = build_fold_addr_expr (expr);
+    }
+  else
+    {
+      align = get_object_alignment (expr);
+      if (align < BITS_PER_UNIT)
+	return false;
+      gcc_checking_assert (is_gimple_addressable (expr));
+      expr_ptr = build_fold_addr_expr (unshare_expr (expr));
+    }
   if (!is_gimple_val (expr_ptr))
     {
       g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr_ptr)), expr_ptr);
@@ -173,7 +227,15 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
       gimple_set_location (g, loc);
       gimple_seq_add_stmt_without_update (&seq, g);
     }
-  if (rhs == NULL)
+  if ((size & (size - 1)) == 0 || size > 16
+      || align < MIN (size, 8) * BITS_PER_UNIT)
+    {
+      builtin_decl = builtin_decl_implicit (is_write
+					    ? BUILT_IN_TSAN_WRITE_RANGE
+					    : BUILT_IN_TSAN_READ_RANGE);
+      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);
   else
Index: gcc/testsuite/c-c++-common/tsan/bitfield_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(revision 0)
+++ gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(working copy)
@@ -0,0 +1,26 @@
+/* { dg-shouldfail "tsan" } */
+
+#include <pthread.h>
+#include <unistd.h>
+
+struct bitfield
+{
+  int a:10;
+  int b:10;
+} Global;
+
+void *Thread1(void *x) {
+  sleep(1);
+  Global.a = 42;
+  return x;
+}
+
+int main() {
+  pthread_t t;
+  pthread_create(&t, 0, Thread1, 0);
+  Global.b = 43;
+  pthread_join(t, 0);
+  return Global.a;
+}
+
+/* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
Index: gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(revision 219135)
+++ gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(working copy)
@@ -1,3 +1,4 @@
+/* { dg-shouldfail "tsan" } */
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -11,8 +12,9 @@ void *Thread1(void *x) {
 
 void *Thread2(void *x) {
   char *p1 = reinterpret_cast<char *>(&Global[0]);
-  uint64_t *p4 = reinterpret_cast<uint64_t *>(p1 + 1);
-  (*p4)++;
+  struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
+  u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
+  (*p4).val++;
   return NULL;
 }
 
@@ -23,7 +25,7 @@ int main() {
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
   printf("Pass\n");
-  /* { dg-prune-output "ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
+  /* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
   /* { dg-output "Pass.*" } */
   return 0;
 }

Re: [PATCH] Instrument bit field and unaligned accesses for TSAN

[Jakub Jelinek]

On Fri, Jan 02, 2015 at 10:06:29PM +0100, Bernd Edlinger wrote:
> OK, I will do that.
> I removed the __tsan_vptr_update stuff from the patch, for now.

Guess we should ask Dmitry about that.

> It will probably be difficult for me to find a test case for this,
> because I am not really sure what __tsan_vptr_update is actually good for,
> (i.e. the use case).

> I think I should also change the function type of BUILT_IN_READ_RANGE
> and BUILT_IN_WRITE_RANGE to BT_FN_VOID_PTR_SIZE?

You should use BT_FN_VOID_PTR_PTRMODE for those instead, and you don't need to
initialize it in asan.c - it is already initialized there.
size_t might be a different integer type from uptr.

	Jakub

RE: [PATCH] Instrument bit field and unaligned accesses for TSAN

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]


Hi,

On Fri, 2 Jan 2015 22:29:01, Jakub Jelinek wrote:
>
> On Fri, Jan 02, 2015 at 10:06:29PM +0100, Bernd Edlinger wrote:
>> OK, I will do that.
>> I removed the __tsan_vptr_update stuff from the patch, for now.
>
> Guess we should ask Dmitry about that.
>
>> It will probably be difficult for me to find a test case for this,
>> because I am not really sure what __tsan_vptr_update is actually good for,
>> (i.e. the use case).
>
>> I think I should also change the function type of BUILT_IN_READ_RANGE
>> and BUILT_IN_WRITE_RANGE to BT_FN_VOID_PTR_SIZE?
>
> You should use BT_FN_VOID_PTR_PTRMODE for those instead, and you don't need to
> initialize it in asan.c - it is already initialized there.
> size_t might be a different integer type from uptr.
>
> Jakub

Dmitry,
while I wrote this patch, I became aware of a minor flaw in GCC's instrumentation
of __tsan_vptr_update. That is, currently we pass only the address of the vptr that is about to be written
to this function, but the second parameter, the value that will be written, is always garbage.
Jakub asked for a test case, but I am not sure about what is the use case, where this would
make a difference.  Can you shed some light on this?  Thanks.

Jakub,
I changed the function type to BT_FN_VOID_PTR_PTRMODE, is the patch OK now?


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: changelog-tsan-bitfields.txt --]
[-- Type: text/plain, Size: 591 bytes --]

gcc/ChangeLog:
2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	Instrument bit field and unaligned accesses for TSAN.
	* sanitizer.def (BUILT_IN_TSAN_READ_RANGE): New built-in function.
	(BUILT_IN_TSAN_WRITE_RANGE): New built-in function.
	* tsan.c (instrument_expr): Handle COMPONENT_REF and BIT_FIELD_REF.
	Use BUILT_IN_TSAN_READ_RANGE and BUILT_IN_TSAN_WRITE_RANGE for
	unaligned memory regions.

testsuite/ChangeLog:
2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/tsan/bitfield_race.c: New testcase.
	* g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed.


[-- Attachment #3: patch-tsan-bitfields.diff --]
[-- Type: application/octet-stream, Size: 6739 bytes --]

Index: gcc/sanitizer.def
===================================================================
--- gcc/sanitizer.def	(revision 219148)
+++ gcc/sanitizer.def	(working copy)
@@ -188,6 +188,10 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE8, "__tsa
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE16, "__tsan_write16",
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
+DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ_RANGE, "__tsan_read_range",
+		      BT_FN_VOID_PTR_PTRMODE, ATTR_NOTHROW_LEAF_LIST)
+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_ATOMIC8_LOAD,
 		      "__tsan_atomic8_load",
Index: gcc/tsan.c
===================================================================
--- gcc/tsan.c	(revision 219148)
+++ gcc/tsan.c	(working copy)
@@ -62,6 +62,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-propagate.h"
 #include "tsan.h"
 #include "asan.h"
+#include "builtins.h"
 
 /* Number of instrumented memory accesses in the current function.  */
 
@@ -121,13 +122,12 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
   gimple stmt, g;
   gimple_seq seq;
   location_t loc;
+  unsigned int align;
 
   size = int_size_in_bytes (TREE_TYPE (expr));
-  if (size == -1)
+  if (size <= 0)
     return false;
 
-  /* For now just avoid instrumenting bit field acceses.
-     TODO: handle bit-fields as if touching the whole field.  */
   HOST_WIDE_INT bitsize, bitpos;
   tree offset;
   machine_mode mode;
@@ -155,17 +155,71 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
 	  && DECL_HARD_REGISTER (base)))
     return false;
 
-  if (size == 0
-      || bitpos % (size * BITS_PER_UNIT)
-      || bitsize != size * BITS_PER_UNIT)
-    return false;
-
   stmt = gsi_stmt (gsi);
   loc = gimple_location (stmt);
   rhs = is_vptr_store (stmt, expr, is_write);
-  gcc_checking_assert (rhs != NULL || is_gimple_addressable (expr));
-  expr_ptr = build_fold_addr_expr (unshare_expr (expr));
   seq = NULL;
+
+  if ((TREE_CODE (expr) == COMPONENT_REF
+       && DECL_BIT_FIELD_TYPE (TREE_OPERAND (expr, 1)))
+      || TREE_CODE (expr) == BIT_FIELD_REF)
+    {
+      base = TREE_OPERAND (expr, 0);
+      if (TREE_CODE (expr) == COMPONENT_REF)
+	{
+	  expr = TREE_OPERAND (expr, 1);
+	  if (is_write && DECL_BIT_FIELD_REPRESENTATIVE (expr))
+	    expr = DECL_BIT_FIELD_REPRESENTATIVE (expr);
+	  if (!tree_fits_uhwi_p (DECL_FIELD_OFFSET (expr))
+	      || !tree_fits_uhwi_p (DECL_FIELD_BIT_OFFSET (expr))
+	      || !tree_fits_uhwi_p (DECL_SIZE (expr)))
+	    return false;
+	  bitpos = tree_to_uhwi (DECL_FIELD_OFFSET (expr)) * BITS_PER_UNIT
+		   + tree_to_uhwi (DECL_FIELD_BIT_OFFSET (expr));
+	  bitsize = tree_to_uhwi (DECL_SIZE (expr));
+	}
+      else
+	{
+	  if (!tree_fits_uhwi_p (TREE_OPERAND (expr, 2))
+	      || !tree_fits_uhwi_p (TREE_OPERAND (expr, 1)))
+	    return false;
+	  bitpos = tree_to_uhwi (TREE_OPERAND (expr, 2));
+	  bitsize = tree_to_uhwi (TREE_OPERAND (expr, 1));
+	}
+      if (bitpos < 0 || bitsize <= 0)
+	return false;
+      size = (bitpos % BITS_PER_UNIT + bitsize + BITS_PER_UNIT - 1)
+	     / BITS_PER_UNIT;
+      align = get_object_alignment (base);
+      if (align < BITS_PER_UNIT)
+	return false;
+      bitpos = bitpos & ~(BITS_PER_UNIT - 1);
+      if ((align - 1) & bitpos)
+	{
+	  align = (align - 1) & bitpos;
+	  align = align & -align;
+	}
+      gcc_checking_assert (is_gimple_addressable (base));
+      expr = build_fold_addr_expr (unshare_expr (base));
+      if (!is_gimple_mem_ref_addr (expr))
+	{
+	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr)), expr);
+	  expr = gimple_assign_lhs (g);
+	  gimple_set_location (g, loc);
+	  gimple_seq_add_stmt_without_update (&seq, g);
+	}
+      expr = build2 (MEM_REF, char_type_node, expr,
+		     build_int_cst (TREE_TYPE (expr), bitpos / BITS_PER_UNIT));
+      expr_ptr = build_fold_addr_expr (expr);
+    }
+  else
+    {
+      align = get_object_alignment (expr);
+      if (align < BITS_PER_UNIT)
+	return false;
+      gcc_checking_assert (is_gimple_addressable (expr));
+      expr_ptr = build_fold_addr_expr (unshare_expr (expr));
+    }
   if (!is_gimple_val (expr_ptr))
     {
       g = gimple_build_assign (make_ssa_name (TREE_TYPE (expr_ptr)), expr_ptr);
@@ -173,7 +227,15 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
       gimple_set_location (g, loc);
       gimple_seq_add_stmt_without_update (&seq, g);
     }
-  if (rhs == NULL)
+  if ((size & (size - 1)) == 0 || size > 16
+      || align < MIN (size, 8) * BITS_PER_UNIT)
+    {
+      builtin_decl = builtin_decl_implicit (is_write
+					    ? BUILT_IN_TSAN_WRITE_RANGE
+					    : BUILT_IN_TSAN_READ_RANGE);
+      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);
   else
Index: gcc/testsuite/c-c++-common/tsan/bitfield_race.c
===================================================================
--- gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(revision 0)
+++ gcc/testsuite/c-c++-common/tsan/bitfield_race.c	(working copy)
@@ -0,0 +1,26 @@
+/* { dg-shouldfail "tsan" } */
+
+#include <pthread.h>
+#include <unistd.h>
+
+struct bitfield
+{
+  int a:10;
+  int b:10;
+} Global;
+
+void *Thread1(void *x) {
+  sleep(1);
+  Global.a = 42;
+  return x;
+}
+
+int main() {
+  pthread_t t;
+  pthread_create(&t, 0, Thread1, 0);
+  Global.b = 43;
+  pthread_join(t, 0);
+  return Global.a;
+}
+
+/* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
Index: gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(revision 219148)
+++ gcc/testsuite/g++.dg/tsan/aligned_vs_unaligned_race.C	(working copy)
@@ -1,3 +1,4 @@
+/* { dg-shouldfail "tsan" } */
 #include <pthread.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -11,8 +12,9 @@ void *Thread1(void *x) {
 
 void *Thread2(void *x) {
   char *p1 = reinterpret_cast<char *>(&Global[0]);
-  uint64_t *p4 = reinterpret_cast<uint64_t *>(p1 + 1);
-  (*p4)++;
+  struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
+  u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
+  (*p4).val++;
   return NULL;
 }
 
@@ -23,7 +25,7 @@ int main() {
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
   printf("Pass\n");
-  /* { dg-prune-output "ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
+  /* { dg-output "WARNING: ThreadSanitizer: data race.*(\n|\r\n|\r)" } */
   /* { dg-output "Pass.*" } */
   return 0;
 }

Re: [PATCH] Instrument bit field and unaligned accesses for TSAN

[Jakub Jelinek]

On Fri, Jan 02, 2015 at 11:01:56PM +0100, Bernd Edlinger wrote:
> gcc/ChangeLog:
> 2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	Instrument bit field and unaligned accesses for TSAN.
> 	* sanitizer.def (BUILT_IN_TSAN_READ_RANGE): New built-in function.
> 	(BUILT_IN_TSAN_WRITE_RANGE): New built-in function.
> 	* tsan.c (instrument_expr): Handle COMPONENT_REF and BIT_FIELD_REF.
> 	Use BUILT_IN_TSAN_READ_RANGE and BUILT_IN_TSAN_WRITE_RANGE for
> 	unaligned memory regions.
> 
> testsuite/ChangeLog:
> 2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* c-c++-common/tsan/bitfield_race.c: New testcase.
> 	* g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed.

Ok for trunk.

	Jakub

Re: [PATCH] Instrument bit field and unaligned accesses for TSAN

[Dmitry Vyukov]

Cool! Thanks.
Gcc is again ahead of clang :)



On Sat, Jan 3, 2015 at 1:11 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jan 02, 2015 at 11:01:56PM +0100, Bernd Edlinger wrote:
>> gcc/ChangeLog:
>> 2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>>       Instrument bit field and unaligned accesses for TSAN.
>>       * sanitizer.def (BUILT_IN_TSAN_READ_RANGE): New built-in function.
>>       (BUILT_IN_TSAN_WRITE_RANGE): New built-in function.
>>       * tsan.c (instrument_expr): Handle COMPONENT_REF and BIT_FIELD_REF.
>>       Use BUILT_IN_TSAN_READ_RANGE and BUILT_IN_TSAN_WRITE_RANGE for
>>       unaligned memory regions.
>>
>> testsuite/ChangeLog:
>> 2015-01-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>>       * c-c++-common/tsan/bitfield_race.c: New testcase.
>>       * g++.dg/tsan/aligned_vs_unaligned_race.C: Fixed.
>
> Ok for trunk.
>
>         Jakub

[PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

Hi Jakub,


I think I have found a reasonable test case, see the attached patch file.
The use case is: a class that destroys an owned thread in the destructor.
The destructor sets the vptr again to thread::vptr but this should
_not_ trigger a diagnostic message, when the vptr does not really change.

Jakub, this is another test case where the TREE_READONLY prevents
the tsan instrumentation.  So I had first to install your patch:

https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01432.html

... to see the test case fail without my patch.

The patch installs cleanly on 4.9 and 4.8, however the 4.8 branch
has no tsan tests, so I would leave the test case away for 4.8.

Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk and 4.9 + 4.8 branches?


Thanks
Bernd.

 		 	   		  

[-- Attachment #2: changelog-tsan-vptr.txt --]
[-- Type: text/plain, Size: 330 bytes --]

gcc/ChangeLog:
2015-01-04  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* sanititer.c (BUILT_IN_TSAN_VPTR_UPDATE): Fixed parameters.
	* tsan.c (instrument_expr): Fixed parameters of __tsan_vptr_update.

gcc/testsuite/ChangeLog
2015-01-04  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* g++.dg/tsan/vptr_update.C: New testcase.


[-- Attachment #3: patch-tsan-vptr.diff --]
[-- Type: application/octet-stream, Size: 1936 bytes --]

Index: gcc/sanitizer.def
===================================================================
--- gcc/sanitizer.def	(revision 219160)
+++ gcc/sanitizer.def	(working copy)
@@ -167,7 +167,7 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_FUNC_ENTRY, "_
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_FUNC_EXIT, "__tsan_func_exit",
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VPTR_UPDATE, "__tsan_vptr_update",
-		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
+		      BT_FN_VOID_PTR_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ1, "__tsan_read1",
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ2, "__tsan_read2",
Index: gcc/tsan.c
===================================================================
--- gcc/tsan.c	(revision 219160)
+++ gcc/tsan.c	(working copy)
@@ -241,7 +241,7 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
   else
     {
       builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
-      g = gimple_build_call (builtin_decl, 1, expr_ptr);
+      g = gimple_build_call (builtin_decl, 2, expr_ptr, unshare_expr (rhs));
     }
   gimple_set_location (g, loc);
   gimple_seq_add_stmt_without_update (&seq, g);
Index: gcc/testsuite/g++.dg/tsan/vptr_update.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/vptr_update.C	(revision 0)
+++ gcc/testsuite/g++.dg/tsan/vptr_update.C	(working copy)
@@ -0,0 +1,39 @@
+#include <pthread.h>
+
+class thread
+{
+public:
+  thread ()
+  {
+    count = 0;
+    pthread_create (&th, 0, tf, this);
+  }
+
+  virtual ~thread ()
+  {
+    pthread_join (th, 0);
+  }
+
+  virtual void tick ()
+  {
+    count++;
+  }
+
+private:
+  pthread_t th;
+  int count;
+
+  static void* tf(void* arg)
+  {
+    thread* t = (thread*) arg;
+    t->tick();
+    return 0;
+  }
+};
+
+int
+main ()
+{
+  thread *t = new thread;
+  delete t;
+}

RE: [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

Hi,



On Sun, 4 Jan 2015 14:54:56, Bernd Edlinger wrote:
>
> Hi Jakub,
>
>
> I think I have found a reasonable test case, see the attached patch file.
> The use case is: a class that destroys an owned thread in the destructor.
> The destructor sets the vptr again to thread::vptr but this should
> _not_ trigger a diagnostic message, when the vptr does not really change.
>
> Jakub, this is another test case where the TREE_READONLY prevents
> the tsan instrumentation.  So I had first to install your patch:
>
> https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01432.html
>
> ... to see the test case fail without my patch.
>

that has been installed in the meantime.

> The patch installs cleanly on 4.9 and 4.8, however the 4.8 branch
> has no tsan tests, so I would leave the test case away for 4.8.
>

I found, 4.8 does not have BT_FN_VOID_PTR_PTR, and no tsan tests
at all, so it is probably not worth the effort.

> Boot-strapped and regression-tested on x86_64-linux-gnu
> OK for trunk and 4.9 + 4.8 branches?
>
>
> Thanks
> Bernd.
>
>

I found some test cases in the clang tree, about the __tsan_vptr_update.
So I thought I should use these instead of inventing new ones.

Attached you'll find an updated patch with one positive and one negative
test for vptr races.

Tested with x86_64-linux-gnu.
OK for trunk and 4.9 after a while?


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: changelog-tsan-vptr.txt --]
[-- Type: text/plain, Size: 385 bytes --]

gcc/ChangeLog:
2015-01-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* sanititer.c (BUILT_IN_TSAN_VPTR_UPDATE): Fixed parameters.
	* tsan.c (instrument_expr): Fixed parameters of __tsan_vptr_update.

gcc/testsuite/ChangeLog
2015-01-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* g++.dg/tsan/vptr_benign_race.C: New testcase.
	* g++.dg/tsan/vptr_harmful_race.C: New testcase.


[-- Attachment #3: patch-tsan-vptr.diff --]
[-- Type: application/octet-stream, Size: 3562 bytes --]

Index: gcc/sanitizer.def
===================================================================
--- gcc/sanitizer.def	(revision 219160)
+++ gcc/sanitizer.def	(working copy)
@@ -167,7 +167,7 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_FUNC_ENTRY, "_
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_FUNC_EXIT, "__tsan_func_exit",
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VPTR_UPDATE, "__tsan_vptr_update",
-		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
+		      BT_FN_VOID_PTR_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ1, "__tsan_read1",
 		      BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
 DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ2, "__tsan_read2",
Index: gcc/tsan.c
===================================================================
--- gcc/tsan.c	(revision 219160)
+++ gcc/tsan.c	(working copy)
@@ -241,7 +241,7 @@ instrument_expr (gimple_stmt_iterator gsi, tree ex
   else
     {
       builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
-      g = gimple_build_call (builtin_decl, 1, expr_ptr);
+      g = gimple_build_call (builtin_decl, 2, expr_ptr, unshare_expr (rhs));
     }
   gimple_set_location (g, loc);
   gimple_seq_add_stmt_without_update (&seq, g);
Index: gcc/testsuite/g++.dg/tsan/vptr_benign_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/vptr_benign_race.C	(revision 0)
+++ gcc/testsuite/g++.dg/tsan/vptr_benign_race.C	(working copy)
@@ -0,0 +1,49 @@
+#include <pthread.h>
+#include <semaphore.h>
+#include <stdio.h>
+
+struct A {
+  A() {
+    sem_init(&sem_, 0, 0);
+  }
+  virtual void F() {
+  }
+  void Done() {
+    sem_post(&sem_);
+  }
+  virtual ~A() {
+  }
+  sem_t sem_;
+};
+
+struct B : A {
+  virtual void F() {
+  }
+  virtual ~B() {
+    sem_wait(&sem_);
+    sem_destroy(&sem_);
+  }
+};
+
+static A *obj = new B;
+
+void *Thread1(void *x) {
+  obj->F();
+  obj->Done();
+  return NULL;
+}
+
+void *Thread2(void *x) {
+  delete obj;
+  return NULL;
+}
+
+int main() {
+  pthread_t t[2];
+  pthread_create(&t[0], NULL, Thread1, NULL);
+  pthread_create(&t[1], NULL, Thread2, NULL);
+  pthread_join(t[0], NULL);
+  pthread_join(t[1], NULL);
+  fprintf(stderr, "PASS\n");
+}
+/* { dg-output "PASS.*" } */
Index: gcc/testsuite/g++.dg/tsan/vptr_harmful_race.C
===================================================================
--- gcc/testsuite/g++.dg/tsan/vptr_harmful_race.C	(revision 0)
+++ gcc/testsuite/g++.dg/tsan/vptr_harmful_race.C	(working copy)
@@ -0,0 +1,58 @@
+/* { dg-shouldfail "tsan" } */
+/* { dg-additional-options "-ldl" } */
+
+#include <pthread.h>
+#include <semaphore.h>
+#include <stdio.h>
+#include <unistd.h>
+#include "tsan_barrier.h"
+
+static pthread_barrier_t barrier;
+
+struct A {
+  A() {
+    sem_init(&sem_, 0, 0);
+  }
+  virtual void F() {
+  }
+  void Done() {
+    sem_post(&sem_);
+  }
+  virtual ~A() {
+    sem_wait(&sem_);
+    sem_destroy(&sem_);
+  }
+  sem_t sem_;
+};
+
+struct B : A {
+  virtual void F() {
+  }
+  virtual ~B() { }
+};
+
+static A *obj = new B;
+
+void *Thread1(void *x) {
+  obj->F();
+  obj->Done();
+  barrier_wait(&barrier);
+  return NULL;
+}
+
+void *Thread2(void *x) {
+  barrier_wait(&barrier);
+  delete obj;
+  return NULL;
+}
+
+int main() {
+  barrier_init(&barrier, 2);
+  pthread_t t[2];
+  pthread_create(&t[0], NULL, Thread1, NULL);
+  pthread_create(&t[1], NULL, Thread2, NULL);
+  pthread_join(t[0], NULL);
+  pthread_join(t[1], NULL);
+}
+
+/* { dg-output "WARNING: ThreadSanitizer: data race on vptr.*(\n|\r\n|\r)" } */

Re: [PATCH] Fix parameters of __tsan_vptr_update

[Dmitry Vyukov]

Hi,

Sorry for the delay, I was on a vacation. Is here anything to do/review for me?


On Sun, Jan 11, 2015 at 4:15 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
>
>
> On Sun, 4 Jan 2015 14:54:56, Bernd Edlinger wrote:
>>
>> Hi Jakub,
>>
>>
>> I think I have found a reasonable test case, see the attached patch file.
>> The use case is: a class that destroys an owned thread in the destructor.
>> The destructor sets the vptr again to thread::vptr but this should
>> _not_ trigger a diagnostic message, when the vptr does not really change.
>>
>> Jakub, this is another test case where the TREE_READONLY prevents
>> the tsan instrumentation.  So I had first to install your patch:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01432.html
>>
>> ... to see the test case fail without my patch.
>>
>
> that has been installed in the meantime.
>
>> The patch installs cleanly on 4.9 and 4.8, however the 4.8 branch
>> has no tsan tests, so I would leave the test case away for 4.8.
>>
>
> I found, 4.8 does not have BT_FN_VOID_PTR_PTR, and no tsan tests
> at all, so it is probably not worth the effort.
>
>> Boot-strapped and regression-tested on x86_64-linux-gnu
>> OK for trunk and 4.9 + 4.8 branches?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>
> I found some test cases in the clang tree, about the __tsan_vptr_update.
> So I thought I should use these instead of inventing new ones.
>
> Attached you'll find an updated patch with one positive and one negative
> test for vptr races.
>
> Tested with x86_64-linux-gnu.
> OK for trunk and 4.9 after a while?
>
>
> Thanks
> Bernd.
>

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

Hi,


I think I should ping for this patch now:
https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00599.html

note that by mistake the change log referenced sanitizer.c instead of
sanitizer.def, consider that fixed on my local copy.


Thanks
Bernd.


> Date: Sun, 11 Jan 2015 14:15:54 +0100
>
> Hi,
>
>
>
> On Sun, 4 Jan 2015 14:54:56, Bernd Edlinger wrote:
>>
>> Hi Jakub,
>>
>>
>> I think I have found a reasonable test case, see the attached patch file.
>> The use case is: a class that destroys an owned thread in the destructor.
>> The destructor sets the vptr again to thread::vptr but this should
>> _not_ trigger a diagnostic message, when the vptr does not really change.
>>
>> Jakub, this is another test case where the TREE_READONLY prevents
>> the tsan instrumentation. So I had first to install your patch:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01432.html
>>
>> ... to see the test case fail without my patch.
>>
>
> that has been installed in the meantime.
>
>> The patch installs cleanly on 4.9 and 4.8, however the 4.8 branch
>> has no tsan tests, so I would leave the test case away for 4.8.
>>
>
> I found, 4.8 does not have BT_FN_VOID_PTR_PTR, and no tsan tests
> at all, so it is probably not worth the effort.
>
>> Boot-strapped and regression-tested on x86_64-linux-gnu
>> OK for trunk and 4.9 + 4.8 branches?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>
> I found some test cases in the clang tree, about the __tsan_vptr_update.
> So I thought I should use these instead of inventing new ones.
>
> Attached you'll find an updated patch with one positive and one negative
> test for vptr races.
>
> Tested with x86_64-linux-gnu.
> OK for trunk and 4.9 after a while?
>
>
> Thanks
> Bernd.
>
 		 	   		  

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Jeff Law]

On 01/16/15 00:17, Bernd Edlinger wrote:
> Hi,
>
>
> I think I should ping for this patch now:
> https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00599.html
>
> note that by mistake the change log referenced sanitizer.c instead of
> sanitizer.def, consider that fixed on my local copy.
That patch is fine.  Thanks for pinging.

jeff

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Dmitry Vyukov]

This is just a copy from llvm repo, right?
Looks good to me.

On Fri, Jan 16, 2015 at 10:17 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
>
> I think I should ping for this patch now:
> https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00599.html
>
> note that by mistake the change log referenced sanitizer.c instead of
> sanitizer.def, consider that fixed on my local copy.
>
>
> Thanks
> Bernd.
>
>
>> Date: Sun, 11 Jan 2015 14:15:54 +0100
>>
>> Hi,
>>
>>
>>
>> On Sun, 4 Jan 2015 14:54:56, Bernd Edlinger wrote:
>>>
>>> Hi Jakub,
>>>
>>>
>>> I think I have found a reasonable test case, see the attached patch file.
>>> The use case is: a class that destroys an owned thread in the destructor.
>>> The destructor sets the vptr again to thread::vptr but this should
>>> _not_ trigger a diagnostic message, when the vptr does not really change.
>>>
>>> Jakub, this is another test case where the TREE_READONLY prevents
>>> the tsan instrumentation. So I had first to install your patch:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01432.html
>>>
>>> ... to see the test case fail without my patch.
>>>
>>
>> that has been installed in the meantime.
>>
>>> The patch installs cleanly on 4.9 and 4.8, however the 4.8 branch
>>> has no tsan tests, so I would leave the test case away for 4.8.
>>>
>>
>> I found, 4.8 does not have BT_FN_VOID_PTR_PTR, and no tsan tests
>> at all, so it is probably not worth the effort.
>>
>>> Boot-strapped and regression-tested on x86_64-linux-gnu
>>> OK for trunk and 4.9 + 4.8 branches?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>
>>
>> I found some test cases in the clang tree, about the __tsan_vptr_update.
>> So I thought I should use these instead of inventing new ones.
>>
>> Attached you'll find an updated patch with one positive and one negative
>> test for vptr races.
>>
>> Tested with x86_64-linux-gnu.
>> OK for trunk and 4.9 after a while?
>>
>>
>> Thanks
>> Bernd.
>>
>

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

Hi,

On Fri, 16 Jan 2015 21:25:42, Dmitry Vyukov wrote:
>
> This is just a copy from llvm repo, right?
> Looks good to me.
>

Thanks.

Yes I found these test case in the llvm tree, and just adapted them
to work in the gcc test suite.

However, here is a small tweak in the positive test:
That is we now use a tsan-invisible barrier_wait function
instead of the not very reliable sleep(1).

barrier_wait is bypassing the tsan interceptor, because
it is accessed with dlsym (dlopen ("libpthread.so.0", RTLD_LAZY),
"pthread_barrier_wait").


Bernd.


> On Fri, Jan 16, 2015 at 10:17 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi,
>>
>>
>> I think I should ping for this patch now:
>> https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00599.html
>>
>> note that by mistake the change log referenced sanitizer.c instead of
>> sanitizer.def, consider that fixed on my local copy.
>>
>>
>> Thanks
>> Bernd.
>>
>>
>>> Date: Sun, 11 Jan 2015 14:15:54 +0100
>>>
>>> Hi,
>>>
>>>
>>>
>>> On Sun, 4 Jan 2015 14:54:56, Bernd Edlinger wrote:
>>>>
>>>> Hi Jakub,
>>>>
>>>>
>>>> I think I have found a reasonable test case, see the attached patch file.
>>>> The use case is: a class that destroys an owned thread in the destructor.
>>>> The destructor sets the vptr again to thread::vptr but this should
>>>> _not_ trigger a diagnostic message, when the vptr does not really change.
>>>>
>>>> Jakub, this is another test case where the TREE_READONLY prevents
>>>> the tsan instrumentation. So I had first to install your patch:
>>>>
>>>> https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01432.html
>>>>
>>>> ... to see the test case fail without my patch.
>>>>
>>>
>>> that has been installed in the meantime.
>>>
>>>> The patch installs cleanly on 4.9 and 4.8, however the 4.8 branch
>>>> has no tsan tests, so I would leave the test case away for 4.8.
>>>>
>>>
>>> I found, 4.8 does not have BT_FN_VOID_PTR_PTR, and no tsan tests
>>> at all, so it is probably not worth the effort.
>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu
>>>> OK for trunk and 4.9 + 4.8 branches?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>>
>>>
>>> I found some test cases in the clang tree, about the __tsan_vptr_update.
>>> So I thought I should use these instead of inventing new ones.
>>>
>>> Attached you'll find an updated patch with one positive and one negative
>>> test for vptr races.
>>>
>>> Tested with x86_64-linux-gnu.
>>> OK for trunk and 4.9 after a while?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>
 		 	   		  

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Dmitry Vyukov]

On Fri, Jan 16, 2015 at 8:42 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> On Fri, 16 Jan 2015 21:25:42, Dmitry Vyukov wrote:
>>
>> This is just a copy from llvm repo, right?
>> Looks good to me.
>>
>
> Thanks.
>
> Yes I found these test case in the llvm tree, and just adapted them
> to work in the gcc test suite.
>
> However, here is a small tweak in the positive test:
> That is we now use a tsan-invisible barrier_wait function
> instead of the not very reliable sleep(1).
>
> barrier_wait is bypassing the tsan interceptor, because
> it is accessed with dlsym (dlopen ("libpthread.so.0", RTLD_LAZY),
> "pthread_barrier_wait").


+Kostya

Hi Bernd,

Yes, that email is marked in my inbox. Sorry for not answering earlier.

I can't really make my mind on this. I would mildly prefer sleep's (if
they work reliably!).
Kostya, please chime in. This is equivalent to StealthNotification
that we used in old data-race-test test suite:
https://code.google.com/p/data-race-test/source/browse/trunk/unittest/test_utils.h#134
Kostya, you had experience with both approaches. What are you thoughts on this?
StealthNotification definitely makes tests faster and more reliable. I
can't really come up with any objective downsides.




>> On Fri, Jan 16, 2015 at 10:17 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Hi,
>>>
>>>
>>> I think I should ping for this patch now:
>>> https://gcc.gnu.org/ml/gcc-patches/2015-01/msg00599.html
>>>
>>> note that by mistake the change log referenced sanitizer.c instead of
>>> sanitizer.def, consider that fixed on my local copy.
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>
>>>> Date: Sun, 11 Jan 2015 14:15:54 +0100
>>>>
>>>> Hi,
>>>>
>>>>
>>>>
>>>> On Sun, 4 Jan 2015 14:54:56, Bernd Edlinger wrote:
>>>>>
>>>>> Hi Jakub,
>>>>>
>>>>>
>>>>> I think I have found a reasonable test case, see the attached patch file.
>>>>> The use case is: a class that destroys an owned thread in the destructor.
>>>>> The destructor sets the vptr again to thread::vptr but this should
>>>>> _not_ trigger a diagnostic message, when the vptr does not really change.
>>>>>
>>>>> Jakub, this is another test case where the TREE_READONLY prevents
>>>>> the tsan instrumentation. So I had first to install your patch:
>>>>>
>>>>> https://gcc.gnu.org/ml/gcc-patches/2014-12/msg01432.html
>>>>>
>>>>> ... to see the test case fail without my patch.
>>>>>
>>>>
>>>> that has been installed in the meantime.
>>>>
>>>>> The patch installs cleanly on 4.9 and 4.8, however the 4.8 branch
>>>>> has no tsan tests, so I would leave the test case away for 4.8.
>>>>>
>>>>
>>>> I found, 4.8 does not have BT_FN_VOID_PTR_PTR, and no tsan tests
>>>> at all, so it is probably not worth the effort.
>>>>
>>>>> Boot-strapped and regression-tested on x86_64-linux-gnu
>>>>> OK for trunk and 4.9 + 4.8 branches?
>>>>>
>>>>>
>>>>> Thanks
>>>>> Bernd.
>>>>>
>>>>>
>>>>
>>>> I found some test cases in the clang tree, about the __tsan_vptr_update.
>>>> So I thought I should use these instead of inventing new ones.
>>>>
>>>> Attached you'll find an updated patch with one positive and one negative
>>>> test for vptr races.
>>>>
>>>> Tested with x86_64-linux-gnu.
>>>> OK for trunk and 4.9 after a while?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>
>

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Jakub Jelinek]

On Mon, Jan 19, 2015 at 12:43:39PM +0400, Dmitry Vyukov wrote:
> Hi Bernd,
> 
> Yes, that email is marked in my inbox. Sorry for not answering earlier.
> 
> I can't really make my mind on this. I would mildly prefer sleep's (if
> they work reliably!).

Sleeps by definition should not be reliable, not to mention that they waste
testing time.

> Kostya, please chime in. This is equivalent to StealthNotification
> that we used in old data-race-test test suite:
> https://code.google.com/p/data-race-test/source/browse/trunk/unittest/test_utils.h#134
> Kostya, you had experience with both approaches. What are you thoughts on this?
> StealthNotification definitely makes tests faster and more reliable. I
> can't really come up with any objective downsides.

	Jakub

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Mike Stump]

On Jan 19, 2015, at 12:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> I can't really make my mind on this. I would mildly prefer sleep's (if
> they work reliably!).

Let me state it more forcefully.  sleeps are not now, nor in the history of computing ever been a synchronization primitive, except for hard real time systems.  If they were, you would be able to cite a paper that uses them.  If I am wrong, I’d welcome a cite.  Any failure of sleep to work is a indication that that system is not a real time system, and the entirety of the gcc test suite is non-real time code (unless someone snuck some in while I wasn’t watching).  Only a synchronization primitive can make the test cases deterministic, therefore, sleep can never be used as a syntonization primitive in the gcc test suite.

> Kostya, you had experience with both approaches. What are you thoughts on this?
> StealthNotification definitely makes tests faster and more reliable.

To me, reliability isn’t a continuum for the gcc test suite.  It is binary.  It is, or, is not reliable and deterministic.  The standard for the gcc test suite is to be realible and deterministic.

> can't really come up with any objective downsides.

Nor I.

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Konstantin Serebryany]

[text-only]

On Mon, Jan 19, 2015 at 7:42 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Jan 19, 2015, at 12:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> I can't really make my mind on this. I would mildly prefer sleep's (if
>> they work reliably!).
>
> Let me state it more forcefully.
You don't have to convince us here.
I'd love to get rid of sleep calls in the tsan test suite -- they are
a minor but a constant annoyance.
But I also want to keep the tests *very simple*, i.e.
  1. Single file w/o any non-system includes, no linking of extra
libraries/objects
  2. Not too much extra code. (ideally, 1 line for init, 1 line for
"signal", 1 line for "wait")
  3. Strictly posix or c++11 (unless we are testing something specific)

Your idea with barrier_wait/dlsym sounds interesting, but I can't see
the code in this mail thread.
What do I miss?

> sleeps are not now, nor in the history of computing ever been a synchronization primitive, except for hard real time systems.  If they were, you would be able to cite a paper that uses them.  If I am wrong, I’d welcome a cite.  Any failure of sleep to work is a indication that that system is not a real time system, and the entirety of the gcc test suite is non-real time code (unless someone snuck some in while I wasn’t watching).  Only a synchronization primitive can make the test cases deterministic, therefore, sleep can never be used as a syntonization primitive in the gcc test suite.
>
>> Kostya, you had experience with both approaches. What are you thoughts on this?
>> StealthNotification definitely makes tests faster and more reliable.
Yep.
>
> To me, reliability isn’t a continuum for the gcc test suite.  It is binary.  It is, or, is not reliable and deterministic.  The standard for the gcc test suite is to be realible and deterministic.
>
>> can't really come up with any objective downsides.

he downside is more code in tests.
If we can satisfy my definition of *very simple* (above) -- let's do it.


>> not to mention that they waste testing time.

We discussed this. With a parallel test runner this is not a problem.
I don't say the sleeps are good, I just say that testing time is not
an argument against them.
Flakiness is.

--kcc

>
> Nor I.

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

Hi,

On Mon, 19 Jan 2015 18:49:21, Konstantin Serebryany wrote:
>
> [text-only]
>
> On Mon, Jan 19, 2015 at 7:42 AM, Mike Stump <mikestump@comcast.net> wrote:
>> On Jan 19, 2015, at 12:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> I can't really make my mind on this. I would mildly prefer sleep's (if
>>> they work reliably!).
>>
>> Let me state it more forcefully.
> You don't have to convince us here.
> I'd love to get rid of sleep calls in the tsan test suite -- they are
> a minor but a constant annoyance.
> But I also want to keep the tests *very simple*, i.e.
> 1. Single file w/o any non-system includes, no linking of extra
> libraries/objects
> 2. Not too much extra code. (ideally, 1 line for init, 1 line for
> "signal", 1 line for "wait")
> 3. Strictly posix or c++11 (unless we are testing something specific)
>
> Your idea with barrier_wait/dlsym sounds interesting, but I can't see
> the code in this mail thread.
> What do I miss?
>

We discussed two alternatives to sleep:

1. step function, optionally with sched_yield to make it somewhat less busy waiting:
__attribute__((no_sanitize_thread))
void step (int i)
{
   while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
     sched_yield();
   __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
}
2. tsan-invisible barriers:

cat tsan_barrier.h 
/* TSAN-invisible barriers.  Link with -ldl.  */
#include <pthread.h>
#include <dlfcn.h>

static __typeof(pthread_barrier_wait) *barrier_wait;

static
void barrier_init (pthread_barrier_t *barrier, unsigned count)
{
  void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
  barrier_wait = (__typeof (pthread_barrier_wait) *)
          dlsym (h, "pthread_barrier_wait");
  pthread_barrier_init (barrier, NULL, count);
}


We preferred the second alternative, because it does not do busy waiting.
We include this header file in every positive test case and link with -ldl.

Bernd.

 		 	   		  

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Konstantin Serebryany]

On Mon, Jan 19, 2015 at 11:09 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>
> Hi,
>
> On Mon, 19 Jan 2015 18:49:21, Konstantin Serebryany wrote:
>>
>> [text-only]
>>
>> On Mon, Jan 19, 2015 at 7:42 AM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Jan 19, 2015, at 12:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> I can't really make my mind on this. I would mildly prefer sleep's (if
>>>> they work reliably!).
>>>
>>> Let me state it more forcefully.
>> You don't have to convince us here.
>> I'd love to get rid of sleep calls in the tsan test suite -- they are
>> a minor but a constant annoyance.
>> But I also want to keep the tests *very simple*, i.e.
>> 1. Single file w/o any non-system includes, no linking of extra
>> libraries/objects
>> 2. Not too much extra code. (ideally, 1 line for init, 1 line for
>> "signal", 1 line for "wait")
>> 3. Strictly posix or c++11 (unless we are testing something specific)
>>
>> Your idea with barrier_wait/dlsym sounds interesting, but I can't see
>> the code in this mail thread.
>> What do I miss?
>>
>
> We discussed two alternatives to sleep:
>
> 1. step function, optionally with sched_yield to make it somewhat less busy waiting:
> __attribute__((no_sanitize_thread))
> void step (int i)
> {
>    while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
>      sched_yield();
>    __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
> }

This will not work: tsan will still  instrument atomics in a function
with __attribute__((no_sanitize_thread))
(This applies to both clang and gcc variants)

> 2. tsan-invisible barriers:
>
> cat tsan_barrier.h
> /* TSAN-invisible barriers.  Link with -ldl.  */
> #include <pthread.h>
> #include <dlfcn.h>
>
> static __typeof(pthread_barrier_wait) *barrier_wait;
>
> static
> void barrier_init (pthread_barrier_t *barrier, unsigned count)
> {
>   void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
>   barrier_wait = (__typeof (pthread_barrier_wait) *)
>           dlsym (h, "pthread_barrier_wait");
>   pthread_barrier_init (barrier, NULL, count);
> }

So, we will have a single extra header file, but no extra .c files.
This sounds tolerable.
I am not sure how portable that is, but today's tsan works only on
modern Linux anyway.

If you want to contribute the code, please send a patch to upstream LLVM:
we may not be able to take the code from gcc source tree to LLVM, the
other direction is easy.
Or please give us some time to fix the tests in upstream ourselves and
than port them to the gcc test suite.

Dmitry, wdyt?


>
>
> We preferred the second alternative, because it does not do busy waiting.
> We include this header file in every positive test case and link with -ldl.

-ldl is not required, since -fsanitize=thread already adds that.

--kcc

>
> Bernd.
>
>

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

Hi,

On Tue, 20 Jan 2015 17:47:29, Konstantin Serebryany wrote:
>>
>> We discussed two alternatives to sleep:
>>
>> 1. step function, optionally with sched_yield to make it somewhat less busy waiting:
>> __attribute__((no_sanitize_thread))
>> void step (int i)
>> {
>> while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
>> sched_yield();
>> __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
>> }
>
> This will not work: tsan will still instrument atomics in a function
> with __attribute__((no_sanitize_thread))
> (This applies to both clang and gcc variants)
>
>> 2. tsan-invisible barriers:
>>
>> cat tsan_barrier.h
>> /* TSAN-invisible barriers. Link with -ldl. */
>> #include <pthread.h>
>> #include <dlfcn.h>
>>
>> static __typeof(pthread_barrier_wait) *barrier_wait;
>>
>> static
>> void barrier_init (pthread_barrier_t *barrier, unsigned count)
>> {
>> void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
>> barrier_wait = (__typeof (pthread_barrier_wait) *)
>> dlsym (h, "pthread_barrier_wait");
>> pthread_barrier_init (barrier, NULL, count);
>> }
>
> So, we will have a single extra header file, but no extra .c files.
> This sounds tolerable.
> I am not sure how portable that is, but today's tsan works only on
> modern Linux anyway.
>
> If you want to contribute the code, please send a patch to upstream LLVM:
> we may not be able to take the code from gcc source tree to LLVM, the
> other direction is easy.
> Or please give us some time to fix the tests in upstream ourselves and
> than port them to the gcc test suite.
>

That's fine, LLVM is out of my reach so I can't do that myself, but I will
always help if I can.

> Dmitry, wdyt?
>
>
>>
>>
>> We preferred the second alternative, because it does not do busy waiting.
>> We include this header file in every positive test case and link with -ldl.
>
> -ldl is not required, since -fsanitize=thread already adds that.
>

Maybe that is a driver issue or just a linker oddity.
Clang will probably use a different linker.

In our case however, if I do not add -ldl to the application,
the linker complains about the missing DSO in the command line.

Also if I use -fsanititze=thread I do in most cases not need -pthread on the
command line, but if I happen to use a non-intercepted pthread function
for instance pthread_yield() I get an error:

cat test.c
#define _GNU_SOURCE
#include <pthread.h>
int main()
{
  pthread_yield();
  return 0;
}

gcc test.c -fsanitize=thread 
/home/ed/gnu/install/lib/gcc/x86_64-unknown-linux-gnu/5.0.0/../../../../x86_64-unknown-linux-gnu/bin/ld: /tmp/ccGGRrgy.o: undefined reference to symbol 'pthread_yield@@GLIBC_2.2.5'
/lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

gcc test.c -fsanitize=thread -pthread
=> OK


Bernd.
 		 	   		  

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Dmitry Vyukov]

On Tue, Jan 20, 2015 at 10:09 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>
> Hi,
>
> On Mon, 19 Jan 2015 18:49:21, Konstantin Serebryany wrote:
>>
>> [text-only]
>>
>> On Mon, Jan 19, 2015 at 7:42 AM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Jan 19, 2015, at 12:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>> I can't really make my mind on this. I would mildly prefer sleep's (if
>>>> they work reliably!).
>>>
>>> Let me state it more forcefully.
>> You don't have to convince us here.
>> I'd love to get rid of sleep calls in the tsan test suite -- they are
>> a minor but a constant annoyance.
>> But I also want to keep the tests *very simple*, i.e.
>> 1. Single file w/o any non-system includes, no linking of extra
>> libraries/objects
>> 2. Not too much extra code. (ideally, 1 line for init, 1 line for
>> "signal", 1 line for "wait")
>> 3. Strictly posix or c++11 (unless we are testing something specific)
>>
>> Your idea with barrier_wait/dlsym sounds interesting, but I can't see
>> the code in this mail thread.
>> What do I miss?
>>
>
> We discussed two alternatives to sleep:
>
> 1. step function, optionally with sched_yield to make it somewhat less busy waiting:
> __attribute__((no_sanitize_thread))
> void step (int i)
> {
>    while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
>      sched_yield();
>    __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
> }
> 2. tsan-invisible barriers:
>
> cat tsan_barrier.h
> /* TSAN-invisible barriers.  Link with -ldl.  */
> #include <pthread.h>
> #include <dlfcn.h>
>
> static __typeof(pthread_barrier_wait) *barrier_wait;
>
> static
> void barrier_init (pthread_barrier_t *barrier, unsigned count)
> {
>   void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
>   barrier_wait = (__typeof (pthread_barrier_wait) *)
>           dlsym (h, "pthread_barrier_wait");
>   pthread_barrier_init (barrier, NULL, count);
> }
>
>
> We preferred the second alternative, because it does not do busy waiting.
> We include this header file in every positive test case and link with -ldl.


The step approach looks better to me at first sight.

Busy waiting looks like a weak argument in this context. It's
absolutely non performance-critical and a yield or usleep(10) will
solve it more than sufficiently.

I will check how complex to make its implementation invisible to tsan
(I suspect that clang does not ignore atomic operations when
no_sanitize_thread attribute is given) and whether it actually makes
more complex tests simpler to write (as compared to the barrier
approach).

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

Hi, 

On Wed, 21 Jan 2015 12:58:27, Dmitry Vyukov wrote:
>
>
> The step approach looks better to me at first sight.
>
> Busy waiting looks like a weak argument in this context. It's
> absolutely non performance-critical and a yield or usleep(10) will
> solve it more than sufficiently.
>
> I will check how complex to make its implementation invisible to tsan
> (I suspect that clang does not ignore atomic operations when
> no_sanitize_thread attribute is given) and whether it actually makes
> more complex tests simpler to write (as compared to the barrier
> approach).

Yes, for Gcc this is a pretty new feature, so I dont think we should use it now.

It _should_ suppress the instrumentation of atomics, but it can not be used to
avoid intercepted calls like usleep, and using sched_yield is safe, but usleep
is intercepted, and may change the printed diagnostics.

But unfortunately it is not really stable at the time:

cat test.c
volatile int serial=0;
__attribute__((no_sanitize_thread))
void step (int i)
{
   while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
     sched_yield();
   __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
}

gcc -S -fsanitize=thread test.c
test.c: In function 'step':
test.c:6:6: warning: implicit declaration of function 'sched_yield' [-Wimplicit-function-declaration]
      sched_yield();
      ^
test.c:3:6: internal compiler error: in expand_TSAN_FUNC_EXIT, at internal-fn.c:243
 void step (int i)
      ^
0x966b17 expand_TSAN_FUNC_EXIT
    ../../gcc-5-20150118/gcc/internal-fn.c:243
0x75438f expand_call_stmt
    ../../gcc-5-20150118/gcc/cfgexpand.c:2311
0x75438f expand_gimple_stmt_1
    ../../gcc-5-20150118/gcc/cfgexpand.c:3327
0x75438f expand_gimple_stmt
    ../../gcc-5-20150118/gcc/cfgexpand.c:3481
0x75a862 expand_gimple_basic_block
    ../../gcc-5-20150118/gcc/cfgexpand.c:5394
0x75c087 execute
    ../../gcc-5-20150118/gcc/cfgexpand.c:6003
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <http://gcc.gnu.org/bugs.html> for instructions.



Bernd.
 		 	   		  

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

Hi Jakub & Mike,

> test.c:3:6: internal compiler error: in expand_TSAN_FUNC_EXIT, at internal-fn.c:243
>  void step (int i)


It looks like your patches shot each other down..

How about this, maybe with a compile-time test of the step function in c-c++common/tsan?


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: patch-tsan-attribute.diff --]
[-- Type: application/octet-stream, Size: 518 bytes --]

--- gcc/gimplify.c.jj	2015-01-15 21:11:12.000000000 +0100
+++ gcc/gimplify.c	2015-01-21 11:49:38.214588504 +0100
@@ -9250,7 +9250,8 @@ gimplify_function_tree (tree fndecl)
       bind = new_bind;
     }
 
-  if (flag_sanitize & SANITIZE_THREAD)
+  if ((flag_sanitize & SANITIZE_THREAD) != 0
+      && !lookup_attribute ("no_sanitize_thread", DECL_ATTRIBUTES (fndecl)))
     {
       gcall *call = gimple_build_call_internal (IFN_TSAN_FUNC_EXIT, 0);
       gimple tf = gimple_build_try (seq, call, GIMPLE_TRY_FINALLY);

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Jakub Jelinek]

On Wed, Jan 21, 2015 at 12:17:51PM +0100, Bernd Edlinger wrote:
> > test.c:3:6: internal compiler error: in expand_TSAN_FUNC_EXIT, at internal-fn.c:243
> >  void step (int i)
> 
> 
> It looks like your patches shot each other down..
> 
> How about this, maybe with a compile-time test of the step function in c-c++common/tsan?

If you add a testcase for it, sure.  It can be just compile time testcase,
perhaps with some assembler match that no __tsan_ calls are made.

	Jakub

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

On Wed, 21 Jan 2015 12:31:57, Jakub Jelinek wrote:
>
> On Wed, Jan 21, 2015 at 12:17:51PM +0100, Bernd Edlinger wrote:
>>> test.c:3:6: internal compiler error: in expand_TSAN_FUNC_EXIT, at internal-fn.c:243
>>> void step (int i)
>>
>>
>> It looks like your patches shot each other down..
>>
>> How about this, maybe with a compile-time test of the step function in c-c++common/tsan?
>
> If you add a testcase for it, sure. It can be just compile time testcase,
> perhaps with some assembler match that no __tsan_ calls are made.
>

yes, that's what I had in mind.


Bernd.
 		 	   		  

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Mike Stump]

On Jan 21, 2015, at 3:17 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> Hi Jakub & Mike,
> 
>> test.c:3:6: internal compiler error: in expand_TSAN_FUNC_EXIT, at internal-fn.c:243
>> void step (int i)
> 
> It looks like your patches shot each other down..

Ah, I’d use the phrase one-step forward.  Yeah, looks not as useful without the second step forward as well.

> How about this, maybe with a compile-time test of the step function in c-c++common/tsan?

I’m fine with it (assuming it passes a regtest), seems like it should be reviewed by the normal tsan people.  They might be able to spot other places that might need updating.

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Dmitry Vyukov]

Well, OK, it is actually not easier to write tests with step function
as compared to barrier.


On Wed, Jan 21, 2015 at 11:58 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Jan 20, 2015 at 10:09 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>>
>> Hi,
>>
>> On Mon, 19 Jan 2015 18:49:21, Konstantin Serebryany wrote:
>>>
>>> [text-only]
>>>
>>> On Mon, Jan 19, 2015 at 7:42 AM, Mike Stump <mikestump@comcast.net> wrote:
>>>> On Jan 19, 2015, at 12:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>> I can't really make my mind on this. I would mildly prefer sleep's (if
>>>>> they work reliably!).
>>>>
>>>> Let me state it more forcefully.
>>> You don't have to convince us here.
>>> I'd love to get rid of sleep calls in the tsan test suite -- they are
>>> a minor but a constant annoyance.
>>> But I also want to keep the tests *very simple*, i.e.
>>> 1. Single file w/o any non-system includes, no linking of extra
>>> libraries/objects
>>> 2. Not too much extra code. (ideally, 1 line for init, 1 line for
>>> "signal", 1 line for "wait")
>>> 3. Strictly posix or c++11 (unless we are testing something specific)
>>>
>>> Your idea with barrier_wait/dlsym sounds interesting, but I can't see
>>> the code in this mail thread.
>>> What do I miss?
>>>
>>
>> We discussed two alternatives to sleep:
>>
>> 1. step function, optionally with sched_yield to make it somewhat less busy waiting:
>> __attribute__((no_sanitize_thread))
>> void step (int i)
>> {
>>    while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
>>      sched_yield();
>>    __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
>> }
>> 2. tsan-invisible barriers:
>>
>> cat tsan_barrier.h
>> /* TSAN-invisible barriers.  Link with -ldl.  */
>> #include <pthread.h>
>> #include <dlfcn.h>
>>
>> static __typeof(pthread_barrier_wait) *barrier_wait;
>>
>> static
>> void barrier_init (pthread_barrier_t *barrier, unsigned count)
>> {
>>   void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
>>   barrier_wait = (__typeof (pthread_barrier_wait) *)
>>           dlsym (h, "pthread_barrier_wait");
>>   pthread_barrier_init (barrier, NULL, count);
>> }
>>
>>
>> We preferred the second alternative, because it does not do busy waiting.
>> We include this header file in every positive test case and link with -ldl.
>
>
> The step approach looks better to me at first sight.
>
> Busy waiting looks like a weak argument in this context. It's
> absolutely non performance-critical and a yield or usleep(10) will
> solve it more than sufficiently.
>
> I will check how complex to make its implementation invisible to tsan
> (I suspect that clang does not ignore atomic operations when
> no_sanitize_thread attribute is given) and whether it actually makes
> more complex tests simpler to write (as compared to the barrier
> approach).

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Dmitry Vyukov]

Refactored tests in clang to use barrier_init/wait:
http://llvm.org/viewvc/llvm-project?view=revision&revision=226659

There are still few sleep call, like when we need to wait for a thread
to exit (there is really no point to insert barrier_wait); or when we
need to wait for a thread to _block_ inside of pthread_cond_wait; or
when a thread needs to process signals. But all remaining uses of
sleep are commented.



On Wed, Jan 21, 2015 at 11:58 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Jan 20, 2015 at 10:09 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>>
>> Hi,
>>
>> On Mon, 19 Jan 2015 18:49:21, Konstantin Serebryany wrote:
>>>
>>> [text-only]
>>>
>>> On Mon, Jan 19, 2015 at 7:42 AM, Mike Stump <mikestump@comcast.net> wrote:
>>>> On Jan 19, 2015, at 12:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>>>> I can't really make my mind on this. I would mildly prefer sleep's (if
>>>>> they work reliably!).
>>>>
>>>> Let me state it more forcefully.
>>> You don't have to convince us here.
>>> I'd love to get rid of sleep calls in the tsan test suite -- they are
>>> a minor but a constant annoyance.
>>> But I also want to keep the tests *very simple*, i.e.
>>> 1. Single file w/o any non-system includes, no linking of extra
>>> libraries/objects
>>> 2. Not too much extra code. (ideally, 1 line for init, 1 line for
>>> "signal", 1 line for "wait")
>>> 3. Strictly posix or c++11 (unless we are testing something specific)
>>>
>>> Your idea with barrier_wait/dlsym sounds interesting, but I can't see
>>> the code in this mail thread.
>>> What do I miss?
>>>
>>
>> We discussed two alternatives to sleep:
>>
>> 1. step function, optionally with sched_yield to make it somewhat less busy waiting:
>> __attribute__((no_sanitize_thread))
>> void step (int i)
>> {
>>    while (__atomic_load_n (&serial, __ATOMIC_ACQUIRE) != i - 1)
>>      sched_yield();
>>    __atomic_store_n (&serial, i, __ATOMIC_RELEASE);
>> }
>> 2. tsan-invisible barriers:
>>
>> cat tsan_barrier.h
>> /* TSAN-invisible barriers.  Link with -ldl.  */
>> #include <pthread.h>
>> #include <dlfcn.h>
>>
>> static __typeof(pthread_barrier_wait) *barrier_wait;
>>
>> static
>> void barrier_init (pthread_barrier_t *barrier, unsigned count)
>> {
>>   void *h = dlopen ("libpthread.so.0", RTLD_LAZY);
>>   barrier_wait = (__typeof (pthread_barrier_wait) *)
>>           dlsym (h, "pthread_barrier_wait");
>>   pthread_barrier_init (barrier, NULL, count);
>> }
>>
>>
>> We preferred the second alternative, because it does not do busy waiting.
>> We include this header file in every positive test case and link with -ldl.
>
>
> The step approach looks better to me at first sight.
>
> Busy waiting looks like a weak argument in this context. It's
> absolutely non performance-critical and a yield or usleep(10) will
> solve it more than sufficiently.
>
> I will check how complex to make its implementation invisible to tsan
> (I suspect that clang does not ignore atomic operations when
> no_sanitize_thread attribute is given) and whether it actually makes
> more complex tests simpler to write (as compared to the barrier
> approach).

RE: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Bernd Edlinger]

Hi,

On 21 Jan 2015 17:57:10, Dmitry Vyukov wrote:
> Subject: Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update
> To: bernd.edlinger@hotmail.de
> CC: konstantin.s.serebryany@gmail.com; mikestump@comcast.net; kcc@google.com; jakub@redhat.com; richard.guenther@gmail.com; gcc-patches@gcc.gnu.org
>
> Refactored tests in clang to use barrier_init/wait:
> http://llvm.org/viewvc/llvm-project?view=revision&revision=226659
>
> There are still few sleep call, like when we need to wait for a thread
> to exit (there is really no point to insert barrier_wait); or when we
> need to wait for a thread to _block_ inside of pthread_cond_wait; or
> when a thread needs to process signals. But all remaining uses of
> sleep are commented.
>

Wow. That was quick!
You should no longer need the %deflake thing now, right?


Thanks,
Bernd.
 		 	   		  

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Dmitry Vyukov]

On Wed, Jan 21, 2015 at 5:38 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> On 21 Jan 2015 17:57:10, Dmitry Vyukov wrote:
>> Subject: Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update
>> To: bernd.edlinger@hotmail.de
>> CC: konstantin.s.serebryany@gmail.com; mikestump@comcast.net; kcc@google.com; jakub@redhat.com; richard.guenther@gmail.com; gcc-patches@gcc.gnu.org
>>
>> Refactored tests in clang to use barrier_init/wait:
>> http://llvm.org/viewvc/llvm-project?view=revision&revision=226659
>>
>> There are still few sleep call, like when we need to wait for a thread
>> to exit (there is really no point to insert barrier_wait); or when we
>> need to wait for a thread to _block_ inside of pthread_cond_wait; or
>> when a thread needs to process signals. But all remaining uses of
>> sleep are commented.
>>
>
> Wow. That was quick!
> You should no longer need the %deflake thing now, right?


You are right.
But I am somewhat tired of editing hundreds of files for today. Why
did I write so many tests, stupid!?

Re: [PING] [PATCH] Fix parameters of __tsan_vptr_update

[Mike Stump]

On Jan 21, 2015, at 6:41 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> But I am somewhat tired of editing hundreds of files for today. Why
> did I write so many tests, stupid!?

:-)  We appreciate all your efforts and all your tests.