I think it is a real data races bugs, or an order violation(the order of two
write operations), look at the following the scenario, (we just treat *lock)
as a normal global shared struct variable X and it has two membersas:
struct X ｛
BOOL initialized_flags
int data
}
struct X x;
thread1:
if( x.data == NULL) {
lock
x.initialized_flags =
x.data = initialized_variables
unlock
}
= x.data ( it's first use)
we must enforce that x.data is used after its initialization. I think your
fix is a very good fix. Please file a bug in the bugzillar.
Thanks for your great finding.
Tianwei
On Tue, Sep 21, 2010 at 1:21 PM, Jiangzhou HE <hejiangz...@gmail.com> wrote:
> 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
>
>
>
> ------------------------------------------------------------------------------
> 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
>
>
--
Sheng, Tianwei
Inst. of High Performance Computing
Dept. of Computer Sci. & Tech.
Tsinghua Univ.
------------------------------------------------------------------------------
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
