[Bug sanitizer/65400] tsan mis-compiles inlineable C functions

["bernd.edlinger at hotmail dot de" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65400

--- Comment #7 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
(In reply to Jakub Jelinek from comment #6)
> Both patches look wrong to me.
> For the first change, it is wrong to add TSAN_FUNC_EXIT (), you should never
> add it out of nothing.  First of all, you might consider allowing
> TSAN_FUNC_EXIT () in find_return_bb - there is no reason why any harm would
> be done if it is considered a part of a return bb.  On your first testcase
> that is not the case though, so instead you need to either duplicate or move
> it.  I'd say best would be to bail out early with fnsplitting if
> TSAN_FUNC_EXIT is present in a bb that is not return_bb itself or one of its
> predecessors; or when it is present in one of the predecessors of return_bb
> and not in all the other predecessors.  The case when TSAN_FUNC_EXIT is in
> the return_bb (after you change find_return_bb) should work fine without any
> extra work, and for the case when it is in the predecessors of return_bb,
> add it.
> 

Would you have time to continue on this?

> The second change doesn't make any sense at all, but from the testcase it
> isn't obvious what you are trying to do at all.  If the problem is that
> fnsplit has set tail call flag and you've added the TSAN_FUNC_EXIT after it,
> then that should be where you clear the flag; if it is something different,
> please explain what you are trying to do and why.

fnsplit does _not_ set the tail call flag, but tail call optimization is
nevertheless happening.  One other thing, that is probably too risky:
If for whatever reason the tail call optimization is not happening here,
the call stack would be incomplete, unfortunately the interesting part
would be missing.  So I agree, the first patch may work for my application,
but, ....


The second test case has nothing to do with fnsplit, all I can tell is,
that these functions are binary identical, but operate on different
types, and these look in gimple like

OpcUa_Int32_P_NativeToWire (OpcUa_Int32_Wire * wire, OpcUa_Int32 * native)
{
  OpcUa_StatusCode retval.4;

  <bb 2>:
  retval.4_5 = OpcUa_Float_P_NativeToWire (wire_2(D), native_3(D)); [tail call]
  return retval.4_5;

}

so no TSAN_FUNC_EXIT at all.

with the patch that is transformed to

OpcUa_Int32_P_NativeToWire (OpcUa_Int32_Wire * wire, OpcUa_Int32 * native)
{
  OpcUa_StatusCode retval.4;
  void * _6;

  <bb 2>:
  _6 = __builtin_return_address (0);
  __builtin___tsan_func_entry (_6);
  retval.4_5 = OpcUa_Float_P_NativeToWire (wire_2(D), native_3(D));
  __builtin___tsan_func_exit ();
  return retval.4_5;

}

but without the patch this is transformed into:

OpcUa_Int32_P_NativeToWire (OpcUa_Int32_Wire * wire, OpcUa_Int32 * native)
{
  OpcUa_StatusCode retval.4;
  void * _6;

  <bb 2>:
  _6 = __builtin_return_address (0);
  __builtin___tsan_func_entry (_6);
  retval.4_5 = OpcUa_Float_P_NativeToWire (wire_2(D), native_3(D)); [tail call]
  __builtin___tsan_func_exit ();
  return retval.4_5;

}



This is probably incorrect gimple, because the former "tail call" is
no longer a tail call, because we add the __builtin__tsan_func_exit?

And the generated code jumps to OpcUa_Float_P_NativeToWire
and skips the call to __builtin___tsan_func_exit.

So the rationale of the patch in tsan.c is, if we find any call, here,
it will certainly not be a tail call any more, thus I think
resetting that flag unconditionally here seems to be OK.