From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28941 invoked by alias); 16 Mar 2015 20:32:17 -0000 Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-bugs-owner@gcc.gnu.org Received: (qmail 28882 invoked by uid 48); 16 Mar 2015 20:32:11 -0000 From: "bernd.edlinger at hotmail dot de" To: gcc-bugs@gcc.gnu.org Subject: [Bug sanitizer/65400] tsan mis-compiles inlineable C functions Date: Mon, 16 Mar 2015 20:32:00 -0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: sanitizer X-Bugzilla-Version: 5.0 X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: bernd.edlinger at hotmail dot de X-Bugzilla-Status: UNCONFIRMED X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-SW-Source: 2015-03/txt/msg01636.txt.bz2 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65400 --- Comment #7 from Bernd Edlinger --- (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; : 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; : _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; : _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.