[JSC][Linux] Support Perf JITDump logging
Created attachment 350557 [details] Patch
Created attachment 350558 [details] Screenshot #1
Created attachment 350559 [details] Screenshot #2
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.
Created attachment 350562 [details] Patch
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.
Created attachment 350563 [details] Patch
Created attachment 350564 [details] Patch
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.
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.
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.
Created attachment 350565 [details] Patch
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.
Ping?
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.
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.
Committed r236883: <https://trac.webkit.org/changeset/236883>
<rdar://problem/45051667>