512587 - js/src/configure should allow specification of valgrind header directory

Status: NEW

(Core :: JavaScript Engine, enhancement, P5)

Description

[Jim Blandy :jimb]

Passing js/src/configure the --enable-valgrind flag causes the JIT to execute certain magic instruction sequences that normally have no effect but when running under Valgrind notify Valgrind that the JIT has generated new machine code, and Valgrind should invalidate (some of) its caches.

For valgrind developers, it's useful to be able to easily point the SpiderMonkey build system at the headers in their source trees.  So, js/src/configure should take a --with-valgrind[=INCLUDEDIR] that has the same effect that --enable-valgrind does now, but, if INCLUDEDIR is given, also adds that directory to our header file search path before the system headers.

Comment 1

[Jim Blandy :jimb]

Attachment: Support specifying Valgrind header path.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 396604 [details] [diff] [review]
Support specifying Valgrind header path.

r+ if you fix the top-level configure in the same way:
http://mxr.mozilla.org/mozilla-central/source/configure.in#6679

Apparently we have valgrind hooks in jemalloc as well.

Comment 3

[Jim Blandy :jimb]

We need the same thing for NSPR, as well.  Let me revise the patch.

Comment 4

[(not currently active) Ted Mielczarek]

That needs to be a separate patch. (but I can still review it)

Comment 5

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Comment 6

[Julian Seward [:jseward]]

Alternative proposal is to embed a copy valgrind.h, memcheck.h and helgrind.h
permanently in the source tree.  Then no need for external versions thereof,
and we could feel freer about adding annotations, particularly w.r.t. 
marking up the tree to support run-time race checking.

Comment 7

[Jim Blandy :jimb]

(In reply to Julian Seward from comment #6)
> Alternative proposal is to embed a copy valgrind.h, memcheck.h and helgrind.h
> permanently in the source tree.  Then no need for external versions thereof,
> and we could feel freer about adding annotations, particularly w.r.t. 
> marking up the tree to support run-time race checking.

Julian, does what you propose seem like a better approach than the one implemented by the patch? The intent of this bug was to help valgrind developers, so we should take whatever approach you prefer.

Comment 8

[Julian Seward [:jseward]]

In hindsight, no.  Embedded copies of the headers might be a bit
overkill in terms of inflexibility.

My impression of the current situation is that --enable-valgrind uses
whatever headers are in /usr/include/valgrind, and that works fairly
well for most people.  Speaking only for myself, if I want to use a
development version of Valgrind in this situation, I simply symlink
/usr/include/valgrind to the relevant place in my V dev tree.  So the
lack of the flag at the moment isn't a big inconvenience.

Comment 9

[Jim Blandy :jimb]

Attachment: If we can't find <valgrind/valgrind.h> in the default search path, try /opt/local/include.

This patch is an alternative (or complementary) approach: if --enable-valgrind is specified, but we can't find <valgrind/valgrind.h> in the default search path, try adding /opt/local/include to the path.

The OSX valgrind-devel port installs the valgrind headers there, so this patch would make that port work a bit more nicely out of the box for OSX developers. 

This won't help valgrind developers, but it seems like Julian isn't too concerned, so perhaps this patch will help a wider and slightly less engaged audience.

Comment 10

[Jim Blandy :jimb]

Luke, if you want this patch, say the word, and I'll r? it.

Comment 11

[Luke Wagner [:luke]]

Awesome, the patch makes it work out of the box!

Comment 12

[Chris Peterson [:cpeterson]]

jimb: is your r+'d patch for valgrind header path still relevant?

Comment 13

[Jim Blandy :jimb]

Yes, it is still relevant. Thanks very much for the reminder!