[Bug libgomp/59194] New: tsan detects race for real variables in an OMP reduction clause

[Bug libgomp/59194] New: tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

            Bug ID: 59194
           Summary: tsan detects race for real variables in an OMP
                    reduction clause
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libgomp
          Assignee: unassigned at gcc dot gnu.org
          Reporter: Joost.VandeVondele at mat dot ethz.ch
                CC: jakub at gcc dot gnu.org

This seems either a bug in libgomp or tsan, not clear which one.

To reproduce, libgomp must be compiled first with -fsanitize=thread as
described in

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55561#c15

and the following workaround applied to get tsan to work with trunk.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59188#c3

The following program:

> cat test.f90 
  INTEGER :: i
  REAL  :: m
  m=0.0
  !$OMP PARALLEL DO REDUCTION(+:m)
  DO i=1,10
     m=m+1.0
  ENDDO
  IF (m.NE.10.0) CALL ABORT()
END

compiled and executed as

gfortran -g -fPIE -pie -fopenmp -fsanitize=thread test.f90 ; export
OMP_NUM_THREADS=2 ; ./a.out

triggers the error below. Interestingly, this only happens if 'm' is decleared
REAL, not if INTEGER.

==================
WARNING: ThreadSanitizer: data race (pid=13744)
  Atomic write of size 4 at 0x7fffc5e9bfa0 by main thread:                      
    #0 __tsan_atomic32_compare_exchange_strong ??:0
(libtsan.so.0+0x000000016da2)                                                   
    #1 MAIN__._omp_fn.0 test.f90:0 (exe+0x000000000ed1)
    #2 GOMP_parallel
/data/vjoost/gnu/gcc_trunk/obj/x86_64-unknown-linux-gnu/libgomp/../../../gcc/libgomp/parallel.c:167
(libgomp.so.1+0x00000000e98c)
    #3 MAIN__ test.f90:0 (exe+0x000000000d73)
    #4 main ??:0 (exe+0x000000000df5)

  Previous read of size 4 at 0x7fffc5e9bfa0 by thread T1:
    #0 MAIN__._omp_fn.0 test.f90:0 (exe+0x000000000e9d)                         
    #1 gomp_thread_start
/data/vjoost/gnu/gcc_trunk/obj/x86_64-unknown-linux-gnu/libgomp/../../../gcc/libgomp/team.c:117
(libgomp.so.1+0x000000014e40)

  Location is stack of main thread.

  Thread T1 (tid=13746, running) created by main thread at:                     
    #0 pthread_create
/data/vjoost/gnu/gcc_trunk/obj/x86_64-unknown-linux-gnu/libsanitizer/tsan/../../../../gcc/libsanitizer/tsan/tsan_interceptors.cc:853
(libtsan.so.0+0x000000035b1a)
    #1 gomp_team_start
/data/vjoost/gnu/gcc_trunk/obj/x86_64-unknown-linux-gnu/libgomp/../../../gcc/libgomp/team.c:795
(libgomp.so.1+0x000000015686)
    #2 GOMP_parallel
/data/vjoost/gnu/gcc_trunk/obj/x86_64-unknown-linux-gnu/libgomp/../../../gcc/libgomp/parallel.c:166
(libgomp.so.1+0x00000000e987)
    #3 MAIN__ test.f90:0 (exe+0x000000000d73)
    #4 main ??:0 (exe+0x000000000df5)

SUMMARY: ThreadSanitizer: data race ??:0
__tsan_atomic32_compare_exchange_strong
==================

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think this is still a dup of bug 40362.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Joost.VandeVondele at mat dot ethz
                   |                            |.ch

--- Comment #2 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Andrew Pinski from comment #1)
> I think this is still a dup of bug 40362.

I think the linux-futex case should be fixed by
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55561#c38
PR40362 is still relevant for --disable-linux-futex

However, admittedly, I don't know this topic well enough to figure this out
from the code myself.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #3 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
actually it seems more general an issue, the following:

SUBROUTINE S1(m)
  REAL  :: m
  !$OMP ATOMIC
  m=m+1.0
END

REAL :: m
m=0.0
!$OMP PARALLEL 
CALL S1(m)
!$OMP END PARALLEL
END

flags  race for the atomic update of m. Again, does work for integers but not
for reals.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Because CPUs obviously don't have floating point atomic instructions, what the
compiler does is just load it as an integer, view convert to floating point,
perform arithmetics, view convert result back to integer, and compare and swap
(if unsuccessful loop).  I bet tsan complains because the load is not atomic,
but does it really matter?  If we read garbage there, compare and swap will
fail and next time we'll have hopefully correct value already from what compare
and swap said was the previous value.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #5 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Jakub Jelinek from comment #4)
> I bet tsan complains because the load is
> not atomic, but does it really matter?  

I think there are (at least) two possible answers to this.

