Bug 189893 – [JSC][Linux] Support Perf JITDump logging

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC][Linux] Support Perf JITDump logging

[Yusuke Suzuki]

Created attachment 350557 [details]
Patch

[Yusuke Suzuki]

Created attachment 350558 [details]
Screenshot #1

[Yusuke Suzuki]

Created attachment 350559 [details]
Screenshot #2

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:356:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:358:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
Total errors found: 2 in 8 files


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

[Yusuke Suzuki]

Created attachment 350562 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:356:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:358:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
Total errors found: 2 in 8 files


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

[Yusuke Suzuki]

Created attachment 350563 [details]
Patch

[Yusuke Suzuki]

Created attachment 350564 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:356:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:358:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
Total errors found: 2 in 8 files


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

[Yusuke Suzuki]

Comment on attachment 350564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350564&action=review

> Source/JavaScriptCore/ChangeLog:20
> +        Currently, due to perf inject command's bug (maybe), some JIT code information is not attached,
> +        but it should be fixed in perf command's side.

I've found that this is because perf's MMAP record is a bit tricky if we use `mmap(..., PROT_NONE, ...)`.
The simple workaround is that using the normal protection in OSAllocator::reserveUncommitted OS(LINUX) code if perf is enabled.

[Yusuke Suzuki]

Comment on attachment 350564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350564&action=review

> Source/JavaScriptCore/assembler/PerfLog.cpp:185
> +    if (!size) {
> +        dataLogLnIf(PerfLogInternal::verbose, "0 size record ", name, " ", RawPointer(executableAddress));
> +        return;
> +    }

If we have 0-sized record, `perf inject` command's fixing up overlapping map region gets stuck! So, recording 0-sized code load is not allowed.

[Yusuke Suzuki]

Created attachment 350565 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:356:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.h:358:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
Total errors found: 2 in 11 files


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

[Yusuke Suzuki]

Ping?

[Mark Lam]

Comment on attachment 350565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350565&action=review

r=me with fixes.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1162
> +		E45E4CF7243D4BFF924852DC /* PerfLog.h in Headers */ = {isa = PBXBuildFile; fileRef = 7CF028A1ED94468C977A3BB2 /* PerfLog.h */; settings = {ATTRIBUTES = (Private, ); }; };

Do we need to add to JavaScriptCore.xcodeproj/project.pbxproj at all given this is a linux only feature?  If you can build on Mac without this, then let's leave it out.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:2205
> +		1AEC53058BC44112AF424E00 /* PerfLog.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PerfLog.cpp; sourceTree = "<group>"; };

Ditto.

> Source/JavaScriptCore/assembler/LinkBuffer.h:357
> +        : (UNLIKELY(JSC::Options::logJITCodeForPerf()) \
> +            ? (linkBufferReference).finalizeCodeWithDisassembly<resultPtrTag>(false, __VA_ARGS__) \

Can you wrap this in #if OS(LINUX)?

> Source/JavaScriptCore/assembler/PerfLog.cpp:210
> +#endif

nit: Add // ENABLE(ASSEMBLER) && OS(LINUX)

> Source/JavaScriptCore/assembler/PerfLog.h:57
> +#endif

nit: Add ENABLE(ASSEMBLER) && OS(LINUX)

> Source/JavaScriptCore/runtime/Options.h:182
> +    v(bool, logJITCodeForPerf, false, Normal, nullptr) \

Since this feature is linux only, can you make this Configurable instead of Normal, and disable it completely for anything other than linux.  See the useSigillCrashAnalyzer option for an example.  This way, we won't falsely advertise that its an available option.

[Yusuke Suzuki]

Comment on attachment 350565 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350565&action=review

Thank you!

>> Source/JavaScriptCore/assembler/LinkBuffer.h:357
>> +            ? (linkBufferReference).finalizeCodeWithDisassembly<resultPtrTag>(false, __VA_ARGS__) \
> 
> Can you wrap this in #if OS(LINUX)?

Fixed.

>> Source/JavaScriptCore/assembler/PerfLog.cpp:210
>> +#endif
> 
> nit: Add // ENABLE(ASSEMBLER) && OS(LINUX)

Fixed.

>> Source/JavaScriptCore/assembler/PerfLog.h:57
>> +#endif
> 
> nit: Add ENABLE(ASSEMBLER) && OS(LINUX)

Fixed.

>> Source/JavaScriptCore/runtime/Options.h:182
>> +    v(bool, logJITCodeForPerf, false, Normal, nullptr) \
> 
> Since this feature is linux only, can you make this Configurable instead of Normal, and disable it completely for anything other than linux.  See the useSigillCrashAnalyzer option for an example.  This way, we won't falsely advertise that its an available option.

Nice, fixed.

[Yusuke Suzuki]

Committed r236883: <https://trac.webkit.org/changeset/236883>

[Radar WebKit Bug Importer]

<rdar://problem/45051667>