Bug 175037

Summary:

Baseline JIT should do caging

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, buildbot, cdumez, cmarcelo, dbates, keith_miller, mark.lam, msaboff, ryanhaddad, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

174917

Attachments:

Description	Flags
the patch	none
the patch	none
more	mark.lam: review+

Filip Pizlo

Reported 2017-08-01 11:10:29 PDT

...

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2017-08-07 15:17:47 PDT

Created attachment 317476 [details] the patch

Filip Pizlo

Comment 2 2017-08-07 15:24:52 PDT

Created attachment 317479 [details] the patch

Filip Pizlo

Comment 3 2017-08-07 15:32:39 PDT

Created attachment 317486 [details] more Hopefully fixes things.

Mark Lam

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

Filip Pizlo

Comment 5 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!

Filip Pizlo

Comment 6 2017-08-07 16:30:35 PDT

Landed in https://trac.webkit.org/changeset/220368/webkit

Radar WebKit Bug Importer

Comment 7 2017-08-07 16:31:25 PDT

<rdar://problem/33764948>

Ryan Haddad

Comment 8 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>

Ryan Haddad

Comment 9 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

Filip Pizlo

Comment 10 2017-08-08 12:44:55 PDT

Relanded in after fixing cageConditionally() for non-WK2/jsc: https://trac.webkit.org/changeset/220416/webkit

Filip Pizlo

Comment 11 2017-08-08 16:41:10 PDT

Relanded in after fixing cageConditionally() for non-WK2/jsc: https://trac.webkit.org/changeset/220416/webkit

Note You need to log in before you can comment on or make changes to this bug.