Fix -fsanitize=thread with -fnon-call-exceptions (PR sanitizer/80110)

Submitted by Jakub Jelinek on March 20, 2017, 9:26 p.m.

Details

Message ID 20170320212603.GQ11094@tucnak
State New
Headers show

Commit Message

Jakub Jelinek March 20, 2017, 9:26 p.m.
Hi!

libtsan atomics aren't throwing, so if we transform atomics which
are throwing with -fnon-call-exceptions, we need to clean up EH stuff.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-03-20  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/80110
	* tsan.c: Include tree-eh.h.
	(instrument_builtin_call): Call maybe_clean_eh_stmt or
	maybe_clean_or_replace_eh_stmt where needed.
	(instrument_memory_accesses): Add cfg_changed argument.
	Call gimple_purge_dead_eh_edges on each block and set *cfg_changed
	if it returned true.
	(tsan_pass): Adjust caller.  Return TODO_cleanup_cfg if cfg_changed.

	* g++.dg/tsan/pr80110.C: New test.


	Jakub

Comments

Richard Guenther March 21, 2017, 8:12 a.m.
On Mon, 20 Mar 2017, Jakub Jelinek wrote:

> Hi!
> 
> libtsan atomics aren't throwing, so if we transform atomics which
> are throwing with -fnon-call-exceptions, we need to clean up EH stuff.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Huh, but this means with TSAN we create wrong-code with 
-fnon-call-exceptions and programs will crash instead of properly
catching things like null-pointer accesses?

We should at least document this or reject sanitize=thread with
-fnon-call-exceptions.

Richard.

