maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Sun, Mar 28, 2021 at 3:00 PM Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxx>
> > committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> > 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@xxxxxxxxxxx