Bug 195982 – [JSC] Do not create JIT related data under non-JIT mode

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

[Yusuke Suzuki]

[JSC] Do not create JIT related data under non-JIT mode

[Yusuke Suzuki]

Created attachment 365292 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 365292 [details]
Patch

I'll add Wasm::Thunk too.

[Yusuke Suzuki]

Created attachment 365467 [details]
Patch

[Yusuke Suzuki]

Created attachment 365473 [details]
Patch

[Yusuke Suzuki]

Created attachment 365474 [details]
Patch

[Yusuke Suzuki]

Created attachment 365476 [details]
Patch

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

And I confirmed that CLoop is built successfully.

[Yusuke Suzuki]

Committed r243312: <https://trac.webkit.org/changeset/243312>

[Radar WebKit Bug Importer]

<rdar://problem/49118236>