> 2017-03-20  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/80110
> 	* tsan.c: Include tree-eh.h.
> 	(instrument_builtin_call): Call maybe_clean_eh_stmt or
> 	maybe_clean_or_replace_eh_stmt where needed.
> 	(instrument_memory_accesses): Add cfg_changed argument.
> 	Call gimple_purge_dead_eh_edges on each block and set *cfg_changed
> 	if it returned true.
> 	(tsan_pass): Adjust caller.  Return TODO_cleanup_cfg if cfg_changed.
> 
> 	* g++.dg/tsan/pr80110.C: New test.
> 
> --- gcc/tsan.c.jj	2017-03-20 17:55:25.000000000 +0100
> +++ gcc/tsan.c	2017-03-20 19:43:34.715321105 +0100
> @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3.
>  #include "tree-iterator.h"
>  #include "tree-ssa-propagate.h"
>  #include "tree-ssa-loop-ivopts.h"
> +#include "tree-eh.h"
>  #include "tsan.h"
>  #include "asan.h"
>  #include "builtins.h"
> @@ -504,6 +505,7 @@ instrument_builtin_call (gimple_stmt_ite
>  	      return;
>  	    gimple_call_set_fndecl (stmt, decl);
>  	    update_stmt (stmt);
> +	    maybe_clean_eh_stmt (stmt);
>  	    if (tsan_atomic_table[i].action == fetch_op)
>  	      {
>  		args[1] = gimple_call_arg (stmt, 1);
> @@ -524,6 +526,7 @@ instrument_builtin_call (gimple_stmt_ite
>  				       ? MEMMODEL_SEQ_CST
>  				       : MEMMODEL_ACQUIRE);
>  	    update_gimple_call (gsi, decl, num + 1, args[0], args[1], args[2]);
> +	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
>  	    stmt = gsi_stmt (*gsi);
>  	    if (tsan_atomic_table[i].action == fetch_op_seq_cst)
>  	      {
> @@ -572,6 +575,7 @@ instrument_builtin_call (gimple_stmt_ite
>  	      return;
>  	    update_gimple_call (gsi, decl, 5, args[0], args[1], args[2],
>  				args[4], args[5]);
> +	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
>  	    return;
>  	  case bool_cas:
>  	  case val_cas:
> @@ -599,6 +603,7 @@ instrument_builtin_call (gimple_stmt_ite
>  					       MEMMODEL_SEQ_CST),
>  				build_int_cst (NULL_TREE,
>  					       MEMMODEL_SEQ_CST));
> +	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
>  	    if (tsan_atomic_table[i].action == val_cas && lhs)
>  	      {
>  		tree cond;
> @@ -623,6 +628,7 @@ instrument_builtin_call (gimple_stmt_ite
>  				build_int_cst (t, 0),
>  				build_int_cst (NULL_TREE,
>  					       MEMMODEL_RELEASE));
> +	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
>  	    return;
>  	  case bool_clear:
>  	  case bool_test_and_set:
> @@ -651,11 +657,13 @@ instrument_builtin_call (gimple_stmt_ite
>  	      {
>  		update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
>  				    build_int_cst (t, 0), last_arg);
> +		maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
>  		return;
>  	      }
>  	    t = build_int_cst (t, targetm.atomic_test_and_set_trueval);
>  	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
>  				t, last_arg);
> +	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
>  	    stmt = gsi_stmt (*gsi);
>  	    lhs = gimple_call_lhs (stmt);
>  	    if (lhs == NULL_TREE)
> @@ -766,7 +774,7 @@ instrument_func_exit (void)
>     Return true if func entry/exit should be instrumented.  */
>  
>  static bool
> -instrument_memory_accesses (void)
> +instrument_memory_accesses (bool *cfg_changed)
>  {
>    basic_block bb;
>    gimple_stmt_iterator gsi;
> @@ -775,20 +783,24 @@ instrument_memory_accesses (void)
>    auto_vec<gimple *> tsan_func_exits;
>  
>    FOR_EACH_BB_FN (bb, cfun)
> -    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -      {
> -	gimple *stmt = gsi_stmt (gsi);
> -	if (gimple_call_internal_p (stmt, IFN_TSAN_FUNC_EXIT))
> -	  {
> -	    if (fentry_exit_instrument)
> -	      replace_func_exit (stmt);
> -	    else
> -	      tsan_func_exits.safe_push (stmt);
> -	    func_exit_seen = true;
> -	  }
> -	else
> -	  fentry_exit_instrument |= instrument_gimple (&gsi);
> -      }
> +    {
> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> +	{
> +	  gimple *stmt = gsi_stmt (gsi);
> +	  if (gimple_call_internal_p (stmt, IFN_TSAN_FUNC_EXIT))
> +	    {
> +	      if (fentry_exit_instrument)
> +		replace_func_exit (stmt);
> +	      else
> +		tsan_func_exits.safe_push (stmt);
> +	      func_exit_seen = true;
> +	    }
> +	  else
> +	    fentry_exit_instrument |= instrument_gimple (&gsi);
> +	}
> +      if (gimple_purge_dead_eh_edges (bb))
> +	*cfg_changed = true;
> +    }
>    unsigned int i;
>    gimple *stmt;
>    FOR_EACH_VEC_ELT (tsan_func_exits, i, stmt)
> @@ -835,9 +847,10 @@ static unsigned
>  tsan_pass (void)
>  {
>    initialize_sanitizer_builtins ();
> -  if (instrument_memory_accesses ())
> +  bool cfg_changed = false;
> +  if (instrument_memory_accesses (&cfg_changed))
>      instrument_func_entry ();
> -  return 0;
> +  return cfg_changed ? TODO_cleanup_cfg : 0;
>  }
>  
>  /* Inserts __tsan_init () into the list of CTORs.  */
> --- gcc/testsuite/g++.dg/tsan/pr80110.C.jj	2017-03-20 19:51:02.525497662 +0100
> +++ gcc/testsuite/g++.dg/tsan/pr80110.C	2017-03-20 19:50:14.000000000 +0100
> @@ -0,0 +1,16 @@
> +// PR sanitizer/80110
> +// { dg-do compile }
> +// { dg-options "-fnon-call-exceptions -fsanitize=thread" }
> +
> +struct A
> +{
> +  int b ();
> +  void c () { static int d = b (); }
> +};
> +
> +void
> +foo ()
> +{
> +  A e;
> +  e.c ();
> +}
> 
> 	Jakub
> 
>
Jakub Jelinek March 21, 2017, 8:21 a.m.
On Tue, Mar 21, 2017 at 09:12:51AM +0100, Richard Biener wrote:
> > libtsan atomics aren't throwing, so if we transform atomics which
> > are throwing with -fnon-call-exceptions, we need to clean up EH stuff.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Huh, but this means with TSAN we create wrong-code with 
> -fnon-call-exceptions and programs will crash instead of properly
> catching things like null-pointer accesses?

True.  I think it is only about atomics, normal loads/stores are done
inline with extra calls before/after that just tell the library about
those loads/stores (though not sure what the library code will do with
invalid or NULL pointers, if it won't crash on them).
The question is what we could do about it and if it is worth bothering,
I guess we'd need to build tsan_interface_atomics.cc with
-fnon-call-exceptions and the question would be if it doesn't slow it
down for the common case where -fnon-call-exceptions isn't used.
Another option would be to add another set of __tsan_atomic* entrypoints
for -fnon-call-exceptions.

> We should at least document this or reject sanitize=thread with
> -fnon-call-exceptions.

Rejecting would mean that even code that doesn't use the atomics or doesn't
use atomics on invalid pointers would not be allowed.

	Jakub
Richard Guenther March 21, 2017, 8:26 a.m.
On Tue, 21 Mar 2017, Jakub Jelinek wrote:

> On Tue, Mar 21, 2017 at 09:12:51AM +0100, Richard Biener wrote:
> > > libtsan atomics aren't throwing, so if we transform atomics which
> > > are throwing with -fnon-call-exceptions, we need to clean up EH stuff.
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > Huh, but this means with TSAN we create wrong-code with 
> > -fnon-call-exceptions and programs will crash instead of properly
> > catching things like null-pointer accesses?
> 
> True.  I think it is only about atomics, normal loads/stores are done
> inline with extra calls before/after that just tell the library about
> those loads/stores (though not sure what the library code will do with
> invalid or NULL pointers, if it won't crash on them).
> The question is what we could do about it and if it is worth bothering,
> I guess we'd need to build tsan_interface_atomics.cc with
> -fnon-call-exceptions and the question would be if it doesn't slow it
> down for the common case where -fnon-call-exceptions isn't used.
> Another option would be to add another set of __tsan_atomic* entrypoints
> for -fnon-call-exceptions.
> 
> > We should at least document this or reject sanitize=thread with
> > -fnon-call-exceptions.
> 
> Rejecting would mean that even code that doesn't use the atomics or doesn't
> use atomics on invalid pointers would not be allowed.

Indeed.  So maybe just document it then.  I suppose sanitizing in
general has the issue that its events are not properly raising
exceptions (of whatever kind), so documenting that programs relying
on async EH to be reliably delivered may not work as expected with
sanitization would be good.

Richard.

Patch hide | download patch | download mbox

--- gcc/tsan.c.jj	2017-03-20 17:55:25.000000000 +0100
+++ gcc/tsan.c	2017-03-20 19:43:34.715321105 +0100
@@ -38,6 +38,7 @@  along with GCC; see the file COPYING3.
 #include "tree-iterator.h"
 #include "tree-ssa-propagate.h"
 #include "tree-ssa-loop-ivopts.h"
+#include "tree-eh.h"
 #include "tsan.h"
 #include "asan.h"
 #include "builtins.h"
@@ -504,6 +505,7 @@  instrument_builtin_call (gimple_stmt_ite
 	      return;
 	    gimple_call_set_fndecl (stmt, decl);
 	    update_stmt (stmt);
+	    maybe_clean_eh_stmt (stmt);
 	    if (tsan_atomic_table[i].action == fetch_op)
 	      {
 		args[1] = gimple_call_arg (stmt, 1);
@@ -524,6 +526,7 @@  instrument_builtin_call (gimple_stmt_ite
 				       ? MEMMODEL_SEQ_CST
 				       : MEMMODEL_ACQUIRE);
 	    update_gimple_call (gsi, decl, num + 1, args[0], args[1], args[2]);
+	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
 	    stmt = gsi_stmt (*gsi);
 	    if (tsan_atomic_table[i].action == fetch_op_seq_cst)
 	      {
@@ -572,6 +575,7 @@  instrument_builtin_call (gimple_stmt_ite
 	      return;
 	    update_gimple_call (gsi, decl, 5, args[0], args[1], args[2],
 				args[4], args[5]);
+	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
 	    return;
 	  case bool_cas:
 	  case val_cas:
@@ -599,6 +603,7 @@  instrument_builtin_call (gimple_stmt_ite
 					       MEMMODEL_SEQ_CST),
 				build_int_cst (NULL_TREE,
 					       MEMMODEL_SEQ_CST));
+	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
 	    if (tsan_atomic_table[i].action == val_cas && lhs)
 	      {
 		tree cond;
@@ -623,6 +628,7 @@  instrument_builtin_call (gimple_stmt_ite
 				build_int_cst (t, 0),
 				build_int_cst (NULL_TREE,
 					       MEMMODEL_RELEASE));
+	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
 	    return;
 	  case bool_clear:
 	  case bool_test_and_set:
@@ -651,11 +657,13 @@  instrument_builtin_call (gimple_stmt_ite
 	      {
 		update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
 				    build_int_cst (t, 0), last_arg);
+		maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
 		return;
 	      }
 	    t = build_int_cst (t, targetm.atomic_test_and_set_trueval);
 	    update_gimple_call (gsi, decl, 3, gimple_call_arg (stmt, 0),
 				t, last_arg);
+	    maybe_clean_or_replace_eh_stmt (stmt, gsi_stmt (*gsi));
 	    stmt = gsi_stmt (*gsi);
 	    lhs = gimple_call_lhs (stmt);
 	    if (lhs == NULL_TREE)
@@ -766,7 +774,7 @@  instrument_func_exit (void)
    Return true if func entry/exit should be instrumented.  */
 
 static bool
-instrument_memory_accesses (void)
+instrument_memory_accesses (bool *cfg_changed)
 {
   basic_block bb;
   gimple_stmt_iterator gsi;
@@ -775,20 +783,24 @@  instrument_memory_accesses (void)
   auto_vec<gimple *> tsan_func_exits;
 
   FOR_EACH_BB_FN (bb, cfun)
-    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-      {
-	gimple *stmt = gsi_stmt (gsi);
-	if (gimple_call_internal_p (stmt, IFN_TSAN_FUNC_EXIT))
-	  {
-	    if (fentry_exit_instrument)
-	      replace_func_exit (stmt);
-	    else
-	      tsan_func_exits.safe_push (stmt);
-	    func_exit_seen = true;
-	  }
-	else
-	  fentry_exit_instrument |= instrument_gimple (&gsi);
-      }
+    {
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
+	{
+	  gimple *stmt = gsi_stmt (gsi);
+	  if (gimple_call_internal_p (stmt, IFN_TSAN_FUNC_EXIT))
+	    {
+	      if (fentry_exit_instrument)
+		replace_func_exit (stmt);
+	      else
+		tsan_func_exits.safe_push (stmt);
+	      func_exit_seen = true;
+	    }
+	  else
+	    fentry_exit_instrument |= instrument_gimple (&gsi);
+	}
+      if (gimple_purge_dead_eh_edges (bb))
+	*cfg_changed = true;
+    }
   unsigned int i;
   gimple *stmt;
   FOR_EACH_VEC_ELT (tsan_func_exits, i, stmt)
@@ -835,9 +847,10 @@  static unsigned
 tsan_pass (void)
 {
   initialize_sanitizer_builtins ();
-  if (instrument_memory_accesses ())
+  bool cfg_changed = false;
+  if (instrument_memory_accesses (&cfg_changed))
     instrument_func_entry ();
-  return 0;
+  return cfg_changed ? TODO_cleanup_cfg : 0;
 }
 
 /* Inserts __tsan_init () into the list of CTORs.  */
--- gcc/testsuite/g++.dg/tsan/pr80110.C.jj	2017-03-20 19:51:02.525497662 +0100
+++ gcc/testsuite/g++.dg/tsan/pr80110.C	2017-03-20 19:50:14.000000000 +0100
@@ -0,0 +1,16 @@ 
+// PR sanitizer/80110
+// { dg-do compile }
+// { dg-options "-fnon-call-exceptions -fsanitize=thread" }
+
+struct A
+{
+  int b ();
+  void c () { static int d = b (); }
+};
+
+void
+foo ()
+{
+  A e;
+  e.c ();
+}