[Open64-devel] Ask for review of a data race bug fix of OpenMP library

[Jiangzhou HE]

Hi, all,

I found a bug in osprey/libopenmp/omp_lock.c. See the following function:

inline void
__ompc_critical(int gtid, volatile ompc_lock_t **lck)
{
  if (**lck == NULL*) { *//(1)*
    __ompc_lock_spinlock(&_ompc_thread_lock);
    if ((ompc_lock_t*)*lck == NULL){
      // put the shared data aligned with cache line
*      *lck = aligned_malloc(sizeof(ompc_lock_t), CACHE_LINE_SIZE); //(2)
*      Is_True(*lck!=NULL,
              ("Cannot allocate lock memory for critical"));
*      __ompc_init_lock (*lck); //(3)*
    }
    __ompc_unlock_spinlock(&_ompc_thread_lock);
  }
  *__ompc_lock((volatile ompc_lock_t *)*lck);* *//(4)*
}

Bug happens in the following execution sequence:

thread1   thread2
(1)
(2)
            (1)
            (4)   *// Error! Trying to lock a uninitialized lock.*
(3)

This is my patch of this bug:

diff --git a/osprey/libopenmp/omp_lock.c b/osprey/libopenmp/omp_lock.c
index 0781647..718117c 100644
--- a/osprey/libopenmp/omp_lock.c
+++ b/osprey/libopenmp/omp_lock.c
@@ -197,10 +197,11 @@ __ompc_critical(int gtid, volatile ompc_lock_t **lck)
     __ompc_lock_spinlock(&_ompc_thread_lock);
     if ((ompc_lock_t*)*lck == NULL){
       // put the shared data aligned with cache line
-      *lck = aligned_malloc(sizeof(ompc_lock_t), CACHE_LINE_SIZE);
-      Is_True(*lck!=NULL,
+      volatile ompc_lock_t* new_lock = aligned_malloc(sizeof(ompc_lock_t),
CACHE_LINE_SIZE);
+      Is_True(new_lock!=NULL,
              ("Cannot allocate lock memory for critical"));
-      __ompc_init_lock (*lck);
+      __ompc_init_lock (new_lock);
+      *lck = new_lock;
     }
     __ompc_unlock_spinlock(&_ompc_thread_lock);
   }

Could the gatekeeper help to review this patch?

Thanks!

Jiangzhou

-- 
Jiangzhou HE (何江舟)

Institute of High-Performance Computing
Department of Computer Science and Technology
Tsinghua University, Beijing, China

diff --git a/osprey/libopenmp/omp_lock.c b/osprey/libopenmp/omp_lock.c
index 0781647..718117c 100644
--- a/osprey/libopenmp/omp_lock.c
+++ b/osprey/libopenmp/omp_lock.c
@@ -197,10 +197,11 @@ __ompc_critical(int gtid, volatile ompc_lock_t **lck)
     __ompc_lock_spinlock(&_ompc_thread_lock);
     if ((ompc_lock_t*)*lck == NULL){
       // put the shared data aligned with cache line
-      *lck = aligned_malloc(sizeof(ompc_lock_t), CACHE_LINE_SIZE);
-      Is_True(*lck!=NULL, 
+      volatile ompc_lock_t* new_lock = aligned_malloc(sizeof(ompc_lock_t), CACHE_LINE_SIZE);
+      Is_True(new_lock!=NULL, 
 	      ("Cannot allocate lock memory for critical"));
-      __ompc_init_lock (*lck);
+      __ompc_init_lock (new_lock);
+      *lck = new_lock;
     }
     __ompc_unlock_spinlock(&_ompc_thread_lock);
   }

------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances
and start using them to simplify application deployment and
accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel