Bug 195982 - [JSC] Do not create JIT related data under non-JIT mode
Summary: [JSC] Do not create JIT related data under non-JIT mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-19 19:33 PDT by Yusuke Suzuki
Modified: 2019-03-21 12:15 PDT (History)
6 users (show)

See Also:


Attachments
Patch (14.94 KB, patch)
2019-03-19 19:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (29.78 KB, patch)
2019-03-20 20:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (29.78 KB, patch)
2019-03-20 20:33 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (29.88 KB, patch)
2019-03-20 20:35 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (30.15 KB, patch)
2019-03-20 20:40 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>