Bug 195982

Summary: [JSC] Do not create JIT related data under non-JIT mode
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch mark.lam: review+

Description Yusuke Suzuki 2019-03-19 19:33:04 PDT
[JSC] Do not create JIT related data under non-JIT mode
Comment 1 Yusuke Suzuki 2019-03-19 19:36:06 PDT
Created attachment 365292 [details]
Patch
Comment 2 Yusuke Suzuki 2019-03-20 16:59:37 PDT
Comment on attachment 365292 [details]
Patch

I'll add Wasm::Thunk too.
Comment 3 Yusuke Suzuki 2019-03-20 20:14:46 PDT
Created attachment 365467 [details]
Patch
Comment 4 Yusuke Suzuki 2019-03-20 20:33:39 PDT
Created attachment 365473 [details]
Patch
Comment 5 Yusuke Suzuki 2019-03-20 20:35:53 PDT
Created attachment 365474 [details]
Patch
Comment 6 Yusuke Suzuki 2019-03-20 20:40:46 PDT
Created attachment 365476 [details]
Patch
Comment 7 Mark Lam 2019-03-21 01:47:15 PDT
Comment on attachment 365476 [details]
Patch

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

r=me with fixes.  If you haven't already done so, please also do a cloop build (build-jsc --cloop) to make sure it still builds.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:66
> +    enum ProtectionSetting { Writable, Executable };

Looks like this is no longer used anywhere.  We can delete it.

> Source/JavaScriptCore/runtime/VM.cpp:195
> +    ExecutableAllocator::initializeUnderlyingAllocator();
> +    if (!ExecutableAllocator::singleton().isValid()) {

While you're in this function, I suggest you also fix it to check getenv("JavaScriptCoreUseJIT") first (currently done below) before initializing the ExecutableAllocator.  No point in doing so if getenv("JavaScriptCoreUseJIT") disallows use of the JIT.

> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:-63
> -#if USE(ARM64_DISASSEMBLER)
> -    A64DOpcode m_arm64Opcode;
> -#endif

Undo this.  See the reason below.

> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:288
> +    auto arm64Opcode = std::make_unique<A64DOpcode>();

dumpCodeBlock() is called from SigillCrashAnalyzer::analyze(), which in turn is called from a signal handler.  We don't want to malloc stuff while in the signal handler.  So, let's keep the A64DOpcode where it's at right now, in the SigillCrashAnalyzer.  Instead, let's change Options.cpp to not enableSigillCrashAnalyzer() if the JIT is not in use.  The SigillCrashAnalyzer is for analyzing JIT code that went haywire.  If the JIT is not in use, we don't need to enable it, and hence, won't allocate the A64DOpcode.
Comment 8 Yusuke Suzuki 2019-03-21 10:11:00 PDT
Comment on attachment 365476 [details]
Patch

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

>> Source/JavaScriptCore/jit/ExecutableAllocator.h:66
>> +    enum ProtectionSetting { Writable, Executable };
> 
> Looks like this is no longer used anywhere.  We can delete it.

Removed.

>> Source/JavaScriptCore/runtime/VM.cpp:195
>> +    if (!ExecutableAllocator::singleton().isValid()) {
> 
> While you're in this function, I suggest you also fix it to check getenv("JavaScriptCoreUseJIT") first (currently done below) before initializing the ExecutableAllocator.  No point in doing so if getenv("JavaScriptCoreUseJIT") disallows use of the JIT.

Right. Fixed.

>> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:-63
>> -#endif
> 
> Undo this.  See the reason below.

Done.

>> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:288
>> +    auto arm64Opcode = std::make_unique<A64DOpcode>();
> 
> dumpCodeBlock() is called from SigillCrashAnalyzer::analyze(), which in turn is called from a signal handler.  We don't want to malloc stuff while in the signal handler.  So, let's keep the A64DOpcode where it's at right now, in the SigillCrashAnalyzer.  Instead, let's change Options.cpp to not enableSigillCrashAnalyzer() if the JIT is not in use.  The SigillCrashAnalyzer is for analyzing JIT code that went haywire.  If the JIT is not in use, we don't need to enable it, and hence, won't allocate the A64DOpcode.

Nice catch! Make sense. I changed

1. Not changing SigillCrashAnalyzer.cpp
2. Calling enableSigillCrashAnalyzer from InitializeThreading instead of Options.cpp
3. Calling enableSigillCrashAnalyzer only when VM::canUseJIT() returns true.
Comment 9 Yusuke Suzuki 2019-03-21 11:58:31 PDT
And I confirmed that CLoop is built successfully.
Comment 10 Yusuke Suzuki 2019-03-21 12:14:18 PDT
Committed r243312: <https://trac.webkit.org/changeset/243312>
Comment 11 Radar WebKit Bug Importer 2019-03-21 12:15:20 PDT
<rdar://problem/49118236>