search

shogun-toolbox / shogun

Shōgun
http://shogun-toolbox.org
BSD 3-Clause "New" or "Revised" License
3.03k stars 1.04k forks source link

OpenMP probably does not work with C++11 features (including std::shared_ptr) #5121

Closed ghost closed 3 years ago

ghost commented 4 years ago

The example cross_validation_support_vector_machine fails with high probability (double free / segfault). Running valgrind showed that the source of the problem is the function evaluate_one_run in CrossValidation. I reported this bug before, suspecting that the problem is SG_REF and SG_UNREF. It turns out that openmp does not officially support C++11 features including std::atomic and std::shared_ptr. Some implementations support them unofficially, but it seems that mine doesn't. Removing the line for using openmp fixes the problem.

Resources: https://stackoverflow.com/questions/21554099/can-stdatomic-be-safely-used-with-openmp https://stackoverflow.com/questions/53198251/openmp-and-shared-ptr

vigsterkr commented 4 years ago

thnx a lot for looking into this :( it's quite a sad news.... what's the compiler you've testing?

ghost commented 4 years ago

I'm using gcc version 9.3.0

ghost commented 4 years ago

Running valgrind on many of the examples reports "possibly lost" memory leaks. One of sources is OpenMP. For example DotFeatures::dot. I really don't get how this is the case, since openmp must use threading primitives, and shared_ptr control block is thread safe.

gf712 commented 4 years ago

OpenMP also has a lot of false positives in valgrind. That is because memory management is handled by the openmp runtime, which probably has some static initialisation of thread pools and valgrind cannot track all mem allocs/free. I think it is safe to assume that most the warnings related to openmp can be ignored. I think some projects add these OpenMP related warnings to the valgrid suppression file.

vigsterkr commented 4 years ago

we do have our suppression file: https://github.com/shogun-toolbox/shogun/blob/develop/configs/valgrind.supp

hasn't been updated in a while though

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 3 years ago

This issue is now being closed due to a lack of activity. Feel free to reopen it.