Re: bzr commit into mysql-5.1-innodb branch (marko.makela:3464) Bug#53593

[Davi Arnaut]

Hi Marko,

On 5/12/10 10:04 AM, marko.makela@stripped wrote:
> #At file:///home/marko/innobase/dev/mysql/5.1-innodb/ based on
> revid:marko.makela@stripped
>
>   3464 Marko Mäkelä	2010-05-12
>        Implement Valgrind memcheck instrumentation in the MySQL core. (Bug #53593)
>
>        Note: I would appreciate feedback regarding my changes to the BUILD scripts.

OK.

>      modified:
>        BUILD/SETUP.sh
>        BUILD/build_mccge.sh
>        BUILD/compile-amd64-valgrind-max
>        BUILD/compile-pentium-icc-valgrind-max
>        BUILD/compile-pentium-valgrind-max
>        BUILD/compile-pentium-valgrind-max-no-ndb
>        BUILD/compile-pentium64-valgrind-max
>        configure.in
>        include/my_sys.h
>        storage/innobase/include/univ.i
>        storage/innodb_plugin/include/univ.i
> === modified file 'BUILD/SETUP.sh'
> --- a/BUILD/SETUP.sh	revid:marko.makela@stripped
> +++ b/BUILD/SETUP.sh	revid:marko.makela@stripped
> @@ -119,8 +119,9 @@ fi
>
>   # Set flags for various build configurations.
>   # Used in -valgrind builds
> -valgrind_flags="-USAFEMALLOC -UFORCE_INIT_OF_VARS -DHAVE_purify "
> +valgrind_flags="-DHAVE_purify "

Hum, removal of FORCE_INIT_OF_VARS should be fine -- in case a valgrind 
false positive is encountered, the suppression file should be used, and 
we also use the self-initialization trick to handle false positives from 
GCC.

SAFEMALLOC is contained within the feature set of valgrind. OK too.


>   valgrind_flags="$valgrind_flags -DMYSQL_SERVER_SUFFIX=-valgrind-max"
> +valgrind_configs="--with-valgrind"
>   #
>   # Used in -debug builds
>   debug_cflags="-DUNIV_MUST_NOT_INLINE -DEXTRA_DEBUG -DFORCE_INIT_OF_VARS "
>
> === modified file 'BUILD/build_mccge.sh'
> ---
> a/BUILD/build_mccge.sh	revid:marko.makela@stripped
> +++
> b/BUILD/build_mccge.sh	revid:marko.makela@stripped
> @@ -941,6 +941,7 @@ set_valgrind_flags()
>       loc_valgrind_flags="-USAFEMALLOC -UFORCE_INIT_OF_VARS -DHAVE_purify "

^ shouldn't the above line be updated too?

>       loc_valgrind_flags="$loc_valgrind_flags -DMYSQL_SERVER_SUFFIX=-valgrind-max"
>       compiler_flags="$compiler_flags $loc_valgrind_flags"
> +    with_flags="$with_flags --with-valgrind"
>     fi
>   }
>
>

[..]

>
> === modified file 'configure.in'
> --- a/configure.in	revid:marko.makela@stripped
> +++ b/configure.in	revid:marko.makela@stripped
> @@ -1729,6 +1729,18 @@ else
>     CXXFLAGS="$OPTIMIZE_CXXFLAGS $CXXFLAGS"
>   fi
>
> +AC_ARG_WITH(valgrind,
> +    [  --with-valgrind         Enable Valgrind instrumentation.],
> +    [ with_valgrind=$withval ],
> +    [ with_valgrind=no ]
> +    )

Suggestion:

AC_ARG_WITH([valgrind],
	[AS_HELP_STRING([--with-valgrind],
		[Valgrind instrumentation @<:@default=no@:>@])],
	[], [with_valgrind=no])


> +if test "$with_valgrind" != "no"
> +then
> +  AC_CHECK_HEADER(valgrind/memcheck.h,
> +    [AC_DEFINE([USE_VALGRIND_MEMCHECK], [1],[Add Valgrind instrumentation])])
> +fi
> +

I suggest a more generic check that can be taken advantage of later:

