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

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

[bernd.edlinger at hotmail dot de]

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

            Bug ID: 65400
           Summary: tsan mis-compiles inlineable C functions
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bernd.edlinger at hotmail dot de
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org

Created attachment 35018
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35018&action=edit
stripped down test case

Hi,

I am not sure when this started, probably in february where I was
busy with other tasks, but current trunk miscompiles numerous
simple C functions if optimizations are enabled.  I isolated one example
in the OPC UA ANSI-C stack, and crated a small test case from it.

It mis-compiles with these set of options:

gcc -O2 -fsanitize=thread,undefined -c opcua_string.c

the problem is here:

00000000000004b0 <OpcUa_String_Clear>:
 4b0:   53                      push   %rbx
 4b1:   48 89 fb                mov    %rdi,%rbx
 4b4:   48 8b 7c 24 08          mov    0x8(%rsp),%rdi
 4b9:   e8 00 00 00 00          callq  4be <OpcUa_String_Clear+0xe>
                        4ba: R_X86_64_PC32      __tsan_func_entry-0x4
 4be:   48 85 db                test   %rbx,%rbx
 4c1:   74 1d                   je     4e0 <OpcUa_String_Clear+0x30>
 4c3:   48 89 df                mov    %rbx,%rdi
 4c6:   e8 00 00 00 00          callq  4cb <OpcUa_String_Clear+0x1b>
                        4c7: R_X86_64_PC32      __tsan_read1-0x4
 4cb:   80 3b 00                cmpb   $0x0,(%rbx)
 4ce:   75 10                   jne    4e0 <OpcUa_String_Clear+0x30>
 4d0:   48 89 df                mov    %rbx,%rdi
 4d3:   5b                      pop    %rbx
 4d4:   e9 27 fb ff ff          jmpq   0 <OpcUa_String_Clear.part.0>
 4d9:   0f 1f 80 00 00 00 00    nopl   0x0(%rax)
 4e0:   5b                      pop    %rbx
 4e1:   e9 00 00 00 00          jmpq   4e6 <OpcUa_String_Clear+0x36>
                        4e2: R_X86_64_PC32      __tsan_func_exit-0x4


a call to __tsan_func_exit is missing before jmpq <OpcUa_String_Clear.part.0>

note that OpcUa_String_Clear.part.0 also call __tsan_func_entry
thus the call stack is completely scrambled:

0000000000000000 <OpcUa_String_Clear.part.0>:
   0:   55                      push   %rbp
   1:   53                      push   %rbx
   2:   48 89 fb                mov    %rdi,%rbx
   5:   48 83 ec 08             sub    $0x8,%rsp
   9:   48 8b 7c 24 18          mov    0x18(%rsp),%rdi
   e:   e8 00 00 00 00          callq  13 <OpcUa_String_Clear.part.0+0x13>
                        f: R_X86_64_PC32        __tsan_func_entry-0x4


The with other optimization levels or without -fsanitize=undefined,
this example compiles correctly, but some other function start
to have problems.

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

[bernd.edlinger at hotmail dot de]

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

--- Comment #1 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
I think the trouble starts at opcua_string.c.039t.fnsplit :

OpcUa_String_Clear (struct OpcUa_String * a_pString)
{
 ...
  <bb 5>:
  # _25 = PHI <0(2), 1(3), 0(4)>
  if (_25 == 0)
    goto <bb 7>;
  else
    goto <bb 6>;

  <bb 6>:
  OpcUa_String_Clear.part.0 (a_pString_3(D));
  goto <bb 8>;

  <bb 7>:
  TSAN_FUNC_EXIT ();

  <bb 8>:
  return;

}

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

[bernd.edlinger at hotmail dot de]

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

--- Comment #2 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Jakub,

at that time we do not have the __tsan_func_entry calls, only
the __TSAN_FUNC_EXIT thing.

in opcua_string.c.038t.local-pure-const1
OpcUa_String_Clear looks like it has OpcUa_String_Clear.part.0
at the end.  folding that into a call to OpcUa_String_Clear.part.0
eats away the __TSAN_FUNC_EXIT.

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

[bernd.edlinger at hotmail dot de]

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

--- Comment #3 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Hmm, 


I tried this:

