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

[Marko Mäkelä]

Hi Davi,

thank you for your prompt response.

On Wed, May 12, 2010 at 12:17:02PM -0300, Davi Arnaut wrote:
>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.

Like I wrote later in the bug report, I would not remove 
-UFORCE_INIT_OF_VARS after all. If we removed it, LINT_INIT would have 
to be edited to declare the variable uninitialized in Valgrind.

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

Good catch.

>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,

Thanks, I will post a revised patch next week. (Tomorrow is a national 
holiday, and I am not working on Fridays.)

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

Definitely. I would leave that to someone who is more familiar with the 
MySQL internals. InnoDB code does not make use of the MySQL MEM_ROOT. I 
guess that the MEM_ROOT is similar to the mem_heap in InnoDB. I see that 
the patch there goes a step further and uses VALGRIND_MAKE_MEM_NOACCESS 
instead of VALGRIND_MAKE_MEM_UNDEFINED in the my_free() code. I did not 
go that far, because I am not familiar with the code. In certain places 
of InnoDB, I do use VALGRIND_MAKE_MEM_NOACCESS. I have also placed some 
VALGRIND_CHECK_MEM_IS_DEFINED and VALGRIND_CHECK_MEM_IS_ADDRESSABLE 
checks to the InnoDB code. (See the UNIV_MEM_ macros in 
innobase/include/univ.i.)

This patch is just a humble beginning; I hope someone who is familiar 
with the MySQL allocators can finish the job and open the floodgates for 
a host of Valgrind bugs. :-)

One last thing: Which tree should this be pushed to? So far, our 
recently created mysql-5.1-innodb and mysql-trunk-innodb trees have been 
"unidirectional", that is, only changes from InnoDB have been merged to 
the "main" tree. Can I push this to the -innodb trees without fearing 
conflicts at merge time? I hope the trees have not diverged too much 
during the few weeks of *-innodb existence.

Best regards,

	Marko