Bug 212543 – Do not install the VMTraps signal handler if Options::useJIT=false.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Radar WebKit Bug Importer]

<rdar://problem/63772519>

[Mark Lam]

Created attachment 400619 [details]
proposed patch.

[Saam Barati]

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?

[Saam Barati]

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.

[Geoffrey Garen]

> 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.

[Saam Barati]

(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

[Mark Lam]

(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).

[Geoffrey Garen]

> 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.

[Mark Lam]

(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.

[Mark Lam]

(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.

[Keith Miller]

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.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 401916 [details]
proposed patch.

[Keith Miller]

Comment on attachment 401916 [details]
proposed patch.

r=me. Is the idea the following should always be true Options::useJIT() == VM::canUseJIT()?

[Mark Lam]

(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.

[Mark Lam]

Thanks for the reviews.  Landed in r263055: <http://trac.webkit.org/r263055>.