475876 - TM: add hooks so Valgrind works with the JIT without requiring --smc-check=all

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch to add Valgrind hooks

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).

Comment 1

[Bob Clary [:bc] (inactive)]

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?

Comment 2

[Jeff Walden [:Waldo]]

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

Comment 3

[Nicholas Nethercote [inactive]]

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.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 5

[Jesse Ruderman]

Comment on attachment 359454 [details] [diff] [review]
patch to add Valgrind hooks

Ted, can you review the configure.in changes?

Comment 6

[(not currently active) Ted Mielczarek]

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.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: adds header tests, changes --with-valgrind to --enable-valgrind

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.

Comment 8

[Jesse Ruderman]

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

Comment 9

[Nicholas Nethercote [inactive]]

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

Comment 10

[Bob Clary [:bc] (inactive)]

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.

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[(not currently active) Ted Mielczarek]

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

Comment 13

[Nicholas Nethercote [inactive]]

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

?

Comment 14

[Jesse Ruderman]

http://hg.mozilla.org/tracemonkey/rev/cec1362c02e3

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cec1362c02e3

Comment 16

[Bob Clary [:bc] (inactive)]

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

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a6f7a1b0fa3d