Bug 175037 - Baseline JIT should do caging
Summary: Baseline JIT should do caging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks: 174917
  Show dependency treegraph
 
Reported: 2017-08-01 11:10 PDT by Filip Pizlo
Modified: 2017-08-08 16:41 PDT (History)
11 users (show)

See Also:


Attachments
the patch (18.82 KB, patch)
2017-08-07 15:17 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (23.77 KB, patch)
2017-08-07 15:24 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (23.77 KB, patch)
2017-08-07 15:32 PDT, Filip Pizlo
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 Filip Pizlo 2017-08-01 11:10:29 PDT
...
Comment 1 Filip Pizlo 2017-08-07 15:17:47 PDT
Created attachment 317476 [details]
the patch
Comment 2 Filip Pizlo 2017-08-07 15:24:52 PDT
Created attachment 317479 [details]
the patch
Comment 3 Filip Pizlo 2017-08-07 15:32:39 PDT
Created attachment 317486 [details]
more

Hopefully fixes things.
Comment 4 Mark Lam 2017-08-07 16:12:34 PDT
Comment on attachment 317486 [details]
more

View in context: https://bugs.webkit.org/attachment.cgi?id=317486&action=review

r=me with suggestions.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1322
> +        if (!Gigacage::shouldBeEnabled())
> +            return;
> +        
> +#if GIGACAGE_ENABLED
> +        andPtr(TrustedImmPtr(static_cast<size_t>(GIGACAGE_MASK)), storage);
> +        addPtr(TrustedImmPtr(Gigacage::basePtr(kind)), storage);
> +#endif

You can move the #if GIGACAGE_ENABLED before the "if (!Gigacage::shouldBeEnabled())" check.  Need to add UNUSED_PARAM for kind and storage in the #else case though.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1339
> +        if (!Gigacage::shouldBeEnabled())
> +            return;
> +        
> +        if (kind != Gigacage::Primitive || Gigacage::isDisablingPrimitiveGigacageDisabled())
> +            return cage(kind, storage);
> +        
> +        loadPtr(Gigacage::basePtr(kind), scratch);
> +        Jump done = branchTestPtr(Zero, scratch);
> +#if GIGACAGE_ENABLED
> +        andPtr(TrustedImmPtr(static_cast<size_t>(GIGACAGE_MASK)), storage);
> +        addPtr(scratch, storage);
> +#endif
> +        done.link(this);

You can move the #if GIGACAGE_ENABLED before the "if (!Gigacage::shouldBeEnabled())" check.  Need to add UNUSED_PARAM for kind, storage, and scratch in the #else case though.
Comment 5 Filip Pizlo 2017-08-07 16:13:35 PDT
(In reply to Mark Lam from comment #4)
> Comment on attachment 317486 [details]
> more
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317486&action=review
> 
> r=me with suggestions.
> 
> > Source/JavaScriptCore/jit/AssemblyHelpers.h:1322
> > +        if (!Gigacage::shouldBeEnabled())
> > +            return;
> > +        
> > +#if GIGACAGE_ENABLED
> > +        andPtr(TrustedImmPtr(static_cast<size_t>(GIGACAGE_MASK)), storage);
> > +        addPtr(TrustedImmPtr(Gigacage::basePtr(kind)), storage);
> > +#endif
> 
> You can move the #if GIGACAGE_ENABLED before the "if
> (!Gigacage::shouldBeEnabled())" check.  Need to add UNUSED_PARAM for kind
> and storage in the #else case though.
> 
> > Source/JavaScriptCore/jit/AssemblyHelpers.h:1339
> > +        if (!Gigacage::shouldBeEnabled())
> > +            return;
> > +        
> > +        if (kind != Gigacage::Primitive || Gigacage::isDisablingPrimitiveGigacageDisabled())
> > +            return cage(kind, storage);
> > +        
> > +        loadPtr(Gigacage::basePtr(kind), scratch);
> > +        Jump done = branchTestPtr(Zero, scratch);
> > +#if GIGACAGE_ENABLED
> > +        andPtr(TrustedImmPtr(static_cast<size_t>(GIGACAGE_MASK)), storage);
> > +        addPtr(scratch, storage);
> > +#endif
> > +        done.link(this);
> 
> You can move the #if GIGACAGE_ENABLED before the "if
> (!Gigacage::shouldBeEnabled())" check.  Need to add UNUSED_PARAM for kind,
> storage, and scratch in the #else case though.

SGTM, I'll make those changes.  Thanks!
Comment 6 Filip Pizlo 2017-08-07 16:30:35 PDT
Landed in https://trac.webkit.org/changeset/220368/webkit
Comment 7 Radar WebKit Bug Importer 2017-08-07 16:31:25 PDT
<rdar://problem/33764948>
Comment 8 Ryan Haddad 2017-08-08 09:00:11 PDT
Reverted r220368 for reason:

This change caused WK1 tests to exit early with crashes.

Committed r220404: <http://trac.webkit.org/changeset/220404>
Comment 9 Ryan Haddad 2017-08-08 09:01:15 PDT
(In reply to Ryan Haddad from comment #8)
> Reverted r220368 for reason:
> 
> This change caused WK1 tests to exit early with crashes.
> 
> Committed r220404: <http://trac.webkit.org/changeset/220404>
Link to test run that exhibits the crashes:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/3717
Comment 10 Filip Pizlo 2017-08-08 12:44:55 PDT
Relanded in after fixing cageConditionally() for non-WK2/jsc: https://trac.webkit.org/changeset/220416/webkit
Comment 11 Filip Pizlo 2017-08-08 16:41:10 PDT
Relanded in after fixing cageConditionally() for non-WK2/jsc: https://trac.webkit.org/changeset/220416/webkit