- Revision
- 281029
- Author
- [email protected]
- Date
- 2021-08-13 12:29:04 -0700 (Fri, 13 Aug 2021)
Log Message
EnumeratorNextUpdatePropertyName always needs to be able to handle IndexedMode
https://bugs.webkit.org/show_bug.cgi?id=229087
Reviewed by Filip Pizlo.
JSTests:
* stress/for-in-own-structure-and-generic-with-late-add-indexed.js: Added.
(test):
(Foo):
Source/_javascript_Core:
Right now, this operation incorrectly assumes that EnumeratorNextUpdateIndexAndMode will guarantee
the mode matches the seen mode set. But no speculation is guaranteed and adding such a guarantee
would require adding checkpoints, which is likely not worth it. Instead, this patch just makes
sure we always handle the allocation for IndexedMode.
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileEnumeratorNextUpdatePropertyName):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (281028 => 281029)
--- trunk/JSTests/ChangeLog 2021-08-13 19:10:25 UTC (rev 281028)
+++ trunk/JSTests/ChangeLog 2021-08-13 19:29:04 UTC (rev 281029)
@@ -1,3 +1,14 @@
+2021-08-13 Keith Miller <[email protected]>
+
+ EnumeratorNextUpdatePropertyName always needs to be able to handle IndexedMode
+ https://bugs.webkit.org/show_bug.cgi?id=229087
+
+ Reviewed by Filip Pizlo.
+
+ * stress/for-in-own-structure-and-generic-with-late-add-indexed.js: Added.
+ (test):
+ (Foo):
+
2021-08-11 Yusuke Suzuki <[email protected]>
WTFCrash in JSC::Lexer<char16_t>::append8
Added: trunk/JSTests/stress/for-in-own-structure-and-generic-with-late-add-indexed.js (0 => 281029)
--- trunk/JSTests/stress/for-in-own-structure-and-generic-with-late-add-indexed.js (rev 0)
+++ trunk/JSTests/stress/for-in-own-structure-and-generic-with-late-add-indexed.js 2021-08-13 19:29:04 UTC (rev 281029)
@@ -0,0 +1,28 @@
+function test(o) {
+ let sum = 0;
+ for (let i in o)
+ sum += o[i];
+ return sum;
+}
+noInline(test);
+
+Object.defineProperty(Object.prototype, "foo", { enumerable: true, value: 4 });
+
+class Foo extends Array {
+ b = 1;
+}
+
+let object = new Foo();
+let object2 = new Foo();
+object2.length = 100;
+object2.fill(1);
+
+for (let i = 0; i < 1e5; ++i) {
+ let sum = test(object);
+ if (sum !== 5)
+ throw new Error(sum);
+}
+
+let sum = test(object2);
+if (sum !== 105)
+ throw new Error(sum);
Modified: trunk/Source/_javascript_Core/ChangeLog (281028 => 281029)
--- trunk/Source/_javascript_Core/ChangeLog 2021-08-13 19:10:25 UTC (rev 281028)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-08-13 19:29:04 UTC (rev 281029)
@@ -1,3 +1,20 @@
+2021-08-13 Keith Miller <[email protected]>
+
+ EnumeratorNextUpdatePropertyName always needs to be able to handle IndexedMode
+ https://bugs.webkit.org/show_bug.cgi?id=229087
+
+ Reviewed by Filip Pizlo.
+
+ Right now, this operation incorrectly assumes that EnumeratorNextUpdateIndexAndMode will guarantee
+ the mode matches the seen mode set. But no speculation is guaranteed and adding such a guarantee
+ would require adding checkpoints, which is likely not worth it. Instead, this patch just makes
+ sure we always handle the allocation for IndexedMode.
+
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileEnumeratorNextUpdatePropertyName):
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):
+
2021-08-12 Mark Lam <[email protected]>
Refactor some ARM64EHash code.
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (281028 => 281029)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2021-08-13 19:10:25 UTC (rev 281028)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2021-08-13 19:29:04 UTC (rev 281029)
@@ -13591,17 +13591,13 @@
MacroAssembler::JumpList doneCases;
MacroAssembler::Jump operationCall;
- bool needsOperation = seenModes.contains(JSPropertyNameEnumerator::IndexedMode);
+ // Make sure we flush on all code paths if we will call the operation.
+ // Note: we can't omit the operation because we are not guaranteed EnumeratorUpdateIndexAndMode will speculate on the mode.
+ flushRegisters();
- // Make sure we flush on all code paths if we could call the operation.
- if (needsOperation)
- flushRegisters();
-
if (seenModes.containsAny({ JSPropertyNameEnumerator::OwnStructureMode, JSPropertyNameEnumerator::GenericMode })) {
+ operationCall = m_jit.branchTest32(MacroAssembler::NonZero, mode, TrustedImm32(JSPropertyNameEnumerator::IndexedMode));
- if (needsOperation)
- operationCall = m_jit.branchTest32(MacroAssembler::NonZero, mode, TrustedImm32(JSPropertyNameEnumerator::IndexedMode));
-
auto outOfBounds = m_jit.branch32(MacroAssembler::AboveOrEqual, index, MacroAssembler::Address(enumerator, JSPropertyNameEnumerator::endGenericPropertyIndexOffset()));
m_jit.loadPtr(MacroAssembler::Address(enumerator, JSPropertyNameEnumerator::cachedPropertyNamesVectorOffset()), resultRegs.payloadGPR());
@@ -13613,16 +13609,12 @@
outOfBounds.link(&m_jit);
m_jit.moveTrustedValue(jsNull(), resultRegs);
+ doneCases.append(m_jit.jump());
+ operationCall.link(&m_jit);
}
- if (needsOperation) {
- if (operationCall.isSet()) {
- doneCases.append(m_jit.jump());
- operationCall.link(&m_jit);
- }
- callOperation(operationEnumeratorNextUpdatePropertyName, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), index, mode, enumerator);
- m_jit.exceptionCheck();
- }
+ callOperation(operationEnumeratorNextUpdatePropertyName, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), index, mode, enumerator);
+ m_jit.exceptionCheck();
doneCases.link(&m_jit);
jsValueResult(resultRegs, node);
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (281028 => 281029)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2021-08-13 19:10:25 UTC (rev 281028)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2021-08-13 19:29:04 UTC (rev 281029)
@@ -13138,20 +13138,15 @@
OptionSet seenModes = m_node->enumeratorMetadata();
if (seenModes.containsAny({ JSPropertyNameEnumerator::OwnStructureMode, JSPropertyNameEnumerator::GenericMode })) {
- LBasicBlock checkIndex = nullptr;
+ LBasicBlock checkIndex = m_out.newBlock();
LBasicBlock outOfBoundsBlock = m_out.newBlock();
LBasicBlock loadPropertyNameBlock = m_out.newBlock();
continuation = m_out.newBlock();
+ operationBlock = m_out.newBlock();
- if (seenModes.contains(JSPropertyNameEnumerator::IndexedMode)) {
- checkIndex = m_out.newBlock();
- operationBlock = m_out.newBlock();
- m_out.branch(m_out.testIsZero32(mode, m_out.constInt32(JSPropertyNameEnumerator::IndexedMode)), unsure(checkIndex), unsure(operationBlock));
- }
-
+ m_out.branch(m_out.testIsZero32(mode, m_out.constInt32(JSPropertyNameEnumerator::IndexedMode)), unsure(checkIndex), unsure(operationBlock));
{
- if (checkIndex)
- m_out.appendTo(checkIndex);
+ m_out.appendTo(checkIndex);
LValue outOfBounds = m_out.aboveOrEqual(index, m_out.load32(enumerator, m_heaps.JSPropertyNameEnumerator_endGenericPropertyIndex));
m_out.branch(outOfBounds, unsure(outOfBoundsBlock), unsure(loadPropertyNameBlock));
}
@@ -13170,17 +13165,15 @@
}
}
- if (seenModes.contains(JSPropertyNameEnumerator::IndexedMode)) {
- if (operationBlock)
- m_out.appendTo(operationBlock);
- results.append(m_out.anchor(vmCall(Int64, operationEnumeratorNextUpdatePropertyName, weakPointer(globalObject), index, mode, enumerator)));
- if (continuation)
- m_out.jump(continuation);
+ if (operationBlock)
+ m_out.appendTo(operationBlock);
+ // Note: We can't omit the operation because we have no guarantee that the mode will match what we profiled.
+ results.append(m_out.anchor(vmCall(Int64, operationEnumeratorNextUpdatePropertyName, weakPointer(globalObject), index, mode, enumerator)));
+ if (continuation) {
+ m_out.jump(continuation);
+ m_out.appendTo(continuation);
}
- if (continuation)
- m_out.appendTo(continuation);
-
ASSERT(results.size());
LValue result = m_out.phi(Int64, results);
setJSValue(result);