AC_CHECK_HEADERS([valgrind/valgrind.h valgrind/memcheck.h],
		[AC_DEFINE([HAVE_VALGRIND], [1], [Define for Valgrind support]])

>   # Debug Sync Facility. NOTE: depends on 'with_debug'. Must be behind it.
>   AC_MSG_CHECKING(if Debug Sync Facility should be enabled.)
>   AC_ARG_ENABLE(debug_sync,
>
> === modified file 'include/my_sys.h'
> --- a/include/my_sys.h	revid:marko.makela@stripped
> +++ b/include/my_sys.h	revid:marko.makela@stripped
> @@ -25,6 +25,13 @@ typedef struct my_aio_result {
>   } my_aio_result;
>   #endif
>
> +#ifdef USE_VALGRIND_MEMCHECK
> +# include<valgrind/memcheck.h>
> +# define MEM_UNDEFINED(addr,size) VALGRIND_MAKE_MEM_UNDEFINED(addr,size)
> +#else
> +# define MEM_UNDEFINED(addr,size) ((void) 0)
> +#endif
> +
>   #ifndef THREAD
>   extern int NEAR my_errno;		/* Last error in mysys */
>   #else
> @@ -141,7 +148,7 @@ extern int NEAR my_errno;		/* Last error
>   #define my_memdup(A,B,C) _my_memdup((A),(B), __FILE__,__LINE__,C)
>   #define my_strdup(A,C) _my_strdup((A), __FILE__,__LINE__,C)
>   #define my_strndup(A,B,C) _my_strndup((A),(B),__FILE__,__LINE__,C)
> -#define TRASH(A,B) bfill(A, B, 0x8F)
> +#define TRASH(A,B) do { bfill(A, B, 0x8F); MEM_UNDEFINED(A, B); } while(0)

Hum, TRASH is used by the memory pool code. OK.

In the future, it would be nice if we could use the memory pool valgrind 
helpers: http://tinyurl.com/3a9dygs

>   #define QUICK_SAFEMALLOC sf_malloc_quick=1
>   #define NORMAL_SAFEMALLOC sf_malloc_quick=0
>   extern uint sf_malloc_prehunc,sf_malloc_endhunc,sf_malloc_quick;
> @@ -169,7 +176,7 @@ extern char *my_strndup(const char *from
>   #define CALLER_INFO_PROTO   /* nothing */
>   #define CALLER_INFO         /* nothing */
>   #define ORIG_CALLER_INFO    /* nothing */
> -#define TRASH(A,B) /* nothing */
> +#define TRASH(A,B) MEM_UNDEFINED(A, B)
>   #endif
>
>   #if defined(ENABLED_DEBUG_SYNC)
>
> === modified file 'storage/innobase/include/univ.i'
> ---
> a/storage/innobase/include/univ.i	revid:marko.makela@stripped
> +++
> b/storage/innobase/include/univ.i	revid:marko.makela@stripped
> @@ -82,9 +82,9 @@ memory is read outside the allocated blo
>
>   /* Make a non-inline debug version */
>
> -#ifdef HAVE_purify
> +#if defined HAVE_purify || defined USE_VALGRIND_MEMCHECK
>   # define UNIV_DEBUG_VALGRIND
> -#endif /* HAVE_purify */
> +#endif /* HAVE_purify || USE_VALGRIND_MEMCHECK */
>   #if 0
>   #define UNIV_DEBUG_VALGRIND			/* Enable extra
>   						Valgrind instrumentation */
>
> === modified file 'storage/innodb_plugin/include/univ.i'
> ---
> a/storage/innodb_plugin/include/univ.i	revid:marko.makela@stripped
> +++
> b/storage/innodb_plugin/include/univ.i	revid:marko.makela@stripped
> @@ -165,9 +165,9 @@ command. Not tested on Windows. */
>   #define UNIV_COMPILE_TEST_FUNCS
>   */
>
> -#ifdef HAVE_purify
> +#if defined HAVE_purify || defined USE_VALGRIND_MEMCHECK
>   # define UNIV_DEBUG_VALGRIND
> -#endif /* HAVE_purify */
> +#endif /* HAVE_purify || USE_VALGRIND_MEMCHECK */
>   #if 0
>   #define UNIV_DEBUG_VALGRIND			/* Enable extra
>   						Valgrind instrumentation */

Otherwise looks good. Thanks!