Re: [Maria-developers] dbb62d20f36: Fixes that enables my_new.cc (new wrapper using my_malloc)
Hi, Michael! On Mar 28, Michael Widenius wrote:
revision-id: dbb62d20f36 (mariadb-10.5.2-518-gdbb62d20f36) parent(s): 0c37211c2a2 author: Michael Widenius <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> timestamp: 2021-03-24 14:31:53 +0200 message:
Fixes that enables my_new.cc (new wrapper using my_malloc)
This is not enabled by default as there are leaks in the server that needs to be fixed first. One can compile with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the memory leaks from 'new'
The change in my_new.cc is ok. The change in my_global.h and CMakeLists.txt is confusing and redundant. You unconditionaly define USE_MYSYS_NEW then conditionally undefine it again? Everything already worked just fine. By default cxx new is used, if one wants to use mysys version, it's done with -DHAVE_CXX_NEW=0 this works for me in 10.5 and fails to compile my_new.cc, as it should. Please, only keed your my_new.cc changes and say in the commit message "One can compile with -DHAVE_CXX_NEW=0 to find memory leaks from 'new'" As for -DSF_REMEMBER_FRAMES=14, perhaps it's better to do it automatically? Like #ifdef USE_MYSYS_NEW #define SF_REMEMBER_FRAMES 14 #else #define SF_REMEMBER_FRAMES 8 #endif or may be even just always make it 14. It should be easy to enable this feature, right?
diff --git a/CMakeLists.txt b/CMakeLists.txt index a0b540f12d2..c51fb98abdc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -366,10 +366,8 @@ ENDIF() # Run platform tests INCLUDE(configure.cmake)
-# force -DUSE_MYSYS_NEW unless already done by HAVE_CXX_NEW -IF(NOT HAVE_CXX_NEW) - ADD_DEFINITIONS(-DUSE_MYSYS_NEW) -ENDIF() +# Always use USE_MYSYS_NEW +ADD_DEFINITIONS(-DUSE_MYSYS_NEW)
# Find header files from the bundled libraries # (wolfssl, readline, pcre2, etc) diff --git a/include/my_global.h b/include/my_global.h index e999e555bf7..6e3b48a00fd 100644 --- a/include/my_global.h +++ b/include/my_global.h @@ -487,6 +487,9 @@ typedef unsigned short ushort; duplicate declaration of __cxa_pure_virtual, solved by declaring it a weak symbol. */ +#ifndef REALLY_USE_MYSYS_NEW +#undef USE_MYSYS_NEW +#endif #if defined(USE_MYSYS_NEW) && ! defined(DONT_DECLARE_CXA_PURE_VIRTUAL) C_MODE_START int __cxa_pure_virtual () __attribute__ ((weak)); diff --git a/mysys/my_static.c b/mysys/my_static.c index 3c4b9efc1f8..96a23cdb283 100644 --- a/mysys/my_static.c +++ b/mysys/my_static.c @@ -48,6 +48,7 @@ PSI_memory_key key_memory_my_err_head; PSI_memory_key key_memory_my_file_info; PSI_memory_key key_memory_pack_frm; PSI_memory_key key_memory_charsets; +PSI_memory_key key_memory_new;
where do you initialize it? I'd suggest statically, like PSI_memory_key key_memory_new= PSI_INSTRUMENT_MEM;
#ifdef _WIN32 PSI_memory_key key_memory_win_SECURITY_ATTRIBUTES;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Sun, Mar 28, 2021 at 3:00 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Michael!
On Mar 28, Michael Widenius wrote:
revision-id: dbb62d20f36 (mariadb-10.5.2-518-gdbb62d20f36) parent(s): 0c37211c2a2 author: Michael Widenius <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> timestamp: 2021-03-24 14:31:53 +0200 message:
Fixes that enables my_new.cc (new wrapper using my_malloc)
This is not enabled by default as there are leaks in the server that needs to be fixed first. One can compile with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the memory leaks from 'new'
The change in my_new.cc is ok. The change in my_global.h and CMakeLists.txt is confusing and redundant.
You unconditionaly define USE_MYSYS_NEW then conditionally undefine it again? Everything already worked just fine. By default cxx new is used, if one wants to use mysys version, it's done with
-DHAVE_CXX_NEW=0
this works for me in 10.5 and fails to compile my_new.cc, as it should.
The problem is that one cannot and should never use HAVE_CXX_NEW as this is something that is found by cmake. In the future USE_MYSYS_NEW should be on by default and one would have to use -DUSE_MYSYS_NEW=0 if one does not want to use it. We cannot do that yet, which is why I added REALLY_USE_MYSYS_NEW temporarily to allow one to test, debug and fix MYSYS_NEW. One cannot fix this by reseting HAVE_CXX_NEW, because my_new.cc requires this to be defined if the platform supports it! Not doing that gave me compiler errors.
Please, only keed your my_new.cc changes and say in the commit message "One can compile with -DHAVE_CXX_NEW=0 to find memory leaks from 'new'"
Does not work, see above.
As for -DSF_REMEMBER_FRAMES=14, perhaps it's better to do it automatically? Like
#ifdef USE_MYSYS_NEW #define SF_REMEMBER_FRAMES 14 #else #define SF_REMEMBER_FRAMES 8 #endif
or may be even just always make it 14. It should be easy to enable this feature, right?
yes, this is a good idea, will do.
--- a/mysys/my_static.c +++ b/mysys/my_static.c @@ -48,6 +48,7 @@ PSI_memory_key key_memory_my_err_head; PSI_memory_key key_memory_my_file_info; PSI_memory_key key_memory_pack_frm; PSI_memory_key key_memory_charsets; +PSI_memory_key key_memory_new;
where do you initialize it? I'd suggest statically, like
PSI_memory_key key_memory_new= PSI_INSTRUMENT_MEM;
#ifdef _WIN32 PSI_memory_key key_memory_win_SECURITY_ATTRIBUTES;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Michael! On Mar 28, Michael Widenius wrote:
On Sun, Mar 28, 2021 at 3:00 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Mar 28, Michael Widenius wrote:
revision-id: dbb62d20f36 (mariadb-10.5.2-518-gdbb62d20f36) parent(s): 0c37211c2a2 author: Michael Widenius <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> timestamp: 2021-03-24 14:31:53 +0200 message:
Fixes that enables my_new.cc (new wrapper using my_malloc)
This is not enabled by default as there are leaks in the server that needs to be fixed first. One can compile with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the memory leaks from 'new'
The change in my_new.cc is ok. The change in my_global.h and CMakeLists.txt is confusing and redundant.
You unconditionaly define USE_MYSYS_NEW then conditionally undefine it again? Everything already worked just fine. By default cxx new is used, if one wants to use mysys version, it's done with
-DHAVE_CXX_NEW=0
this works for me in 10.5 and fails to compile my_new.cc, as it should.
The problem is that one cannot and should never use HAVE_CXX_NEW as this is something that is found by cmake.
That's okay, it's always possible to override cmake check, cmake won't mind. We routinely do it all the time. You might've missed that I wrote above that I tested it and it worked.
One cannot fix this by reseting HAVE_CXX_NEW, because my_new.cc requires this to be defined
No, it doesn't. There's no code that checks for HAVE_CXX_NEW, HAVE_CXX_NEW is only for cmake and my_new.cc only checks USE_MYSYS_NEW. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Monty! On Mar 28, Sergei Golubchik wrote:
On Mar 28, Michael Widenius wrote:
On Sun, Mar 28, 2021 at 3:00 PM Sergei Golubchik <serg@mariadb.org> wrote:
On Mar 28, Michael Widenius wrote:
revision-id: dbb62d20f36 (mariadb-10.5.2-518-gdbb62d20f36) parent(s): 0c37211c2a2 author: Michael Widenius <michael.widenius@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> timestamp: 2021-03-24 14:31:53 +0200 message:
Fixes that enables my_new.cc (new wrapper using my_malloc)
This is not enabled by default as there are leaks in the server that needs to be fixed first. One can compile with -DREALLY_USE_MYSYS_NEW -DSF_REMEMBER_FRAMES=14 to find the memory leaks from 'new'
Here's a simplified patch that works (I've applied it, compiled, run the test suite, and verified in a debugger that new() from my_new.cc is used): ================== diff --git a/mysys/my_new.cc b/mysys/my_new.cc index 88080f3e7eb..89869f6e0fa 100644 --- a/mysys/my_new.cc +++ b/mysys/my_new.cc @@ -25,34 +25,52 @@ #include "mysys_priv.h" #include <new> +/* + We don't yet enable the my new operators by default. + The reasons (for MariaDB) are: + + - There are several global objects in plugins (wsrep_info, InnoDB, + tpool) that allocates data with 'new'. These objects are not + freed properly before exit() is called and safemalloc will report + these as lost memory. The proper fix is to ensure that all plugins + either ensure that all objects frees there data or the global object are + changed to a pointer that as allocated and freed on demand. + Doing this will make it easier to find leaks and also speed up plugin + loads when we don't have to initialize a lot of objects until they + are really needed. + - Rocksdb calls malloc_usable_size, that will crash if used with new based + on my_malloc. One suggested fix would be to not define + ROCKSDB_MALLOC_USABLE_SIZE if MYSYS_USE_NEW is defined. +*/ + #ifdef USE_MYSYS_NEW void *operator new (size_t sz) { - return (void *) my_malloc (sz ? sz : 1, MYF(0)); + return (void *) my_malloc(key_memory_new, sz ? sz : 1, MYF(0)); } void *operator new[] (size_t sz) { - return (void *) my_malloc (sz ? sz : 1, MYF(0)); + return (void *) my_malloc(key_memory_new, sz ? sz : 1, MYF(0)); } void* operator new(std::size_t sz, const std::nothrow_t&) throw() { - return (void *) my_malloc (sz ? sz : 1, MYF(0)); + return (void *) my_malloc(key_memory_new, sz ? sz : 1, MYF(0)); } void* operator new[](std::size_t sz, const std::nothrow_t&) throw() { - return (void *) my_malloc (sz ? sz : 1, MYF(0)); + return (void *) my_malloc(key_memory_new, sz ? sz : 1, MYF(0)); } -void operator delete (void *ptr, std::size_t) +void operator delete (void *ptr, std::size_t) throw () { my_free(ptr); } -void operator delete (void *ptr) +void operator delete (void *ptr) throw () { my_free(ptr); } diff --git a/mysys/my_static.c b/mysys/my_static.c index 6c09ab8d94b..1da04599793 100644 --- a/mysys/my_static.c +++ b/mysys/my_static.c @@ -48,6 +48,7 @@ PSI_memory_key key_memory_my_err_head; PSI_memory_key key_memory_my_file_info; PSI_memory_key key_memory_pack_frm; PSI_memory_key key_memory_charsets; +PSI_memory_key key_memory_new; #ifdef _WIN32 PSI_memory_key key_memory_win_SECURITY_ATTRIBUTES; diff --git a/mysys/mysys_priv.h b/mysys/mysys_priv.h index adf2d39046a..c5e05ec7955 100644 --- a/mysys/mysys_priv.h +++ b/mysys/mysys_priv.h @@ -88,6 +88,7 @@ extern PSI_memory_key key_memory_my_err_head; extern PSI_memory_key key_memory_my_file_info; extern PSI_memory_key key_memory_pack_frm; extern PSI_memory_key key_memory_charsets; +extern PSI_memory_key key_memory_new; #ifdef _WIN32 extern PSI_memory_key key_memory_win_SECURITY_ATTRIBUTES; ================== Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Sun, Mar 28, 2021 at 11:18 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Monty!
<cut>
Here's a simplified patch that works (I've applied it, compiled, run the test suite, and verified in a debugger that new() from my_new.cc is used):
One which systems did you test it? Also on older systems that does not provide <new> ? Please always provide a diff on top of my diff, so that I can see what changes you propose, or comment inline what you want to change. I did not find the diff you provide usable (without too much work on my side comparing diffs. Anyway, I do feel my version is safer as it takes into account if HAVE_CXX_NEW exists or not, so I will stick with it. Regards, Monty
participants (2)
-
Michael Widenius -
Sergei Golubchik