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

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

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

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

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.