Replaced incorrect double checked locking with a static#21612
Replaced incorrect double checked locking with a static#21612opencv-pushbot merged 1 commit intoopencv:4.xfrom
Conversation
modules/core/src/matrix.cpp
Outdated
| { | ||
| cv::AutoLock lock(cv::getInitializationMutex()); | ||
| std::call_once(g_singletonOnceFlag, []() { | ||
| if (g_matAllocator == NULL) |
There was a problem hiding this comment.
There are other cases with double-checked locking of pre-C++11 approach which are not touched by this patch.
This is strategically wrong to keep both approaches.
What is the error message from TSAN?
Which version of TSAN is used?
There was a problem hiding this comment.
What is the error message from TSAN? Which version of TSAN is used?
It is the TSan included in Xcode 13.2.1. The error message is:
WARNING: ThreadSanitizer: data race (pid=34443)
Write of size 8 at 0x00011359bc28 by thread T27 (mutexes: write M1824):
#0 cv::Mat::getDefaultAllocator() matrix.cpp:191 (Brainsight:x86_64+0x108eadf9d)
#1 cv::Mat::create(int, int const*, int) matrix.cpp:696 (Brainsight:x86_64+0x108eb0d7a)
#2 cv::Mat::create(int, int, int) matrix.cpp:537 (Brainsight:x86_64+0x108eb0016)
#3 cv::_OutputArray::create(int, int, int, int, bool, cv::_OutputArray::DepthMask) const matrix_wrap.cpp:1221 (Brainsight:x86_64+0x108f649d1)
#4 cv::Mat::copyTo(cv::_OutputArray const&) const copy.cpp:349 (Brainsight:x86_64+0x108d3ea9b)
#5 cv::Mat::clone() const matrix.cpp:519 (Brainsight:x86_64+0x108eb363d)
#6 SomeCodeOfMine()
Previous read of size 8 at 0x00011359bc28 by thread T24:
#0 cv::Mat::getDefaultAllocator() matrix.cpp:186 (Brainsight:x86_64+0x108eadf1a)
#1 cv::Mat::create(int, int const*, int) matrix.cpp:696 (Brainsight:x86_64+0x108eb0d7a)
#2 cv::Mat::create(int, int, int) matrix.cpp:537 (Brainsight:x86_64+0x108eb0016)
#3 cv::_OutputArray::create(int, int, int, int, bool, cv::_OutputArray::DepthMask) const matrix_wrap.cpp:1221 (Brainsight:x86_64+0x108f649d1)
#4 cv::Mat::copyTo(cv::_OutputArray const&) const copy.cpp:349 (Brainsight:x86_64+0x108d3ea9b)
#5 cv::Mat::clone() const matrix.cpp:519 (Brainsight:x86_64+0x108eb363d)
#6 SomeCodeOfMine()
Location is global 'cv::(anonymous namespace)::g_matAllocator' at 0x00011359bc28 (Brainsight+0x00010bf56c28)
Mutex M1824 (0x7b100003d7c0) created at:
#0 pthread_mutex_init <null>:2 (libclang_rt.tsan_osx_dynamic.dylib:x86_64+0x2db73)
#1 std::__1::recursive_mutex::recursive_mutex() <null>:2 (libc++.1.dylib:x86_64+0x3947b)
#2 cv::utils::logging::internal::GlobalLoggingInitStruct::GlobalLoggingInitStruct() logger.cpp:48 (Brainsight:x86_64+0x108e0ee2d)
#3 cv::utils::logging::internal::GlobalLoggingInitStruct::GlobalLoggingInitStruct() logger.cpp:47 (Brainsight:x86_64+0x108e0edb5)
#4 cv::utils::logging::internal::getGlobalLoggingInitStruct() logger.cpp:97 (Brainsight:x86_64+0x108e0d6ea)
#5 cv::utils::logging::internal::GlobalLoggingInitCall::GlobalLoggingInitCall() logger.cpp:109 (Brainsight:x86_64+0x108e0ed61)
#6 cv::utils::logging::internal::GlobalLoggingInitCall::GlobalLoggingInitCall() logger.cpp:108 (Brainsight:x86_64+0x108e0d4f5)
#7 __cxx_global_var_init logger.cpp:114 (Brainsight:x86_64+0x108e127fc)
#8 _GLOBAL__sub_I_logger.cpp logger.cpp (Brainsight:x86_64+0x108e12835)
#9 ImageLoaderMachO::doModInitFunctions(ImageLoader::LinkContext const&) <null>:2 (dyld:x86_64+0x1db46)
SUMMARY: ThreadSanitizer: data race matrix.cpp:191 in cv::Mat::getDefaultAllocator()
It seems pretty clear that one thread was reading g_matAllocator (outside the lock) while another was writing to g_matAllocator (inside the lock).
There are other cases with double locking of pre-C++11 approach which is not touched by this patch.
If you say so. :) OpenCV is very large. This is the only one I have encountered. I can try to find & fix others if this call_once solution is acceptable.
There was a problem hiding this comment.
There was a problem hiding this comment.
As per both footnote 2 and footnote 4 in that wikipedia article, the OpenCV code is currently wrong, which of course is also what TSan is telling us.
All the C++ solutions I am aware of need the <mutex> header.
There was a problem hiding this comment.
Could you try this code?
static
MatAllocator*& getDefaultAllocatorMatRef()
{
static MatAllocator* g_matAllocator = Mat::getStdAllocator();
return g_matAllocator;
}
MatAllocator* Mat::getDefaultAllocator()
{
return getDefaultAllocatorMatRef();
}
void Mat::setDefaultAllocator(MatAllocator* allocator)
{
getDefaultAllocatorMatRef() = allocator;
}
BTW, existed API of getDefaultAllocator/setDefaultAllocator is not thread safe
There was a problem hiding this comment.
Could you try this code?
It satisfies TSan.
Thread Sanitizer identified an incorrect implementation of double checked locking. Replaced it with a static, which therefore can only be created once.
b00f8e7 to
5b23752
Compare
Thread Sanitizer identified an incorrect implementation of double checked locking. Use std::call_once to ensure the singleton object is created exactly once, and in a thread-safe manner.