VMTraps is only needed for JITted code. Hence, if the JIT is disabled, we should set Options::usePollingTraps() to true to indicate that we won't be using VMTraps.
<rdar://problem/63772519>
Created attachment 400619 [details] proposed patch.
Comment on attachment 400619 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=400619&action=review > Source/JavaScriptCore/runtime/Options.cpp:413 > + Options::usePollingTraps() = true; why not actually look at vm.canUseJIT or whatever?
Comment on attachment 400619 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=400619&action=review > Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:201 > + ASSERT(Options::useJIT()); and then relatedly, same check here.
> Source/JavaScriptCore/runtime/VMTraps.cpp:299 > + if (!Options::usePollingTraps()) { > + ASSERT(Options::useJIT()); Could Options::usePollingTraps() check useJIT() explicitly instead? Seems better for Options::usePollingTraps() to always do the right thing, instead of having callers check whether it did the right thing or not in an ASSERT.
(In reply to Saam Barati from comment #3) > Comment on attachment 400619 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400619&action=review > > > Source/JavaScriptCore/runtime/Options.cpp:413 > > + Options::usePollingTraps() = true; > > why not actually look at vm.canUseJIT or whatever? this is slightly tricky, since we rely on VM canUseJIT to be initialized after. I propose having "recomputeJITDependentOptions" that runs after we compute VM canUseJIT
(In reply to Geoffrey Garen from comment #5) > > Source/JavaScriptCore/runtime/VMTraps.cpp:299 > > + if (!Options::usePollingTraps()) { > > + ASSERT(Options::useJIT()); > > Could Options::usePollingTraps() check useJIT() explicitly instead? > > Seems better for Options::usePollingTraps() to always do the right thing, > instead of having callers check whether it did the right thing or not in an > ASSERT. That's what this patch is about: making Options::usePollingTraps() check useJIT(). Note: 1. if useJIT is true, then usePollingTraps is allowed to be true (for testing) or false (normal behavior). 2. if useJIT is false, then usePollingTraps should always be true (which is what this patch is making true).
> 2. if useJIT is false, then usePollingTraps should always be true (which is > what this patch is making true). Currently, this condition is enforced by an assignment upon initialization and an ASSERT at a call site to verify that the assignment happened. I'm asking if this condition could be enforced by a direct check inside Options::usePollingTraps() instead. That way, the behavior is more local and encapsulated, and harder / impossible to get wrong.
(In reply to Geoffrey Garen from comment #8) > > 2. if useJIT is false, then usePollingTraps should always be true (which is > > what this patch is making true). > > Currently, this condition is enforced by an assignment upon initialization > and an ASSERT at a call site to verify that the assignment happened. I'm > asking if this condition could be enforced by a direct check inside > Options::usePollingTraps() instead. That way, the behavior is more local and > encapsulated, and harder / impossible to get wrong. OK. Can't do it in Options::usePollingTraps() literally (because this is an auto generated accessors to a bool somewhere), but I can just check both conditions there.
(In reply to Mark Lam from comment #9) > (In reply to Geoffrey Garen from comment #8) > > > 2. if useJIT is false, then usePollingTraps should always be true (which is > > > what this patch is making true). > > > > Currently, this condition is enforced by an assignment upon initialization > > and an ASSERT at a call site to verify that the assignment happened. I'm > > asking if this condition could be enforced by a direct check inside > > Options::usePollingTraps() instead. That way, the behavior is more local and > > encapsulated, and harder / impossible to get wrong. > > OK. Can't do it in Options::usePollingTraps() literally (because this is an > auto generated accessors to a bool somewhere), but I can just check both > conditions there. Still feels redundant though. Options::usePollingTraps() is the canonical switch for the VMTraps feature. The right thing to do is to set it properly and have less conditions. The assert is there for documentation of the contract in addition to the check.
FYI, I think I disabled the ensure-crash.js test on 32-bit since I couldn't figure out why my change to expected crashing tests wasn't working. You might want to reenable it with this patch.
(In reply to Keith Miller from comment #11) > FYI, I think I disabled the ensure-crash.js test on 32-bit since I couldn't > figure out why my change to expected crashing tests wasn't working. You > might want to reenable it with this patch. I'll investigate and re-enable it in a separate patch if appropriate. Will truly to land this one first today.
Created attachment 401916 [details] proposed patch.
Comment on attachment 401916 [details] proposed patch. r=me. Is the idea the following should always be true Options::useJIT() == VM::canUseJIT()?
(In reply to Keith Miller from comment #14) > Comment on attachment 401916 [details] > proposed patch. > > r=me. Is the idea the following should always be true Options::useJIT() == > VM::canUseJIT()? After this patch, yes. But I won't leave the use of VM::canUseJIT() in place for now. Will address that in a separate patch. Removing it is not trivial.
Thanks for the reviews. Landed in r263055: <http://trac.webkit.org/r263055>.