TraceMonkey generates and modifies code. This confuses Valgrind, if run in the default manner, and usually causes Valgrind to quickly crash. Valgrind's --smc-check=all flag avoids the problem, but it makes Valgrind roughly 2x slower. This patch adds VALGRIND_DISCARD_TRANSLATION hooks to nanojit to tell Valgrind when code is being generated/modified. The hooks are enabled if --with-valgrind is used (in either the top-level ./configure or in js/src/configure). The conditional compilation style used to guard these hooks was modelled after the one in memory/jemalloc/jemalloc.c, the only other part of Mozilla that uses Valgrind hooks. I've included hooks in NativeARM.cpp even though Valgrind doesn't work on ARM. This is because Valgrind hopefully will work on ARM in the future. The patch also modifies the meaning of --with-valgrind in both those configure scripts -- previously --with-valgrind only took effect if --enable-jemalloc was also set. This was confusing (eg. see comments #17, #18, #19 in bug #424040).

Nicholas, this is great. the --smc-check=all makes valgrind runs incredibly slow. How close is this patch to being ready? Who would be a good reviewer?

Perhaps Graydon? I seem to recall he hacked on valgrindy stuff when doing the cycle collector.

I was thinking Andreas... knowing the JIT well is more important than knowing Valgrind well for this patch. It's basically a matter of checking that all places where code is generated or modified in memory gets a VALGRIND_DISCARD_TRANSLATION(addr, szB) hook.

This will prove useful for jsfunfuzz + valgrind in the future. :)

Comment on attachment 359454 [details] [diff] [review] patch to add Valgrind hooks Ted, can you review the configure.in changes?

Comment on attachment 359454 [details] [diff] [review] patch to add Valgrind hooks I'm not sure why this is --with-valgrind and not --enable-valgrind, since it's just a boolean flag, but that's not your fault. While you're here, though, could you change that AC_CHECK_HEADER line? Currently, if you specify --with-valgrind, but don't have the Valgrind headers, it will just be silently disabled. That sucks. Let's change it to something like: if test "x$enable_valgrind" = "xyes" ; then AC_CHECK_HEADER([valgrind/valgrind.h], [], AC_MSG_ERROR([Valgrind development headers not found!])) AC_DEFINE(MOZ_VALGRIND) fi This way it will error out on you if you're missing the headers.

I added the header check like you suggested. I also changed it to be --enable-valgrind instead of --with-valgrind. One problem with this is that --with-valgrind is silently ignored, so if people don't realise it's changed they could get mysterious behaviour. I can add a check for --with-valgrind and abort if it's given if people think that would be useful.

If it's silently ignoring unknown options, that sounds like a more general problem.

Jesse, as I understand it that's just the way --with-xxx options works in autoconf.

I would prefer a warning rather than a full abort for bisection where older builds would support one option and not the other. If they just warn, I could specify both and have it work for older as well as newer builds.

Would anyone notice a warning in amongst all the output that configure spews?

Comment on attachment 365041 [details] [diff] [review] adds header tests, changes --with-valgrind to --enable-valgrind Yeah, a warning is unlikely to do much good, and an error will make life harder for bc. Given that there are unlikely to be many users of this switch at this point, just change it and deal with the silent ignoring for now. + [Valgrind enabled but valgrind/valgrind.h not found or usable])) Something like "Please install the Valgrind development headers." at the end of this would be nice. (I assume that's the fix for the error?)

If Valgrind is (properly) installed the headers will be there. How about I change it so configure outputs this: checking for valgrind/valgrind.h... no configure: error: --enable-valgrind specified but Valgrind is not installed ?

1.9.1 is going to live for a while and it would be very nice to have this there.