--- gcc/ipa-split.c.jj    2015-02-08 21:13:01.000000000 +0100
+++ gcc/ipa-split.c    2015-03-13 11:29:08.878923384 +0100
@@ -1509,6 +1509,16 @@ split_function (struct split_point *spli
       || DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))))
     gimple_call_set_return_slot_opt (call, true);

+  /* Re-insert a TSAN_FUNC_EXIT immediately _before_ the actual call,
+     because we are actually calling ourselves, so the call stack
+     should look correct this way, and it does not prevent the
+     possible tail-call optimization.  */
+  if ((flag_sanitize & SANITIZE_THREAD) != 0
+      && !lookup_attribute ("no_sanitize_thread",
+                DECL_ATTRIBUTES (current_function_decl)))
+    gsi_insert_after (&gsi, gimple_build_call_internal (IFN_TSAN_FUNC_EXIT,
0),
+              GSI_NEW_STMT);
+
   /* Update return value.  This is bit tricky.  When we do not return,
      do nothing.  When we return we might need to update return_bb
      or produce a new return statement.  */


BUT there are more problems (in another C file):

opcua_p_binary.c.103t.sink-OpcUa_Int32_P_NativeToWire (OpcUa_Int32_Wire * wire,
OpcUa_Int32 * native)
opcua_p_binary.c.103t.sink-{
opcua_p_binary.c.103t.sink-  OpcUa_StatusCode retval.25;
opcua_p_binary.c.103t.sink-
opcua_p_binary.c.103t.sink-  <bb 2>:
opcua_p_binary.c.103t.sink:  retval.25_5 = OpcUa_Float_P_NativeToWire
(wire_2(D), native_3(D)); [tail call]
opcua_p_binary.c.103t.sink:  return retval.25_5;
opcua_p_binary.c.103t.sink-
opcua_p_binary.c.103t.sink-}
--
opcua_p_binary.c.105t.tsan1-OpcUa_Int32_P_NativeToWire (OpcUa_Int32_Wire *
wire, OpcUa_Int32 * native)
opcua_p_binary.c.105t.tsan1-{
opcua_p_binary.c.105t.tsan1-  OpcUa_StatusCode retval.25;
opcua_p_binary.c.105t.tsan1-  void * _6;
opcua_p_binary.c.105t.tsan1-
opcua_p_binary.c.105t.tsan1-  <bb 2>:
opcua_p_binary.c.105t.tsan1-  _6 = __builtin_return_address (0);
opcua_p_binary.c.105t.tsan1-  __builtin___tsan_func_entry (_6);
opcua_p_binary.c.105t.tsan1:  retval.25_5 = OpcUa_Float_P_NativeToWire
(wire_2(D), native_3(D)); [tail call]
opcua_p_binary.c.105t.tsan1-  __builtin___tsan_func_exit ();
opcua_p_binary.c.105t.tsan1:  return retval.25_5;
opcua_p_binary.c.105t.tsan1-
opcua_p_binary.c.105t.tsan1-}

objdump -d shows this!
00000000021755a0 <OpcUa_Int32_P_NativeToWire>:
 21755a0:       55                      push   %rbp
 21755a1:       53                      push   %rbx
 21755a2:       48 89 fb                mov    %rdi,%rbx
 21755a5:       48 89 f5                mov    %rsi,%rbp
 21755a8:       48 83 ec 08             sub    $0x8,%rsp
 21755ac:       48 8b 7c 24 18          mov    0x18(%rsp),%rdi
 21755b1:       e8 6a 72 29 fe          callq  40c820 <__tsan_func_entry@plt>
 21755b6:       48 83 c4 08             add    $0x8,%rsp
 21755ba:       48 89 ee                mov    %rbp,%rsi
 21755bd:       48 89 df                mov    %rbx,%rdi
 21755c0:       5b                      pop    %rbx
 21755c1:       5d                      pop    %rbp
 21755c2:       e9 19 ff ff ff          jmpq   21754e0
<OpcUa_Float_P_NativeToWire>
 21755c7:       66 0f 1f 84 00 00 00    nopw   0x0(%rax,%rax,1)
 21755ce:       00 00

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

[bernd.edlinger at hotmail dot de]

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

--- Comment #4 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
ok,

this should fix the second regression:

