Bug 73300 – ExecutableAllocator::cacheFlush() should flush valgrind's caches

• Status: UNCONFIRMED
• Component: JavaScriptCore

[Andy Wingo]

You probably know valgrind.  It is helpful to run on release binaries to get backtraces, detect errors, and other such things.  Like most real CPUs, valgrind caches program text.  It has an option not to cache the text -- or rather, to detect self-modifying code.  This --smc-check option works, but it's easy to forget it (as in bug 72883 comment 0), and it is very, very slow.

We can make valgrind work though by adding some strange noop instructions that valgrind detects and uses to flush its caches:

    http://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clientreq

Quoting:

    The macros in these header files have the magical property that they generate code in-line which Valgrind can spot. However, the code does nothing when not run on Valgrind, so you are not forced to run your program under Valgrind just because you use the macros in this file. Also, you are not required to link your program with any extra supporting libraries.

    The code added to your binary has negligible performance impact: on x86, amd64, ppc32, ppc64 and ARM, the overhead is 6 simple integer instructions and is probably undetectable except in tight loops. However, if you really wish to compile out the client requests, you can compile with -DNVALGRIND (analogous to -DNDEBUG's effect on assert).

    You are encouraged to copy the valgrind/*.h headers into your project's include directory, so your program doesn't have a compile-time dependency on Valgrind being installed. The Valgrind headers, unlike most of the rest of the code, are under a BSD-style license so you may include them without worrying about license incompatibility.

So this patch does just that, adding the valgrind.h header file to wtf.  V8 does the same, so there shouldn't be a significant performance impact.

[Andy Wingo]

Created attachment 116934 [details]
Patch

[WebKit Review Bot]

Attachment 116934 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Last 3072 characters of output:
identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/valgrind.h:3722:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3722:  _qzz_res is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/valgrind.h:3724:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3725:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3727:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3733:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3739:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3740:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3745:  VALGRIND_PRINTF_BACKTRACE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/valgrind.h:3746:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3755:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3758:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3758:  _qzz_res is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/valgrind.h:3760:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3760:  _qzz_res is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/valgrind.h:3762:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3763:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3765:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3771:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3777:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/valgrind.h:3778:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 644 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 116934 [details]
Patch

Attachment 116934 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10678686

[Andy Wingo]

AFAICT the style error is fine, as it's just the valgrind.h, and the chromium EWS appears to have failed for some other reason.

[Andy Wingo]

I'll repost this patch after the WTF merge, but if a JIT hacker has any comment on the inclusion of valgrind.h into wtf, it is most welcome.

[Eric Seidel (no email)]

Comment on attachment 116934 [details]
Patch

I'm not convinced this is a good patch.  Why don't we implement our own macros which optionally compile to Valgrind macros when valgrind is installed/enabled.

[Andy Wingo]

(In reply to comment #6)
> (From update of attachment 116934 [details])
> I'm not convinced this is a good patch.  Why don't we implement our own macros which optionally compile to Valgrind macros when valgrind is installed/enabled.

The reason is that Valgrind headers are not normally installed; they are designed to be copied into your source tree.  The binary ABI is stable, but the source API is not (AFAIK).

Copying this single header into WTF is the right thing IMHO.  We already have our own abstractions to flush the icaches, and adding a new one will not help anything.

[Filip Pizlo]

(In reply to comment #6)
> (From update of attachment 116934 [details])
> I'm not convinced this is a good patch.  Why don't we implement our own macros which optionally compile to Valgrind macros when valgrind is installed/enabled.

See Andy's response.

I think that (1) valgrind is a great tool for those using JSC on Linux and (2) this patch isn't going to get in anyone's way.

The only issue is to make sure that the license is appropriately compatible.