Bug 212543 - Do not install the VMTraps signal handler if Options::useJIT=false.
Summary: Do not install the VMTraps signal handler if Options::useJIT=false.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-29 14:01 PDT by Mark Lam
Modified: 2020-06-15 13:12 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (3.29 KB, patch)
2020-05-29 14:37 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
proposed patch. (6.71 KB, patch)
2020-06-15 11:37 PDT, Mark Lam
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-05-29 14:01:15 PDT
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.
Comment 1 Radar WebKit Bug Importer 2020-05-29 14:01:40 PDT
<rdar://problem/63772519>
Comment 2 Mark Lam 2020-05-29 14:37:34 PDT
Created attachment 400619 [details]
proposed patch.
Comment 3 Saam Barati 2020-05-29 14:47:07 PDT
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 4 Saam Barati 2020-05-29 14:47:50 PDT
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.
Comment 5 Geoffrey Garen 2020-05-29 14:48:28 PDT
> 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.
Comment 6 Saam Barati 2020-05-29 15:00:08 PDT
(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
Comment 7 Mark Lam 2020-05-29 15:05:46 PDT
(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).
Comment 8 Geoffrey Garen 2020-05-29 15:26:24 PDT
> 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.
Comment 9 Mark Lam 2020-05-29 15:35:28 PDT
(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.
Comment 10 Mark Lam 2020-05-29 15:38:33 PDT
(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.
Comment 11 Keith Miller 2020-06-15 11:16:04 PDT
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.
Comment 12 Mark Lam 2020-06-15 11:17:24 PDT
(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.
Comment 13 Mark Lam 2020-06-15 11:37:32 PDT
Created attachment 401916 [details]
proposed patch.
Comment 14 Keith Miller 2020-06-15 12:55:04 PDT
Comment on attachment 401916 [details]
proposed patch.

r=me. Is the idea the following should always be true Options::useJIT() == VM::canUseJIT()?
Comment 15 Mark Lam 2020-06-15 12:56:26 PDT
(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.
Comment 16 Mark Lam 2020-06-15 13:00:56 PDT
Thanks for the reviews.  Landed in r263055: <http://trac.webkit.org/r263055>.