Hi! On Sun, Mar 28, 2021 at 3:00 PM Sergei Golubchik <s...@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.widen...@gmail.com> > > committer: Michael Widenius <michael.widen...@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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
