Forget to mention, your fix is good, you can check in that and record that
in the bug database.
Tianwei
On Tue, Sep 21, 2010 at 2:56 PM, Tianwei <[email protected]> wrote:
> 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 <[email protected]>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
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/open64-devel
>>
>>
>
>
> --
> Sheng, Tianwei
> Inst. of High Performance Computing
> Dept. of Computer Sci. & Tech.
> Tsinghua Univ.
>
--
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/open64-devel