1) No, it doesn't matter, because we get the right result. This actually is
great, already and your and other gcc-ers effort is much appreciated.

2) But yes, it does. For users, having a tool to help verify/debug an OMP
program is very valuable. tsan is pretty close to doing being exactly that, and
the gomp+tsan combination would be really a plus for gcc, increasing in
importance as OMP (or related variants) becomes more and more common. Such
false positives reduce the value of the gomp+tsan tool significantly.
Therefore, if these issues could be fixed easily without significant downside,
I think this should be done. I'd happy to try patches, and do some tsan+omp
testing.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #6 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Jakub Jelinek from comment #4)
> I bet tsan complains because the load is
> not atomic, but does it really matter?  If we read garbage there, compare
> and swap will fail and next time we'll have hopefully correct value already
> from what compare and swap said was the previous value.

Actually, I believe this might fail. If the garbage read happens to be equal to
what a 3rd thread writes in the meanwhile (admittedly, small change), the
compare and swap will succeed and not fail.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
And the problem with that is?  Because the arithmetics is based on the value
we've read, it shouldn't be a problem.

That said, during stage3 I'll look at how costly would be to use there
__atomic_load_n with MEMMODEL_RELAXED.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #8 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Jakub Jelinek from comment #7)
> And the problem with that is?  Because the arithmetics is based on the value
> we've read, it shouldn't be a problem.

Ah...right. 

> That said, during stage3 I'll look at how costly would be to use there
> __atomic_load_n with MEMMODEL_RELAXED.

Thanks!

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[dvyukov at google dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

Dmitry Vyukov <dvyukov at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dvyukov at google dot com

--- Comment #9 from Dmitry Vyukov <dvyukov at google dot com> ---
(In reply to Jakub Jelinek from comment #4)
> Because CPUs obviously don't have floating point atomic instructions, what
> the compiler does is just load it as an integer, view convert to floating
> point, perform arithmetics, view convert result back to integer, and compare
> and swap (if unsuccessful loop).  I bet tsan complains because the load is
> not atomic, but does it really matter?  If we read garbage there, compare
> and swap will fail and next time we'll have hopefully correct value already
> from what compare and swap said was the previous value.

Data races do not work this way. It's undefined behavior:
http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

The code must use relaxed atomic load. If the code becomes slower due to that
then:
(1) the current generated code is incorrect
or (2) there is a performance bug in gcc handling of atomic operations

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #10 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Jakub Jelinek from comment #7)
> That said, during stage3 I'll look at how costly would be to use there
> __atomic_load_n with MEMMODEL_RELAXED.

any chance this could be looked at in the remaining part of stage3? I can
benchmark the patch on our application if this makes sense.

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Mon Jan 13 07:56:40 2014
New Revision: 206572

URL: http://gcc.gnu.org/viewcvs?rev=206572&root=gcc&view=rev
Log:
    PR libgomp/59194
    * omp-low.c (expand_omp_atomic_pipeline): Expand the initial
    load as __atomic_load_N if possible.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/omp-low.c

[Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause

[Joost.VandeVondele at mat dot ethz.ch]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59194

--- Comment #12 from Joost VandeVondele <Joost.VandeVondele at mat dot ethz.ch> ---
(In reply to Jakub Jelinek from comment #11)
> Author: jakub
> Date: Mon Jan 13 07:56:40 2014
> New Revision: 206572
> 
> URL: http://gcc.gnu.org/viewcvs?rev=206572&root=gcc&view=rev
> Log:
> 	PR libgomp/59194
> 	* omp-low.c (expand_omp_atomic_pipeline): Expand the initial
> 	load as __atomic_load_N if possible.
> 
> Modified:
>     trunk/gcc/ChangeLog
>     trunk/gcc/omp-low.c

I have verified that this patch fixes the reported issues. From my point of
view, the PR can be closed as fixed. Thanks!
>From gcc-bugs-return-440196-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Mon Jan 13 10:56:25 2014
Return-Path: <gcc-bugs-return-440196-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 22396 invoked by alias); 13 Jan 2014 10:56:25 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 22353 invoked by uid 48); 13 Jan 2014 10:56:21 -0000
From: "jakub at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug libgomp/59194] tsan detects race for real variables in an OMP reduction clause
Date: Mon, 13 Jan 2014 10:56:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: libgomp
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: jakub at gcc dot gnu.org
X-Bugzilla-Status: RESOLVED
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: bug_status resolution
Message-ID: <bug-59194-4-IfuD9AcB3p@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-59194-4@http.gcc.gnu.org/bugzilla/>
References: <bug-59194-4@http.gcc.gnu.org/bugzilla/>
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: 2014-01/txt/msg01338.txt.bz2
Content-length: 427

http://gcc.gnu.org/bugzilla/show_bug.cgi?idY194

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

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