[webkit-reviews] review granted: [Bug 189893] [JSC][Linux] Support Perf JITDump logging : [Attachment 350565] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<yusukesuzuki at slowstart.org>'s request for review:
Bug 189893: [JSC][Linux] Support Perf JITDump logging
https://bugs.webkit.org/show_bug.cgi?id=189893

Attachment 350565: Patch

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




--- Comment #15 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 350565
  --> https://bugs.webkit.org/attachment.cgi?id=350565
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.