Index: tsan.c
===================================================================
--- tsan.c    (revision 221355)
+++ tsan.c    (working copy)
@@ -682,6 +682,7 @@
     {
       if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
     instrument_builtin_call (gsi);
+      gimple_call_set_tail (as_a <gcall *> (stmt), false);
       return true;
     }
   else if (is_gimple_assign (stmt)


--
opcua_p_binary.c.103t.sink-{
opcua_p_binary.c.103t.sink-  OpcUa_StatusCode retval.25;
opcua_p_binary.c.103t.sink-
opcua_p_binary.c.103t.sink-  <bb 2>:
opcua_p_binary.c.103t.sink:  retval.25_5 = OpcUa_Float_P_NativeToWire
(wire_2(D), native_3(D)); [tail call]
opcua_p_binary.c.103t.sink:  return retval.25_5;
opcua_p_binary.c.103t.sink-
opcua_p_binary.c.103t.sink-}
--
opcua_p_binary.c.105t.tsan1-  <bb 2>:
opcua_p_binary.c.105t.tsan1-  _6 = __builtin_return_address (0);
opcua_p_binary.c.105t.tsan1-  __builtin___tsan_func_entry (_6);
opcua_p_binary.c.105t.tsan1:  retval.25_5 = OpcUa_Float_P_NativeToWire
(wire_2(D), native_3(D));
opcua_p_binary.c.105t.tsan1-  __builtin___tsan_func_exit ();
opcua_p_binary.c.105t.tsan1:  return retval.25_5;
opcua_p_binary.c.105t.tsan1-
opcua_p_binary.c.105t.tsan1-}

see the "[tail call]" is removed,
and the generated code is now OK.

trying to use -O3 next.

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

[bernd.edlinger at hotmail dot de]

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

--- Comment #5 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Created attachment 35033
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35033&action=edit
stripped down second test case

For completeness, here is the stripped down test case for the tail call flag.

It reproduces with current trunk and:
[-O2|-Os] -fsanitize=treand[,undefined]

not with -O1 and not with -O3, and not in january either :)

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

[bernd.edlinger at hotmail dot de]

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.

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

[jakub at gcc dot gnu.org]

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 35059
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35059&action=edit
gcc5-pr65400.patch

Untested fix for the first issue.

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

[bernd.edlinger at hotmail dot de]

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

--- Comment #9 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
(In reply to Jakub Jelinek from comment #8)
> Created attachment 35059 [details]
> gcc5-pr65400.patch
> 
> Untested fix for the first issue.

patch looks good!

and a really nice test case.

one minor nit on the test case:
I think the -g in the dg-additional-options is not necesary,
as that is alread added in tsan.exp somewhere.

Thanks.

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

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Thu Mar 19 07:53:38 2015
New Revision: 221508

URL: https://gcc.gnu.org/viewcvs?rev=221508&root=gcc&view=rev
Log:
    PR sanitizer/65400
    * ipa-split.c (find_return_bb): Allow TSAN_FUNC_EXIT internal
    call in the return bb.
    (find_split_points): Add RETURN_BB argument, don't call
    find_return_bb.
    (split_function): Likewise.  Add ADD_TSAN_FUNC_EXIT argument,
    if true append TSAN_FUNC_EXIT internal call after the call to
    the split off function.
    (execute_split_functions): Call find_return_bb here.
    Don't optimize if TSAN_FUNC_EXIT is found in unexpected places.
    Adjust find_split_points and split_function calls.

    * c-c++-common/tsan/pr65400-1.c: New test.
    * c-c++-common/tsan/pr65400-2.c: New test.

Added:
    trunk/gcc/testsuite/c-c++-common/tsan/pr65400-1.c
    trunk/gcc/testsuite/c-c++-common/tsan/pr65400-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-split.c
    trunk/gcc/testsuite/ChangeLog

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

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
   Target Milestone|---                         |5.0
            Summary|tsan mis-compiles           |[5 Regression] tsan
                   |inlineable C functions      |mis-compiles inlineable C
                   |                            |functions

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

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

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Thu Mar 19 10:12:34 2015
New Revision: 221512

URL: https://gcc.gnu.org/viewcvs?rev=221512&root=gcc&view=rev
Log:
    PR sanitizer/65400
    * tsan.c (instrument_gimple): Clear tail call flag on
    calls.

    * c-c++-common/tsan/pr65400-3.c: New test.

Added:
    trunk/gcc/testsuite/c-c++-common/tsan/pr65400-3.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tsan.c