[Bug c++/40362] New: openmp: some libgomp functions trigger data races

[Bug c++/40362] New: openmp: some libgomp functions trigger data races

[bart dot vanassche at gmail dot com]

The source code of libgomp should be reviewed carefully whether all variables
that are accessed by more than one thread are protected by a consistent locking
strategy. When analyzing OpenMP programs with Helgrind or DRD, several data
races are reported. The test I ran myself and the results are as follows:
* Download Valgrind r10269 from the repository
svn://svn.valgrind.org/valgrind/trunk.
* Remove the libgomp suppression pattern from the file glibc-2.X-drd.supp.
* Build and install Valgrind.
* Recompile gcc 4.4.0 using the script drd/scripts/download-and-build-gcc
(which passes --disable-linux-futex to gcc's configure).
* Replace the distro-provided libgomp.so files by the freshly compiled ones.
* Check out the GraphicsMagick revision 2009-06-04 from CVS (see also
http://www.graphicsmagick.org/CVS.html for instructions).
* Run ./configure in the GraphicsMagick directory.
* Edit the drd: target in the generated Makefile and add the options
--check-stack-var=yes and --var-info=yes.
* Run the following command: export OMP_NUM_THREADS=4; make drd
* Inspect the files tests/*.log. A few extracts from these files:

>From drawtest.log:

==21970== Conflicting load by thread 3/3 at 0x07b411e8 size 8
==21970==    at 0x71A721F: gomp_work_share_start (ptrlock.h:42)
==21970==    by 0x71A466A: GOMP_loop_dynamic_start (loop.c:120)
==21970==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==21970==    by 0x71A6977: gomp_thread_start (team.c:115)
==21970==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==21970==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==21970==    by 0x769B10C: clone (in /lib64/libc-2.9.so)
==21970== Address 0x7b411e8 is at offset 376 from 0x7b41070. Allocation
context:
==21970==    at 0x4C249E9: malloc (vg_replace_malloc.c:193)
==21970==    by 0x71A2068: gomp_malloc (alloc.c:36)
==21970==    by 0x71A6FFC: gomp_new_team (team.c:143)
==21970==    by 0x71A5B2B: GOMP_parallel_start (parallel.c:108)
==21970==    by 0x449D71: PixelIterateMonoModify (pixel_iterator.c:230)
==21970==    by 0x431DAF: SetImage (image.c:4527)
==21970==    by 0x520198: ReadXCImage (xc.c:118)
==21970==    by 0x40B4FF: ReadImage (constitute.c:8478)
==21970==    by 0x40A006: main (drawtest.c:380)

==21970== Conflicting load by thread 3/3 at 0x07b41234 size 4
==21970==    at 0x71A35FD: gomp_iter_dynamic_next (iter.c:190)
==21970==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==21970==    by 0x71A6977: gomp_thread_start (team.c:115)
==21970==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==21970==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==21970==    by 0x769B10C: clone (in /lib64/libc-2.9.so)
==21970== Address 0x7b41234 is at offset 452 from 0x7b41070. Allocation
context:
==21970==    at 0x4C249E9: malloc (vg_replace_malloc.c:193)
==21970==    by 0x71A2068: gomp_malloc (alloc.c:36)
==21970==    by 0x71A6FFC: gomp_new_team (team.c:143)
==21970==    by 0x71A5B2B: GOMP_parallel_start (parallel.c:108)
==21970==    by 0x449D71: PixelIterateMonoModify (pixel_iterator.c:230)
==21970==    by 0x431DAF: SetImage (image.c:4527)
==21970==    by 0x520198: ReadXCImage (xc.c:118)
==21970==    by 0x40B4FF: ReadImage (constitute.c:8478)
==21970==    by 0x40A006: main (drawtest.c:380)

==21970== Conflicting load by thread 3/3 at 0x07b41240 size 8
==21970==    at 0x71A3601: gomp_iter_dynamic_next (iter.c:186)
==21970==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==21970==    by 0x71A6977: gomp_thread_start (team.c:115)
==21970==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==21970==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==21970==    by 0x769B10C: clone (in /lib64/libc-2.9.so)
==21970== Address 0x7b41240 is at offset 464 from 0x7b41070. Allocation
context:
==21970==    at 0x4C249E9: malloc (vg_replace_malloc.c:193)
==21970==    by 0x71A2068: gomp_malloc (alloc.c:36)
==21970==    by 0x71A6FFC: gomp_new_team (team.c:143)
==21970==    by 0x71A5B2B: GOMP_parallel_start (parallel.c:108)
==21970==    by 0x449D71: PixelIterateMonoModify (pixel_iterator.c:230)
==21970==    by 0x431DAF: SetImage (image.c:4527)
==21970==    by 0x520198: ReadXCImage (xc.c:118)
==21970==    by 0x40B4FF: ReadImage (constitute.c:8478)
==21970==    by 0x40A006: main (drawtest.c:380)

==21970== Conflicting load by thread 3/3 at 0x07b41248 size 8
==21970==    at 0x71A3605: gomp_iter_dynamic_next (iter.c:187)
==21970==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==21970==    by 0x71A6977: gomp_thread_start (team.c:115)
==21970==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==21970==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==21970==    by 0x769B10C: clone (in /lib64/libc-2.9.so)
==21970== Address 0x7b41248 is at offset 472 from 0x7b41070. Allocation
context:
==21970==    at 0x4C249E9: malloc (vg_replace_malloc.c:193)
==21970==    by 0x71A2068: gomp_malloc (alloc.c:36)
==21970==    by 0x71A6FFC: gomp_new_team (team.c:143)
==21970==    by 0x71A5B2B: GOMP_parallel_start (parallel.c:108)
==21970==    by 0x449D71: PixelIterateMonoModify (pixel_iterator.c:230)
==21970==    by 0x431DAF: SetImage (image.c:4527)
==21970==    by 0x520198: ReadXCImage (xc.c:118)
==21970==    by 0x40B4FF: ReadImage (constitute.c:8478)
==21970==    by 0x40A006: main (drawtest.c:380)

==21970== Conflicting load by thread 3/3 at 0x07b412a0 size 8
==21970==    at 0x71A3614: gomp_iter_dynamic_next (iter.c:192)
==21970==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==21970==    by 0x71A6977: gomp_thread_start (team.c:115)
==21970==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==21970==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==21970==    by 0x769B10C: clone (in /lib64/libc-2.9.so)
==21970== Address 0x7b412a0 is at offset 560 from 0x7b41070. Allocation
context:
==21970==    at 0x4C249E9: malloc (vg_replace_malloc.c:193)
==21970==    by 0x71A2068: gomp_malloc (alloc.c:36)
==21970==    by 0x71A6FFC: gomp_new_team (team.c:143)
==21970==    by 0x71A5B2B: GOMP_parallel_start (parallel.c:108)
==21970==    by 0x449D71: PixelIterateMonoModify (pixel_iterator.c:230)
==21970==    by 0x431DAF: SetImage (image.c:4527)
==21970==    by 0x520198: ReadXCImage (xc.c:118)
==21970==    by 0x40B4FF: ReadImage (constitute.c:8478)
==21970==    by 0x40A006: main (drawtest.c:380)


-- 
           Summary: openmp: some libgomp functions trigger data races
           Product: gcc
           Version: 4.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: bart dot vanassche at gmail dot com
 GCC build triplet: x86_64-unknown-linux-gnu
  GCC host triplet: x86_64-unknown-linux-gnu
GCC target triplet: x86_64-unknown-linux-gnu


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[pinskia at gcc dot gnu dot org]

------- Comment #1 from pinskia at gcc dot gnu dot org  2009-06-07 00:11 -------
>==21970==    at 0x71A35FD: gomp_iter_dynamic_next (iter.c:190)

Is a bogus warning as that is thread specific data:
  struct gomp_thread *thr = gomp_thread ();
  struct gomp_work_share *ws = thr->ts.work_share;


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[bart dot vanassche at gmail dot com]

------- Comment #2 from bart dot vanassche at gmail dot com  2009-06-07 06:14 -------
(In reply to comment #1)
> >==21970==    at 0x71A35FD: gomp_iter_dynamic_next (iter.c:190)
> 
> Is a bogus warning as that is thread specific data:
>   struct gomp_thread *thr = gomp_thread ();
>   struct gomp_work_share *ws = thr->ts.work_share;

Are you sure that *thr is only modified by a single thread ? The full DRD
output for this specific race shows that the data in *thr was modified by
thread 4 before thread 2 read that data, and that there was no proper
synchronization between the two accesses of *thr:

==14553== Thread 2:
==14553== Conflicting load by thread 2/2 at 0x07b41234 size 4
==14553==    at 0x71A35FD: gomp_iter_dynamic_next (iter.c:190)
==14553==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==14553==    by 0x71A6977: gomp_thread_start (team.c:115)
==14553==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==14553==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==14553==    by 0x769B10C: clone (in /lib64/libc-2.9.so)
==14553== Address 0x7b41234 is at offset 452 from 0x7b41070. Allocation
context:
==14553==    at 0x4C249E9: malloc (vg_replace_malloc.c:193)
==14553==    by 0x71A2068: gomp_malloc (alloc.c:36)
==14553==    by 0x71A6FFC: gomp_new_team (team.c:143)
==14553==    by 0x71A5B2B: GOMP_parallel_start (parallel.c:108)
==14553==    by 0x449D71: PixelIterateMonoModify (pixel_iterator.c:230)
==14553==    by 0x431DAF: SetImage (image.c:4527)
==14553==    by 0x520198: ReadXCImage (xc.c:118)
==14553==    by 0x40B4FF: ReadImage (constitute.c:8478)
==14553==    by 0x40A006: main (drawtest.c:380)
==14553== Other segment start (thread 4/4)
==14553==    at 0x4C25A56: pthread_mutex_lock (drd_pthread_intercepts.c:489)
==14553==    by 0x71A7266: gomp_work_share_start (mutex.h:44)
==14553==    by 0x71A466A: GOMP_loop_dynamic_start (loop.c:120)
==14553==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==14553==    by 0x71A6977: gomp_thread_start (team.c:115)
==14553==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==14553==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==14553==    by 0x769B10C: clone (in /lib64/libc-2.9.so)
==14553== Other segment end (thread 4/4)
==14553==    at 0x4C25D91: pthread_mutex_unlock (drd_pthread_intercepts.c:535)
==14553==    by 0x71A4748: GOMP_loop_dynamic_start (mutex.h:49)
==14553==    by 0x449AE7: PixelIterateMonoModify.omp_fn.3
(pixel_iterator.c:230)
==14553==    by 0x71A6977: gomp_thread_start (team.c:115)
==14553==    by 0x4C287BB: vgDrd_thread_wrapper (drd_pthread_intercepts.c:224)
==14553==    by 0x73B306F: start_thread (in /lib64/libpthread-2.9.so)
==14553==    by 0x769B10C: clone (in /lib64/libc-2.9.so)


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[pinskia at gcc dot gnu dot org]

------- Comment #3 from pinskia at gcc dot gnu dot org  2009-06-07 06:22 -------
Except this is a false positive as thread 4 is not created when the thread 2
writes to *thr.  It looks like valgrind does not know what is happening here
really.


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[bart dot vanassche at gmail dot com]

------- Comment #4 from bart dot vanassche at gmail dot com  2009-06-07 06:52 -------
(In reply to comment #3)
> Except this is a false positive as thread 4 is not created when the thread 2
> writes to *thr.  It looks like valgrind does not know what is happening here
> really.

I'm not sure that we both did interpret the above information in the same way.
What makes you think that thread 4 was not yet created at the time thread 2
wrote to *thr ?


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[pinskia at gcc dot gnu dot org]

------- Comment #5 from pinskia at gcc dot gnu dot org  2009-06-07 06:57 -------
You have to read the code to understand how that happens.
But basically GOMP_parallel_start does:
  gomp_team_start (fn, data, num_threads, gomp_new_team (num_threads));

Where gomp_new_team creates the *thr (which is a big malloc) and then
gomp_team_start creates the threads (and also does the write before creating
the thread).


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[bart dot vanassche at gmail dot com]

------- Comment #6 from bart dot vanassche at gmail dot com  2009-06-07 07:09 -------
(In reply to comment #5)
> You have to read the code to understand how that happens.
> But basically GOMP_parallel_start does:
>   gomp_team_start (fn, data, num_threads, gomp_new_team (num_threads));
> 
> Where gomp_new_team creates the *thr (which is a big malloc) and then
> gomp_team_start creates the threads (and also does the write before creating
> the thread).

As far as I can see gomp_new_team() only creates threads upon the first
invocation or when its third argument is larger than for any previous
invocation. So I'm still not convinced that comment #3 is correct.


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[jakub at gcc dot gnu dot org]

------- Comment #7 from jakub at gcc dot gnu dot org  2009-06-07 13:07 -------
valgrind just isn't smart enough to understand it.
Obviously --enable-linux-futex build has a lot of synchronization primitives
that are beyond what valgrind is able to understand, but even with the posix
only primitives e.g.:
static inline void *gomp_ptrlock_get (gomp_ptrlock_t *ptrlock)
{
  if (ptrlock->ptr != NULL)
    return ptrlock->ptr;

  gomp_mutex_lock (&ptrlock->lock);
  if (ptrlock->ptr != NULL)
    {
      gomp_mutex_unlock (&ptrlock->lock);
      return ptrlock->ptr;
    }

  return NULL;
}
is not something valgrind can understand.  Try removing the first if/return and
see if it helps drd.  The reason why it is safe to do is that all supported
libgomp targets have atomic pointer-sized loads/stores, and the pointer always
starts as NULL and is only set to non-NULL value inside of a critical section
guarded by the associated lock.  Once it is non-NULL, it never changes its
value again.  So, if the inline sees the value non-NULL, it can safely assume
it will be non-NULL all the time and doesn't have to take the lock...


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[bart dot vanassche at gmail dot com]

------- Comment #8 from bart dot vanassche at gmail dot com  2009-06-08 18:30 -------
(In reply to comment #7)

Thanks for the feedback. I'll use the above information to suppress the
complaints on thr->ts.work_share->next_ws and will post the results here.


-- 


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

[Bug c++/40362] openmp: some libgomp functions trigger data races

[bart dot vanassche at gmail dot com]

------- Comment #9 from bart dot vanassche at gmail dot com  2009-06-13 16:23 -------
(In reply to comment #7)

The patch below is sufficient to suppress all conflicting accesses reported by
DRD. I've done my best to ensure that this patch is not only sufficient but
also minimal. Although Jakub Jelinek has already explained that the access
pattern of gomp_work_share::next_ws is safe, there are several other members of
struct gomp_work_share that have to be verified too.

$ diff -upr orig/gcc-4.4.0/libgomp gcc-4.4.0/libgomp
diff -upr orig/gcc-4.4.0/libgomp/work.c gcc-4.4.0/libgomp/work.c
--- orig/gcc-4.4.0/libgomp/work.c       2009-04-10 01:23:07.000000000 +0200
+++ gcc-4.4.0/libgomp/work.c    2009-06-13 18:00:55.000000000 +0200        
@@ -29,6 +29,7 @@
 #include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
+#include <valgrind/drd.h>


 /* Allocate a new work share structure, preferably from current team's
@@ -92,6 +93,14 @@ void
 gomp_init_work_share (struct gomp_work_share *ws, bool ordered,
                      unsigned nthreads)
 {
+  DRD_IGNORE_VAR(ws->chunk_size);
+  DRD_IGNORE_VAR(ws->end);
+  DRD_IGNORE_VAR(ws->incr);
+  DRD_IGNORE_VAR(ws->mode);
+  DRD_IGNORE_VAR(ws->next);
+  DRD_IGNORE_VAR(ws->next_ws);
+  DRD_IGNORE_VAR(ws->threads_completed);
+
   gomp_mutex_init (&ws->lock);
   if (__builtin_expect (ordered, 0))
     {


-- 


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