Summary: | [JSC] Do not create JIT related data under non-JIT mode | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Component: | New Bugs | Assignee: | 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
Yusuke Suzuki
2019-03-19 19:33:04 PDT
Created attachment 365292 [details]
Patch
Comment on attachment 365292 [details]
Patch
I'll add Wasm::Thunk too.
Created attachment 365467 [details]
Patch
Created attachment 365473 [details]
Patch
Created attachment 365474 [details]
Patch
Created attachment 365476 [details]
Patch
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 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. And I confirmed that CLoop is built successfully. Committed r243312: <https://trac.webkit.org/changeset/243312> |