- Revision
- 226907
- Author
- sbar...@apple.com
- Date
- 2018-01-12 12:47:44 -0800 (Fri, 12 Jan 2018)
Log Message
CheckStructure can be incorrectly subsumed by CheckStructureOrEmpty
https://bugs.webkit.org/show_bug.cgi?id=181177
<rdar://problem/36205704>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/check-structure-ir-ensures-empty-does-not-flow-through.js: Added.
(runNearStackLimit.t):
(runNearStackLimit):
(test.f):
(test):
Source/_javascript_Core:
The semantics of CheckStructure are such that it does not allow the empty value to flow through it.
However, we may eliminate a CheckStructure if it's preceded by a CheckStructureOrEmpty. This doesn't
have semantic consequences when validation is turned off. However, with validation on, this trips up
our OSR exit machinery that says when an exit is allowed to happen.
Consider the following IR:
a: GetClosureVar // Or any other node that produces BytecodeTop
...
c: CheckStructure(Cell:@a, {s2})
d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
In the TypeCheckHoistingPhase, we may insert CheckStructureOrEmptys like this:
a: GetClosureVar
e: CheckStructureOrEmpty(@a, {s1})
...
f: CheckStructureOrEmpty(@a, {s2})
c: CheckStructure(Cell:@a, {s2})
d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
This will cause constant folding to change the IR to:
a: GetClosureVar
e: CheckStructureOrEmpty(@a, {s1})
...
f: CheckStructureOrEmpty(@a, {s2})
d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
Our mayExit analysis determines that the PutByOffset should not exit. Note
that AI will determine the only value the PutByOffset can see in @a is
the empty value. Because KnownCell filters SpecCell and not SpecCellCheck,
when lowering the PutByOffset, we reach a contradiction in AI and emit
an OSR exit. However, because mayExit said we couldn't exit, we assert.
Note that if we did not run the TypeCheckHoistingPhase on this IR, AI
would have determined we would OSR exit at the second CheckStructure.
This patch makes it so constant folding produces the following IR:
a: GetClosureVar
e: CheckStructureOrEmpty(@a, {s1})
g: AssertNotEmpty(@a)
...
f: CheckStructureOrEmpty(@a, {s2})
h: AssertNotEmpty(@a)
d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
This modification will cause AI to know we will OSR exit before even reaching
the PutByOffset. Note that in the original IR, the GetClosureVar won't
actually produce the TDZ value. If it did, bytecode would have caused us
to emit a CheckNotEmpty before the CheckStructure/PutByOffset combo. That's
why this bug is about IR bookkeeping and not an actual error in IR analysis.
This patch introduces AssertNotEmpty instead of using CheckNotEmpty to be
more congruous with CheckStructure's semantics of crashing on the empty value
as input (on 64 bit platforms).
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGNodeType.h:
* dfg/DFGPredictionPropagationPhase.cpp:
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileNode):
(JSC::FTL::DFG::LowerDFGToB3::compileAssertNotEmpty):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (226906 => 226907)
--- trunk/JSTests/ChangeLog 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/JSTests/ChangeLog 2018-01-12 20:47:44 UTC (rev 226907)
@@ -1,5 +1,19 @@
2018-01-12 Saam Barati <sbar...@apple.com>
+ CheckStructure can be incorrectly subsumed by CheckStructureOrEmpty
+ https://bugs.webkit.org/show_bug.cgi?id=181177
+ <rdar://problem/36205704>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/check-structure-ir-ensures-empty-does-not-flow-through.js: Added.
+ (runNearStackLimit.t):
+ (runNearStackLimit):
+ (test.f):
+ (test):
+
+2018-01-12 Saam Barati <sbar...@apple.com>
+
Each variant of a polymorphic inlined call should be exitOK at the top of the block
https://bugs.webkit.org/show_bug.cgi?id=181562
<rdar://problem/36445624>
Added: trunk/JSTests/stress/check-structure-ir-ensures-empty-does-not-flow-through.js (0 => 226907)
--- trunk/JSTests/stress/check-structure-ir-ensures-empty-does-not-flow-through.js (rev 0)
+++ trunk/JSTests/stress/check-structure-ir-ensures-empty-does-not-flow-through.js 2018-01-12 20:47:44 UTC (rev 226907)
@@ -0,0 +1,26 @@
+function runNearStackLimit(f) {
+ function t() {
+ try {
+ return t();
+ } catch (e) {
+ return f();
+ }
+ }
+ return t()
+}
+
+function test() {
+ function f(arg) {
+ let loc = arg;
+ try {
+ loc.p = 0;
+ } catch (e) {}
+ arg.p = 1;
+ }
+ let obj = {};
+ runNearStackLimit(() => {
+ return f(obj);
+ });
+}
+for (let i = 0; i < 50; ++i)
+ test();
Modified: trunk/Source/_javascript_Core/ChangeLog (226906 => 226907)
--- trunk/Source/_javascript_Core/ChangeLog 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-01-12 20:47:44 UTC (rev 226907)
@@ -1,3 +1,89 @@
+2018-01-12 Saam Barati <sbar...@apple.com>
+
+ CheckStructure can be incorrectly subsumed by CheckStructureOrEmpty
+ https://bugs.webkit.org/show_bug.cgi?id=181177
+ <rdar://problem/36205704>
+
+ Reviewed by Yusuke Suzuki.
+
+ The semantics of CheckStructure are such that it does not allow the empty value to flow through it.
+ However, we may eliminate a CheckStructure if it's preceded by a CheckStructureOrEmpty. This doesn't
+ have semantic consequences when validation is turned off. However, with validation on, this trips up
+ our OSR exit machinery that says when an exit is allowed to happen.
+
+ Consider the following IR:
+
+ a: GetClosureVar // Or any other node that produces BytecodeTop
+ ...
+ c: CheckStructure(Cell:@a, {s2})
+ d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
+
+ In the TypeCheckHoistingPhase, we may insert CheckStructureOrEmptys like this:
+ a: GetClosureVar
+ e: CheckStructureOrEmpty(@a, {s1})
+ ...
+ f: CheckStructureOrEmpty(@a, {s2})
+ c: CheckStructure(Cell:@a, {s2})
+ d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
+
+ This will cause constant folding to change the IR to:
+ a: GetClosureVar
+ e: CheckStructureOrEmpty(@a, {s1})
+ ...
+ f: CheckStructureOrEmpty(@a, {s2})
+ d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
+
+ Our mayExit analysis determines that the PutByOffset should not exit. Note
+ that AI will determine the only value the PutByOffset can see in @a is
+ the empty value. Because KnownCell filters SpecCell and not SpecCellCheck,
+ when lowering the PutByOffset, we reach a contradiction in AI and emit
+ an OSR exit. However, because mayExit said we couldn't exit, we assert.
+
+ Note that if we did not run the TypeCheckHoistingPhase on this IR, AI
+ would have determined we would OSR exit at the second CheckStructure.
+
+ This patch makes it so constant folding produces the following IR:
+ a: GetClosureVar
+ e: CheckStructureOrEmpty(@a, {s1})
+ g: AssertNotEmpty(@a)
+ ...
+ f: CheckStructureOrEmpty(@a, {s2})
+ h: AssertNotEmpty(@a)
+ d: PutByOffset(KnownCell:@a, KnownCell:@a, @value)
+
+ This modification will cause AI to know we will OSR exit before even reaching
+ the PutByOffset. Note that in the original IR, the GetClosureVar won't
+ actually produce the TDZ value. If it did, bytecode would have caused us
+ to emit a CheckNotEmpty before the CheckStructure/PutByOffset combo. That's
+ why this bug is about IR bookkeeping and not an actual error in IR analysis.
+ This patch introduces AssertNotEmpty instead of using CheckNotEmpty to be
+ more congruous with CheckStructure's semantics of crashing on the empty value
+ as input (on 64 bit platforms).
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGClobberize.h:
+ (JSC::DFG::clobberize):
+ * dfg/DFGConstantFoldingPhase.cpp:
+ (JSC::DFG::ConstantFoldingPhase::foldConstants):
+ * dfg/DFGDoesGC.cpp:
+ (JSC::DFG::doesGC):
+ * dfg/DFGFixupPhase.cpp:
+ (JSC::DFG::FixupPhase::fixupNode):
+ * dfg/DFGNodeType.h:
+ * dfg/DFGPredictionPropagationPhase.cpp:
+ * dfg/DFGSafeToExecute.h:
+ (JSC::DFG::safeToExecute):
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * ftl/FTLCapabilities.cpp:
+ (JSC::FTL::canCompile):
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileNode):
+ (JSC::FTL::DFG::LowerDFGToB3::compileAssertNotEmpty):
+
2018-01-12 Joseph Pecoraro <pecor...@apple.com>
Web Inspector: Remove unnecessary raw pointer in InspectorConsoleAgent
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2018-01-12 20:47:44 UTC (rev 226907)
@@ -3001,6 +3001,7 @@
break;
}
+ case AssertNotEmpty:
case CheckNotEmpty: {
AbstractValue& value = forNode(node->child1());
if (!(value.m_type & SpecEmpty)) {
Modified: trunk/Source/_javascript_Core/dfg/DFGClobberize.h (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2018-01-12 20:47:44 UTC (rev 226907)
@@ -417,6 +417,10 @@
def(PureValue(CheckNotEmpty, AdjacencyList(AdjacencyList::Fixed, node->child1())));
return;
+ case AssertNotEmpty:
+ write(SideState);
+ return;
+
case CheckStringIdent:
def(PureValue(CheckStringIdent, AdjacencyList(AdjacencyList::Fixed, node->child1()), node->uidOperand()));
return;
Modified: trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -175,8 +175,13 @@
RegisteredStructureSet set;
if (node->op() == ArrayifyToStructure)
set = node->structure();
- else
+ else {
set = node->structureSet();
+ if ((SpecCellCheck & SpecEmpty) && node->child1().useKind() == CellUse && m_state.forNode(node->child1()).m_type & SpecEmpty) {
+ m_insertionSet.insertNode(
+ indexInBlock, SpecNone, AssertNotEmpty, node->origin, Edge(node->child1().node(), UntypedUse));
+ }
+ }
if (value.m_structure.isSubsetOf(set)) {
m_interpreter.execute(indexInBlock); // Catch the fact that we may filter on cell.
node->remove();
@@ -293,6 +298,7 @@
break;
}
+ case AssertNotEmpty:
case CheckNotEmpty: {
if (m_state.forNode(node->child1()).m_type & SpecEmpty)
break;
Modified: trunk/Source/_javascript_Core/dfg/DFGDoesGC.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGDoesGC.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGDoesGC.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -134,6 +134,7 @@
case PutGlobalVariable:
case CheckCell:
case CheckNotEmpty:
+ case AssertNotEmpty:
case CheckStringIdent:
case RegExpExec:
case RegExpTest:
Modified: trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -2145,6 +2145,7 @@
case ForceOSRExit:
case CheckBadCell:
case CheckNotEmpty:
+ case AssertNotEmpty:
case CheckTraps:
case Unreachable:
case ExtractOSREntryLocal:
Modified: trunk/Source/_javascript_Core/dfg/DFGNodeType.h (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGNodeType.h 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGNodeType.h 2018-01-12 20:47:44 UTC (rev 226907)
@@ -239,6 +239,7 @@
macro(RecordRegExpCachedResult, NodeMustGenerate | NodeHasVarArgs) \
macro(CheckCell, NodeMustGenerate) \
macro(CheckNotEmpty, NodeMustGenerate) \
+ macro(AssertNotEmpty, NodeMustGenerate) \
macro(CheckBadCell, NodeMustGenerate) \
macro(CheckInBounds, NodeMustGenerate) \
macro(CheckStringIdent, NodeMustGenerate) \
Modified: trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -1163,6 +1163,7 @@
case CheckStructure:
case CheckCell:
case CheckNotEmpty:
+ case AssertNotEmpty:
case CheckStringIdent:
case CheckBadCell:
case PutStructure:
Modified: trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h 2018-01-12 20:47:44 UTC (rev 226907)
@@ -258,6 +258,7 @@
case CheckCell:
case CheckBadCell:
case CheckNotEmpty:
+ case AssertNotEmpty:
case CheckStringIdent:
case RegExpExec:
case RegExpTest:
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -5186,6 +5186,7 @@
case InitializeEntrypointArguments:
case EntrySwitch:
case CPUIntrinsic:
+ case AssertNotEmpty:
DFG_CRASH(m_jit.graph(), node, "unexpected node in DFG backend");
break;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -4220,6 +4220,18 @@
break;
}
+ case AssertNotEmpty: {
+ if (validationEnabled()) {
+ JSValueOperand operand(this, node->child1());
+ GPRReg input = operand.gpr();
+ auto done = m_jit.branchTest64(MacroAssembler::NonZero, input);
+ m_jit.breakpoint();
+ done.link(&m_jit);
+ }
+ noResult(node);
+ break;
+ }
+
case CheckStringIdent:
compileCheckStringIdent(node);
break;
Modified: trunk/Source/_javascript_Core/ftl/FTLCapabilities.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/ftl/FTLCapabilities.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/ftl/FTLCapabilities.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -136,6 +136,7 @@
case CheckCell:
case CheckBadCell:
case CheckNotEmpty:
+ case AssertNotEmpty:
case CheckStringIdent:
case CheckTraps:
case StringCharCodeAt:
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (226906 => 226907)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-01-12 20:41:55 UTC (rev 226906)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-01-12 20:47:44 UTC (rev 226907)
@@ -670,6 +670,9 @@
case CheckNotEmpty:
compileCheckNotEmpty();
break;
+ case AssertNotEmpty:
+ compileAssertNotEmpty();
+ break;
case CheckBadCell:
compileCheckBadCell();
break;
@@ -2895,6 +2898,23 @@
speculate(TDZFailure, noValue(), nullptr, m_out.isZero64(lowJSValue(m_node->child1())));
}
+ void compileAssertNotEmpty()
+ {
+ if (!validationEnabled())
+ return;
+
+ B3::PatchpointValue* patchpoint = m_out.patchpoint(Void);
+ patchpoint->appendSomeRegister(lowJSValue(m_node->child1()));
+ patchpoint->setGenerator(
+ [=] (CCallHelpers& jit, const StackmapGenerationParams& params) {
+ AllowMacroScratchRegisterUsage allowScratch(jit);
+ GPRReg input = params[0].gpr();
+ CCallHelpers::Jump done = jit.branchTest64(CCallHelpers::NonZero, input);
+ jit.breakpoint();
+ done.link(&jit);
+ });
+ }
+
void compileCheckStringIdent()
{
UniquedStringImpl* uid = m_node->uidOperand();
