- Revision
- 284590
- Author
- [email protected]
- Date
- 2021-10-20 23:27:53 -0700 (Wed, 20 Oct 2021)
Log Message
*IsSane API's could take in the Structure's we're consulting, or they can be out parameters, so we don't rely on the CPU's memory ordering
https://bugs.webkit.org/show_bug.cgi?id=231996
Reviewed by Filip Pizlo.
objectPrototypeIsSane, arrayPrototypeChainIsSane, and stringPrototypeChainIsSane reloads structures from prototype objects while the caller
is already getting them and validating them. This introduces a race condition where structure transition happens just before calling these
APIs and we will see different structures which are already validated. This is simply wrong: if we validate one structure, then we should
continue using that and we should put a watchpoint on this structure. We should not reload structures from the prototype again.
We add Concurrently postfix to these functions, and passing structures to these APIs to continue using these structures instead of reloading
it from prototype objects. This eliminate the race condition we had, and this removes the necessity of load-load-fence on watchpoint state
retrieval.
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGArrayMode.cpp:
(JSC::DFG::ArrayMode::refine const):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleIntrinsicCall):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::setSaneChainIfPossible):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileGetByValOnString):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileStringCharAtImpl):
* runtime/JSGlobalObject.h:
* runtime/JSGlobalObjectInlines.h:
(JSC::JSGlobalObject::objectPrototypeIsSane):
(JSC::JSGlobalObject::arrayPrototypeChainIsSaneConcurrently):
(JSC::JSGlobalObject::stringPrototypeChainIsSaneConcurrently):
(JSC::JSGlobalObject::arrayPrototypeChainIsSane):
(JSC::JSGlobalObject::stringPrototypeChainIsSane):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (284589 => 284590)
--- trunk/Source/_javascript_Core/ChangeLog 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-10-21 06:27:53 UTC (rev 284590)
@@ -1,5 +1,43 @@
2021-10-20 Yusuke Suzuki <[email protected]>
+ *IsSane API's could take in the Structure's we're consulting, or they can be out parameters, so we don't rely on the CPU's memory ordering
+ https://bugs.webkit.org/show_bug.cgi?id=231996
+
+ Reviewed by Filip Pizlo.
+
+ objectPrototypeIsSane, arrayPrototypeChainIsSane, and stringPrototypeChainIsSane reloads structures from prototype objects while the caller
+ is already getting them and validating them. This introduces a race condition where structure transition happens just before calling these
+ APIs and we will see different structures which are already validated. This is simply wrong: if we validate one structure, then we should
+ continue using that and we should put a watchpoint on this structure. We should not reload structures from the prototype again.
+
+ We add Concurrently postfix to these functions, and passing structures to these APIs to continue using these structures instead of reloading
+ it from prototype objects. This eliminate the race condition we had, and this removes the necessity of load-load-fence on watchpoint state
+ retrieval.
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGArgumentsEliminationPhase.cpp:
+ * dfg/DFGArrayMode.cpp:
+ (JSC::DFG::ArrayMode::refine const):
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::handleIntrinsicCall):
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ (JSC::DFG::FixupPhase::setSaneChainIfPossible):
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileGetByValOnString):
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileStringCharAtImpl):
+ * runtime/JSGlobalObject.h:
+ * runtime/JSGlobalObjectInlines.h:
+ (JSC::JSGlobalObject::objectPrototypeIsSane):
+ (JSC::JSGlobalObject::arrayPrototypeChainIsSaneConcurrently):
+ (JSC::JSGlobalObject::stringPrototypeChainIsSaneConcurrently):
+ (JSC::JSGlobalObject::arrayPrototypeChainIsSane):
+ (JSC::JSGlobalObject::stringPrototypeChainIsSane):
+
+2021-10-20 Yusuke Suzuki <[email protected]>
+
[JSC] ArithAbs should care about INT32_MIN
https://bugs.webkit.org/show_bug.cgi?id=232051
rdar://84338648
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (284589 => 284590)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2021-10-21 06:27:53 UTC (rev 284590)
@@ -2245,7 +2245,7 @@
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);
if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->arrayPrototypeChainIsSane()) {
+ && globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure)) {
m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
if (node->arrayMode().type() == Array::Double && node->arrayMode().isOutOfBoundsSaneChain() && !(node->flags() & NodeBytecodeUsesAsOther))
Modified: trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp (284589 => 284590)
--- trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2021-10-21 06:27:53 UTC (rev 284590)
@@ -252,7 +252,7 @@
JSGlobalObject* globalObject = m_graph.globalObjectFor(edge->origin.semantic);
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_graph.m_vm);
if (objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->objectPrototypeIsSane()) {
+ && globalObject->objectPrototypeIsSaneConcurrently(objectPrototypeStructure)) {
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
break;
}
@@ -278,7 +278,7 @@
Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_graph.m_vm);
if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->arrayPrototypeChainIsSane()) {
+ && globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure)) {
m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
break;
@@ -285,7 +285,7 @@
}
} else {
if (objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->objectPrototypeIsSane()) {
+ && globalObject->objectPrototypeIsSaneConcurrently(objectPrototypeStructure)) {
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGArrayMode.cpp (284589 => 284590)
--- trunk/Source/_javascript_Core/dfg/DFGArrayMode.cpp 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/dfg/DFGArrayMode.cpp 2021-10-21 06:27:53 UTC (rev 284590)
@@ -260,10 +260,10 @@
&& !graph.hasExitSite(node->origin.semantic, OutOfBounds)
&& arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->arrayPrototypeChainIsSane()) {
+ && globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure)) {
graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
graph.registerAndWatchStructureTransition(objectPrototypeStructure);
- if (globalObject->arrayPrototypeChainIsSane())
+ if (globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure))
return withSpeculation(Array::InBoundsSaneChain);
}
return ArrayMode(Array::Generic, action());
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (284589 => 284590)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2021-10-21 06:27:53 UTC (rev 284590)
@@ -2525,7 +2525,7 @@
&& globalObject->havingABadTimeWatchpoint()->isStillValid()
&& arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->arrayPrototypeChainIsSane()) {
+ && globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure)) {
m_graph.watchpoints().addLazily(globalObject->arraySpeciesWatchpointSet());
m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
@@ -2616,7 +2616,7 @@
// https://bugs.webkit.org/show_bug.cgi?id=173171
if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->arrayPrototypeChainIsSane()) {
+ && globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure)) {
m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
Modified: trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp (284589 => 284590)
--- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2021-10-21 06:27:53 UTC (rev 284590)
@@ -1679,7 +1679,7 @@
if (node->child1()->shouldSpeculateArray()
&& arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->arrayPrototypeChainIsSane()
+ && globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure)
&& m_graph.isWatchingArrayIteratorProtocolWatchpoint(node->child1().node())
&& m_graph.isWatchingHavingABadTimeWatchpoint(node->child1().node())) {
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
@@ -3783,7 +3783,7 @@
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm());
if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid()
&& objectPrototypeStructure->transitionWatchpointSetIsStillValid()
- && globalObject->arrayPrototypeChainIsSane()) {
+ && globalObject->arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure)) {
m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
node->setArrayMode(node->arrayMode().withSpeculation(speculation));
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (284589 => 284590)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2021-10-21 06:27:53 UTC (rev 284590)
@@ -2651,11 +2651,11 @@
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm);
WTF::dependentLoadLoadFence();
- if (globalObject->stringPrototypeChainIsSane()) {
+ if (globalObject->stringPrototypeChainIsSaneConcurrently(stringPrototypeStructure, objectPrototypeStructure)) {
// FIXME: This could be captured using a Speculation mode that means "out-of-bounds
// loads return a trivial value". Something like OutOfBoundsSaneChain. This should
// speculate that we don't take negative out-of-bounds, or better yet, it should rely
- // on a stringPrototypeChainIsSane() guaranteeing that the prototypes have no negative
+ // on a stringPrototypeChainIsSaneConcurrently() guaranteeing that the prototypes have no negative
// indexed properties either.
// https://bugs.webkit.org/show_bug.cgi?id=144668
m_jit.graph().registerAndWatchStructureTransition(stringPrototypeStructure);
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (284589 => 284590)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2021-10-21 06:27:53 UTC (rev 284590)
@@ -8927,7 +8927,7 @@
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm());
WTF::dependentLoadLoadFence();
- if (globalObject->stringPrototypeChainIsSane()) {
+ if (globalObject->stringPrototypeChainIsSaneConcurrently(stringPrototypeStructure, objectPrototypeStructure)) {
// FIXME: This could be captured using a Speculation mode that means
// "out-of-bounds loads return a trivial value", something like
// OutOfBoundsSaneChain.
Modified: trunk/Source/_javascript_Core/runtime/JSGlobalObject.h (284589 => 284590)
--- trunk/Source/_javascript_Core/runtime/JSGlobalObject.h 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/runtime/JSGlobalObject.h 2021-10-21 06:27:53 UTC (rev 284590)
@@ -1044,7 +1044,9 @@
void haveABadTime(VM&);
- bool objectPrototypeIsSane();
+ static bool objectPrototypeIsSaneConcurrently(Structure* objectPrototypeStructure);
+ bool arrayPrototypeChainIsSaneConcurrently(Structure* arrayPrototypeStructure, Structure* objectPrototypeStructure);
+ bool stringPrototypeChainIsSaneConcurrently(Structure* stringPrototypeStructure, Structure* objectPrototypeStructure);
bool arrayPrototypeChainIsSane();
bool stringPrototypeChainIsSane();
Modified: trunk/Source/_javascript_Core/runtime/JSGlobalObjectInlines.h (284589 => 284590)
--- trunk/Source/_javascript_Core/runtime/JSGlobalObjectInlines.h 2021-10-21 06:15:51 UTC (rev 284589)
+++ trunk/Source/_javascript_Core/runtime/JSGlobalObjectInlines.h 2021-10-21 06:27:53 UTC (rev 284590)
@@ -37,24 +37,45 @@
namespace JSC {
-ALWAYS_INLINE bool JSGlobalObject::objectPrototypeIsSane()
+ALWAYS_INLINE bool JSGlobalObject::objectPrototypeIsSaneConcurrently(Structure* objectPrototypeStructure)
{
- return !hasIndexedProperties(m_objectPrototype->indexingType())
- && m_objectPrototype->getPrototypeDirect(vm()).isNull();
+ return !hasIndexedProperties(objectPrototypeStructure->indexingType())
+ && objectPrototypeStructure->hasMonoProto()
+ && objectPrototypeStructure->storedPrototype().isNull();
}
+ALWAYS_INLINE bool JSGlobalObject::arrayPrototypeChainIsSaneConcurrently(Structure* arrayPrototypeStructure, Structure* objectPrototypeStructure)
+{
+ return !hasIndexedProperties(arrayPrototypeStructure->indexingType())
+ && arrayPrototypeStructure->hasMonoProto()
+ && arrayPrototypeStructure->storedPrototype() == objectPrototype()
+ && objectPrototypeIsSaneConcurrently(objectPrototypeStructure);
+}
+
+ALWAYS_INLINE bool JSGlobalObject::stringPrototypeChainIsSaneConcurrently(Structure* stringPrototypeStructure, Structure* objectPrototypeStructure)
+{
+ return !hasIndexedProperties(stringPrototypeStructure->indexingType())
+ && stringPrototypeStructure->hasMonoProto()
+ && stringPrototypeStructure->storedPrototype() == objectPrototype()
+ && objectPrototypeIsSaneConcurrently(objectPrototypeStructure);
+}
+
ALWAYS_INLINE bool JSGlobalObject::arrayPrototypeChainIsSane()
{
- return !hasIndexedProperties(m_arrayPrototype->indexingType())
- && m_arrayPrototype->getPrototypeDirect(vm()) == m_objectPrototype.get()
- && objectPrototypeIsSane();
+ VM& vm = this->vm();
+ ASSERT(!isCompilationThread() && !Thread::mayBeGCThread());
+ Structure* arrayPrototypeStructure = arrayPrototype()->structure(vm);
+ Structure* objectPrototypeStructure = objectPrototype()->structure(vm);
+ return arrayPrototypeChainIsSaneConcurrently(arrayPrototypeStructure, objectPrototypeStructure);
}
ALWAYS_INLINE bool JSGlobalObject::stringPrototypeChainIsSane()
{
- return !hasIndexedProperties(m_stringPrototype->indexingType())
- && m_stringPrototype->getPrototypeDirect(vm()) == m_objectPrototype.get()
- && objectPrototypeIsSane();
+ VM& vm = this->vm();
+ ASSERT(!isCompilationThread() && !Thread::mayBeGCThread());
+ Structure* stringPrototypeStructure = stringPrototype()->structure(vm);
+ Structure* objectPrototypeStructure = objectPrototype()->structure(vm);
+ return stringPrototypeChainIsSaneConcurrently(stringPrototypeStructure, objectPrototypeStructure);
}
ALWAYS_INLINE bool JSGlobalObject::isArrayPrototypeIteratorProtocolFastAndNonObservable()
