Closed Bug 475876 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: njn, Assigned: njn)

Details

(Keywords: fixed1.9.1, valgrind, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch patch to add Valgrind hooks (obsolete) — Splinter Review
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.
Attachment #359454 - Flags: review?(gal)
Attachment #359454 - Flags: review?(gal) → review+
Whiteboard: checkin-needed
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?
Attachment #359454 - Flags: review?(ted.mielczarek)
Attachment #359454 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #365041 - Flags: review?(ted.mielczarek)
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?
Attachment #365041 - Flags: review?(ted.mielczarek) → review+
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

?
http://hg.mozilla.org/tracemonkey/rev/cec1362c02e3
Whiteboard: checkin-needed → fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/cec1362c02e3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
1.9.1 is going to live for a while and it would be very nice to have this there.
Flags: wanted1.9.1?
Attachment #365041 - Flags: approval1.9.1?
Attachment #365041 - Flags: approval1.9.1?
Attachment #359454 - Attachment is obsolete: true
Flags: wanted1.9.1?
Target Milestone: --- → mozilla1.9.2a1
No longer blocks: 545133
You need to log in before you can comment on or make changes to this bug.