From 9eead8746a5812c60efee97a68423d413534c756 Mon Sep 17 00:00:00 2001 From: Ilan Truanovsky Date: Thu, 16 Feb 2023 11:24:18 -0800 Subject: [PATCH] Remove trylock + unlock in `acl_reset_condvar()` See https://github.com/intel/fpga-runtime-for-opencl/issues/276 for an explanation on why this needs to be done. In short, assuming library destructors work as we expect (i.e. they do not get called on abort), this code is unnecessary and can introduce undefined behavior. This will also fix the following Coverity issues: ``` lib/acl_threadsupport/test/acl_threadsupport_test.cpp:333:3: Type: Double unlock (LOCK) lib/acl_threadsupport/test/acl_threadsupport_test.cpp:332:3: 1. unlock: "acl_release_condvar" unlocks "cc.waiter_mutex". lib/acl_threadsupport/src/acl_threadsupport.c:375:5: 1.1. unlock: "pthread_mutex_unlock" unlocks "C->waiter_mutex". lib/acl_threadsupport/test/acl_threadsupport_test.cpp:333:3: 2. double_unlock: "acl_reset_condvar" unlocks "cc.waiter_mutex" while it is unlocked. lib/acl_threadsupport/src/acl_threadsupport.c:300:5: 2.1. unlock: "pthread_mutex_unlock" unlocks "C->waiter_mutex". lib/acl_threadsupport/src/acl_threadsupport.c:301:5: 2.2. destroy: "pthread_mutex_destroy" destroys "C->waiter_mutex". lib/acl_threadsupport/test/acl_threadsupport_test.cpp:353:3: Type: Double unlock (LOCK) lib/acl_threadsupport/test/acl_threadsupport_test.cpp:345:3: 1. path: Condition "i < 10000000U /* COUNT */", taking true branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:346:5: 2. path: Condition "0 == i % (1000000U /* COUNT / 10 */)", taking true branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:352:3: 3. path: Jumping back to the beginning of the loop. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:345:3: 4. path: Condition "i < 10000000U /* COUNT */", taking true branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:346:5: 5. path: Condition "0 == i % (1000000U /* COUNT / 10 */)", taking false branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:351:5: 6. unlock: "acl_release_condvar" unlocks "cc.waiter_mutex". lib/acl_threadsupport/src/acl_threadsupport.c:375:5: 6.1. unlock: "pthread_mutex_unlock" unlocks "C->waiter_mutex". lib/acl_threadsupport/test/acl_threadsupport_test.cpp:352:3: 7. path: Jumping back to the beginning of the loop. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:345:3: 8. path: Condition "i < 10000000U /* COUNT */", taking false branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:353:3: 9. double_unlock: "acl_reset_condvar" unlocks "cc.waiter_mutex" while it is unlocked. lib/acl_threadsupport/src/acl_threadsupport.c:300:5: 9.1. unlock: "pthread_mutex_unlock" unlocks "C->waiter_mutex". lib/acl_threadsupport/src/acl_threadsupport.c:301:5: 9.2. destroy: "pthread_mutex_destroy" destroys "C->waiter_mutex". lib/acl_threadsupport/test/acl_threadsupport_test.cpp:477:3: Type: Double unlock (LOCK) lib/acl_threadsupport/test/acl_threadsupport_test.cpp:464:3: 1. path: Condition "1 != acl_timed_wait_condvar(&cc, 1)", taking false branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:465:3: 2. path: Condition "1 != cc.timedout[0]", taking true branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:465:3: 3. path: Falling through to end of if statement. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:473:3: 4. path: Condition "0 != acl_timed_wait_condvar(&cc, 1)", taking true branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:473:3: 5. path: Falling through to end of if statement. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:474:3: 6. path: Condition "0 != cc.timedout[1]", taking true branch. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:474:3: 7. path: Falling through to end of if statement. lib/acl_threadsupport/test/acl_threadsupport_test.cpp:475:3: 8. unlock: "acl_release_condvar" unlocks "cc.waiter_mutex". lib/acl_threadsupport/src/acl_threadsupport.c:375:5: 8.1. unlock: "pthread_mutex_unlock" unlocks "C->waiter_mutex". lib/acl_threadsupport/test/acl_threadsupport_test.cpp:477:3: 9. double_unlock: "acl_reset_condvar" unlocks "cc.waiter_mutex" while it is unlocked. lib/acl_threadsupport/src/acl_threadsupport.c:300:5: 9.1. unlock: "pthread_mutex_unlock" unlocks "C->waiter_mutex". lib/acl_threadsupport/src/acl_threadsupport.c:301:5: 9.2. destroy: "pthread_mutex_destroy" destroys "C->waiter_mutex". ``` --- lib/acl_threadsupport/src/acl_threadsupport.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/acl_threadsupport/src/acl_threadsupport.c b/lib/acl_threadsupport/src/acl_threadsupport.c index 6db16698..65b800af 100644 --- a/lib/acl_threadsupport/src/acl_threadsupport.c +++ b/lib/acl_threadsupport/src/acl_threadsupport.c @@ -293,11 +293,6 @@ void acl_reset_condvar(struct acl_condvar_s *C) { #else { int ret = 0; - // Try to unlock then unlock. This is done just in case it is already - // locked, because if the mutex is locked we can't destroy it. This may - // happen in cases where an asssert causes an exit, etc. - pthread_mutex_trylock(&(C->waiter_mutex)); - pthread_mutex_unlock(&(C->waiter_mutex)); ret |= pthread_mutex_destroy(&(C->waiter_mutex)); ret |= sem_destroy(&(C->signal_sem)); ret |= sem_destroy(&(C->passive_sem[0]));