maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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.

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@xxxxxxxxxxx