List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:May 19 2010 6:55pm
Subject:Re: bzr commit into mysql-5.1-innodb branch (marko.makela:3474)
Bug#53593
View as plain text  
Hi Marko,

Some minor requests and suggestions below. Otherwise, patch approved.

On 5/19/10 11:32 AM, marko.makela@stripped wrote:
> #At file:///home/marko/innobase/dev/mysql/5.1-innodb/ based on
> revid:marko.makela@stripped
>
>   3474 Marko Mäkelä	2010-05-19
>        Implement Valgrind memcheck instrumentation in the MySQL core. (Bug #53593)

We usually use the syntax:

Bug#nnnnn: Title

in the first line. There are tools that use this information to control 
the bug state -- like, move it to Documenting once it is merged to one 
of the main trees.

>        BUILD/*: Add valgrind_configs=--with-valgrind.
>        BUILD/*: Remove -USAFEMALLOC from valgrind_flags.
>

[..]

> === 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,27 @@ typedef struct my_aio_result {
>   } my_aio_result;
>   #endif
>
> +#ifdef HAVE_VALGRIND
> +# include<valgrind/memcheck.h>
> +# define MEM_UNDEFINED(a,len) VALGRIND_MAKE_MEM_UNDEFINED(a,len)
> +# define MEM_NOACCESS(a,len) VALGRIND_MAKE_MEM_NOACCESS(a,len)
> +# define MEM_DEFINED_ADDRESSABLE(a,len) VALGRIND_MAKE_MEM_DEFINED(a,len)
> +# define MEM_DEFINED(a,len) VALGRIND_MAKE_MEM_DEFINED_IF_ADDRESSABLE(a,len)
> +# define MEM_GET_VBITS(a,v,len) VALGRIND_GET_VBITS(a,v,len)
> +# define MEM_SET_VBITS(a,v,len) VALGRIND_SET_VBITS(a,v,len)
> +# define MEM_CHECK_ADDRESSABLE(a,len) VALGRIND_CHECK_MEM_IS_ADDRESSABLE(a,len)
> +# define MEM_CHECK_DEFINED(a,len) VALGRIND_CHECK_MEM_IS_DEFINED(a,len)
> +#else /* HAVE_VALGRIND */
> +# define MEM_UNDEFINED(a,len) ((void) 0)
> +# define MEM_NOACCESS(a,len) ((void) 0)
> +# define MEM_DEFINED_ADDRESSABLE(a,len) ((void) 0)
> +# define MEM_DEFINED(a,len) ((void) 0)
> +# define MEM_GET_VBITS(a,v,len) ((void) 0)
> +# define MEM_SET_VBITS(a,v,len) ((void) 0)
> +# define MEM_CHECK_ADDRESSABLE(a,len) ((void) 0)
> +# define MEM_CHECK_DEFINED(a,len) ((void) 0)
> +#endif /* HAVE_VALGRIND */

Do you plan to use all of those soon? If no, please only keep those that 
are likely to be used in the near future.

>   #ifndef THREAD
>   extern int NEAR my_errno;		/* Last error in mysys */
>   #else
> @@ -141,7 +162,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)
>   #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 +190,10 @@ 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) do {				\
> +	MEM_CHECK_ADDRESSABLE(A, B);		\
> +	MEM_UNDEFINED(A, B);			\
> +} while (0)

No tabs in MySQL code.

http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines

Otherwise looks good. Thanks for working on this!

Regards,

--
Davi Arnaut
Thread
bzr commit into mysql-5.1-innodb branch (marko.makela:3474) Bug#53593marko.makela19 May
  • Re: bzr commit into mysql-5.1-innodb branch (marko.makela:3474)Bug#53593Davi Arnaut19 May
    • Re: bzr commit into mysql-5.1-innodb branch (marko.makela:3474)Bug#53593Marko Mäkelä20 May
    • Re: bzr commit into mysql-5.1-innodb branch (marko.makela:3474)Bug#53593Marko Mäkelä20 May