On Wed, Sep 07, 2016 at 10:40:20PM +0200, Jakub Jelinek wrote:
> On Wed, Sep 07, 2016 at 09:07:45AM +0200, Richard Biener wrote:
> > > @@ -493,6 +504,8 @@ instrument_builtin_call (gimple_stmt_ite
> > > if (!tree_fits_uhwi_p (last_arg)
> > > || memmodel_base (tree_to_uhwi (last_arg)) >= MEMMODEL_LAST)
> > > return;
> > > + if (lookup_stmt_eh_lp (stmt))
> > > + remove_stmt_from_eh_lp (stmt);
> >
> > These changes look bogus to me -- how can the tsan instrumentation
> > function _not_ throw when the original function we replace it can?
>
> The __tsan*atomic* functions are right now declared to be nothrow, so the
> patch just matches how they are declared.
> While the sync-builtins.def use
> #define ATTR_NOTHROWCALL_LEAF_LIST (flag_non_call_exceptions ? \
> ATTR_LEAF_LIST : ATTR_NOTHROW_LEAF_LIST)
> I guess I could use the same for the tsan atomics, but wonder if it will work
> properly when the libtsan is built with exceptions disabled and without
> -fnon-call-exceptions. Perhaps it would, at least if it is built with
> -fasynchronous-unwind-tables (which is the case for x86_64 and aarch64 and
> tsan isn't supported elsewhere).
Actually, looking at the libsanitizer/tsan/ implementation, I have doubts
the atomics handling code would behave reasonably if an exception is thrown
during the atomic memory access, I'm afraid some classes wouldn't be
destructed.
I can split the patch into two, one dealing just with the
__atomic_clear/__atomic_test_and_set instrumentation and another for the
preexisting -fnon-call-exceptions ICEs. For the latter, one possibility
would be to error out on the -fsanitize=thread -fnon-call-exceptions
combination.
Jakub
