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

NEW
Assigned to

Status

()

9 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 396604 [details] [diff] [review]
Support specifying Valgrind header path.
Assignee: general → jim
Attachment #396604 - Flags: review?(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.
Attachment #396604 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 3

9 years ago
We need the same thing for NSPR, as well.  Let me revise the patch.
That needs to be a separate patch. (but I can still review it)
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.
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.
(Assignee)

Comment 7

7 years ago
(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.
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.
(Assignee)

Comment 9

7 years ago
Created attachment 625678 [details] [diff] [review]
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.
(Assignee)

Comment 10

7 years ago
Luke, if you want this patch, say the word, and I'll r? it.
Awesome, the patch makes it work out of the box!
jimb: is your r+'d patch for valgrind header path still relevant?
Flags: needinfo?(jimb)
(Assignee)

Comment 13

5 years ago
Yes, it is still relevant. Thanks very much for the reminder!
Flags: needinfo?(jimb)
You need to log in before you can comment on or make changes to this bug.