Title: [249247] trunk
- Revision
- 249247
- Author
- mark....@apple.com
- Date
- 2019-08-28 23:49:28 -0700 (Wed, 28 Aug 2019)
Log Message
DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
https://bugs.webkit.org/show_bug.cgi?id=201281
<rdar://problem/54028228>
Reviewed by Yusuke Suzuki and Saam Barati.
JSTests:
* stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js: Added.
Source/_javascript_Core:
This (see title above) is already the preferred idiom used in most places in our
compiler, except for 2: DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
compileStringCharAt(). Consider the following:
bool prototypeChainIsSane = false;
if (globalObject->stringPrototypeChainIsSane()) {
...
m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));
prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
}
What's essential for correctness here is that the stringPrototype and objectPrototype
structures be loaded before the loads in the second stringPrototypeChainIsSane()
check. Without a loadLoadFence before the second stringPrototypeChainIsSane()
check, we can't guarantee that. Elsewhere in the compiler, the preferred idiom
for doing this right is to pre-load the structures first, do a loadLoadFence, and
then do the IsSane check just once after e.g.
Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_vm);
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);
if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
&& globalObject->arrayPrototypeChainIsSane()) {
m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
...
}
This patch changes DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
compileStringCharAt() to follow the same idiom.
We also fix a bad assertion in Structure::storedPrototype() and
Structure::storedPrototypeObject(). The assertion is only correct when those
methods are called from the mutator thread. The assertion has been updated to
only check its test condition if the current thread is the mutator thread.
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileGetByValOnString):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
* runtime/StructureInlines.h:
(JSC::Structure::storedPrototype const):
(JSC::Structure::storedPrototypeObject const):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (249246 => 249247)
--- trunk/JSTests/ChangeLog 2019-08-29 06:46:27 UTC (rev 249246)
+++ trunk/JSTests/ChangeLog 2019-08-29 06:49:28 UTC (rev 249247)
@@ -1,5 +1,15 @@
2019-08-28 Mark Lam <mark....@apple.com>
+ DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
+ https://bugs.webkit.org/show_bug.cgi?id=201281
+ <rdar://problem/54028228>
+
+ Reviewed by Yusuke Suzuki and Saam Barati.
+
+ * stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js: Added.
+
+2019-08-28 Mark Lam <mark....@apple.com>
+
Placate exception check validation in DFG's operationHasGenericProperty().
https://bugs.webkit.org/show_bug.cgi?id=201245
<rdar://problem/54777512>
Added: trunk/JSTests/stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js (0 => 249247)
--- trunk/JSTests/stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js (rev 0)
+++ trunk/JSTests/stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js 2019-08-29 06:49:28 UTC (rev 249247)
@@ -0,0 +1,22 @@
+//@ skip if ["arm", "mips"].include?($architecture)
+//@ slow!
+//@ runDefault("--jitPolicyScale=0")
+
+// This test should not crash.
+
+// Increase iterations to 10000 if you want the regression to reproduce more reliably.
+// It can manifest in just a few iterations or may take a lot more iterations. We're
+// reducing iterations here to shorten the execution time of this test for normal runs,
+// with the tradeoff that some runs may not trigger the regression (if present). This is
+// so that fixed builds (which is the likely case going forward) won't have to wait too
+// long for this test to finish.
+const iterations = 500;
+for (let i = 0; i < iterations; i++) {
+ let code = `
+ for (let i = 0; i < 1000; i++) {
+ String.prototype.__proto__ = [];
+ const w = 'abcdefg'[-2];
+ }
+ `;
+ runString(code);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (249246 => 249247)
--- trunk/Source/_javascript_Core/ChangeLog 2019-08-29 06:46:27 UTC (rev 249246)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-08-29 06:49:28 UTC (rev 249247)
@@ -1,5 +1,61 @@
2019-08-28 Mark Lam <mark....@apple.com>
+ DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
+ https://bugs.webkit.org/show_bug.cgi?id=201281
+ <rdar://problem/54028228>
+
+ Reviewed by Yusuke Suzuki and Saam Barati.
+
+ This (see title above) is already the preferred idiom used in most places in our
+ compiler, except for 2: DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
+ compileStringCharAt(). Consider the following:
+
+ bool prototypeChainIsSane = false;
+ if (globalObject->stringPrototypeChainIsSane()) {
+ ...
+ m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
+ m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));
+
+ prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
+ }
+
+ What's essential for correctness here is that the stringPrototype and objectPrototype
+ structures be loaded before the loads in the second stringPrototypeChainIsSane()
+ check. Without a loadLoadFence before the second stringPrototypeChainIsSane()
+ check, we can't guarantee that. Elsewhere in the compiler, the preferred idiom
+ for doing this right is to pre-load the structures first, do a loadLoadFence, and
+ then do the IsSane check just once after e.g.
+
+ Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_vm);
+ Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);
+
+ if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
+ && objectPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
+ && globalObject->arrayPrototypeChainIsSane()) {
+
+ m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
+ m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
+ ...
+ }
+
+ This patch changes DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
+ compileStringCharAt() to follow the same idiom.
+
+ We also fix a bad assertion in Structure::storedPrototype() and
+ Structure::storedPrototypeObject(). The assertion is only correct when those
+ methods are called from the mutator thread. The assertion has been updated to
+ only check its test condition if the current thread is the mutator thread.
+
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileGetByValOnString):
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
+ * runtime/StructureInlines.h:
+ (JSC::Structure::storedPrototype const):
+ (JSC::Structure::storedPrototypeObject const):
+
+2019-08-28 Mark Lam <mark....@apple.com>
+
Placate exception check validation in DFG's operationHasGenericProperty().
https://bugs.webkit.org/show_bug.cgi?id=201245
<rdar://problem/54777512>
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (249246 => 249247)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2019-08-29 06:46:27 UTC (rev 249246)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2019-08-29 06:49:28 UTC (rev 249247)
@@ -2231,7 +2231,10 @@
#endif
JSGlobalObject* globalObject = m_jit.globalObjectFor(node->origin.semantic);
- bool prototypeChainIsSane = false;
+ Structure* stringPrototypeStructure = globalObject->stringPrototype()->structure(vm);
+ Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm);
+ WTF::loadLoadFence();
+
if (globalObject->stringPrototypeChainIsSane()) {
// FIXME: This could be captured using a Speculation mode that means "out-of-bounds
// loads return a trivial value". Something like SaneChainOutOfBounds. This should
@@ -2239,11 +2242,9 @@
// on a stringPrototypeChainIsSane() guaranteeing that the prototypes have no negative
// indexed properties either.
// https://bugs.webkit.org/show_bug.cgi?id=144668
- m_jit.graph().registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm));
- m_jit.graph().registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm));
- prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
- }
- if (prototypeChainIsSane) {
+ m_jit.graph().registerAndWatchStructureTransition(stringPrototypeStructure);
+ m_jit.graph().registerAndWatchStructureTransition(objectPrototypeStructure);
+
#if USE(JSVALUE64)
addSlowPathGenerator(makeUnique<SaneStringGetByValSlowPathGenerator>(
outOfBounds, this, JSValueRegs(scratchReg), baseReg, propertyReg));
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (249246 => 249247)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-08-29 06:46:27 UTC (rev 249246)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-08-29 06:49:28 UTC (rev 249247)
@@ -6978,8 +6978,10 @@
results.append(m_out.anchor(m_out.intPtrZero));
} else {
JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
-
- bool prototypeChainIsSane = false;
+ Structure* stringPrototypeStructure = globalObject->stringPrototype()->structure(vm());
+ Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm());
+ WTF::loadLoadFence();
+
if (globalObject->stringPrototypeChainIsSane()) {
// FIXME: This could be captured using a Speculation mode that means
// "out-of-bounds loads return a trivial value", something like
@@ -6986,12 +6988,9 @@
// SaneChainOutOfBounds.
// https://bugs.webkit.org/show_bug.cgi?id=144668
- m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
- m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));
+ m_graph.registerAndWatchStructureTransition(stringPrototypeStructure);
+ m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
- prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
- }
- if (prototypeChainIsSane) {
LBasicBlock negativeIndex = m_out.newBlock();
results.append(m_out.anchor(m_out.constInt64(JSValue::encode(jsUndefined()))));
Modified: trunk/Source/_javascript_Core/runtime/StructureInlines.h (249246 => 249247)
--- trunk/Source/_javascript_Core/runtime/StructureInlines.h 2019-08-29 06:46:27 UTC (rev 249246)
+++ trunk/Source/_javascript_Core/runtime/StructureInlines.h 2019-08-29 06:49:28 UTC (rev 249247)
@@ -108,7 +108,7 @@
ALWAYS_INLINE JSValue Structure::storedPrototype(const JSObject* object) const
{
- ASSERT(object->structure() == this);
+ ASSERT(!isMainThread() || object->structure() == this);
if (hasMonoProto())
return storedPrototype();
return object->getDirect(knownPolyProtoOffset);
@@ -116,7 +116,7 @@
ALWAYS_INLINE JSObject* Structure::storedPrototypeObject(const JSObject* object) const
{
- ASSERT(object->structure() == this);
+ ASSERT(!isMainThread() || object->structure() == this);
if (hasMonoProto())
return storedPrototypeObject();
JSValue proto = object->getDirect(knownPolyProtoOffset);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
