Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #2063 #2064

Merged

Conversation

embray
Copy link
Contributor

@embray embray commented Mar 19, 2019

The DllMain used in Cygwin did not run the thread memory pool cleanup upon DLL_THREAD_DETACH which is needed when compiled with USE_TLS=1.

There is also a DllMain implemented in memory.c for some reason, which is not used on Cygwin (just plain Windows), and also has some special bits for when OpenBLAS is used as a static lib. If anyone who understands this better thinks it would be good to use this on Cygwin instead, and has a good idea how it would be nice to get some input on this.

But otherwise I don't see a cleaner way to do this. It seems to work perfectly though to fix the memory leak reported in #2063.

…thread memory

pool cleanup upon THREAD_DETACH which is needed when compiled with
USE_TLS=1.
@@ -1313,6 +1313,13 @@ void blas_memory_free_nolock(void * map_address) {
free(map_address);
}

#ifdef SMP
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll note that this ifdef is completely superfluous, as this entire section is, I believe, technically under an #ifdef SMP. Nevertheless, it us used in several other places in the surrounding code, so I'm just using it here for consistency's sake. Later it will be good to clean this up when refactoring this file.

@embray embray force-pushed the cygwin/use-tls-thread-memory-cleanup branch from ddedc88 to 8ba9e2a Compare March 19, 2019 10:22
@martin-frbg martin-frbg merged commit 8502030 into OpenMathLib:develop Mar 19, 2019
@embray embray deleted the cygwin/use-tls-thread-memory-cleanup branch March 29, 2019 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants