frang75 / nappgui_src

SDK for building cross-platform desktop apps in ANSI-C
https://www.nappgui.com
MIT License
503 stars 50 forks source link

nappgui may falsely report memory leaks at exit when linking with static libraries on windows #152

Closed colugomusic closed 2 months ago

colugomusic commented 2 months ago

In bmem_win.c, _bmem_atexit() is currently defined as:

void _bmem_atexit(void)
{
#if defined(__MEMORY_AUDITOR__)
#if _WITH_CRTDBG
    _CrtDumpMemoryLeaks();
#endif
#else
    cassert(i_HEAP == NULL);
/*HeapDestroy(i_HEAP);*/
#endif
}

I have found that this will falsely report memory leaks at exit if your application is linking with a library which still has static objects hanging around at the point where nappgui emits the call to _CrtDumpMemoryLeaks. In my case it is the Boost.process library. I think the order in which static objects across different linked libraries are deallocated is not well defined so nappgui should probably remove this call.

frang75 commented 2 months ago

Hi @colugomusic, thanks for using NAppGUI.

Indeed, with external libraries with static objects it is possible that _CrtDumpMemoryLeaks() detects memory leaks due to objects not yet released. However, this call is very useful during development, since native Win32 objects are not controlled by the NAppGUI memory auditor. At the moment, it is not going to be removed.

Control over the CRT (#include <crtdbg.h>) is only included in DEBUG builds. You can also disable it by commenting the line:

#define _WITH_CRTDBG 1

colugomusic commented 2 months ago

Hi @colugomusic, thanks for using NAppGUI.

Indeed, with external libraries with static objects it is possible that _CrtDumpMemoryLeaks() detects memory leaks due to objects not yet released. However, this call is very useful during development, since native Win32 objects are not controlled by the NAppGUI memory auditor. At the moment, it is not going to be removed.

Control over the CRT (#include <crtdbg.h>) is only included in DEBUG builds. You can also disable it by commenting the line:

#define _WITH_CRTDBG 1

Maybe a compromise could be to allow the user to opt out by passing an additional compiler flag, e.g. _NAPPGUI_NO_CRTDBG if they know they are linking against other libraries:

#if defined(_MSC_VER) && _MSC_VER > 1400 && !defined(_NAPPGUI_NO_CRTDBG)
#define _WITH_CRTDBG 1
#include <crtdbg.h>
#endif

That way they don't have to keep a separate fork of nappgui. Otherwise the output of _CrtDumpMemoryLeaks can add quite a lot of noise to the build output so it's a bit annoying to ignore.

frang75 commented 2 months ago

@colugomusic Fixed in this commit: https://github.com/frang75/nappgui_src/commit/b65fc3194b22b77bab00ba079e25de96a4d3accc Doc: https://nappgui.com/docs/r5452/en/guide/build.html#h3

colugomusic commented 2 months ago

@colugomusic Fixed in this commit: b65fc31 Doc: https://nappgui.com/docs/r5452/en/guide/build.html#h3

Perfect thank you!

frang75 commented 2 months ago

There is a typo in doc. The correct option is -DCMAKE_DISABLE_CRTDBG=YES