- Revision
- 240398
- Author
- [email protected]
- Date
- 2019-01-23 17:22:48 -0800 (Wed, 23 Jan 2019)
Log Message
Cherry-pick r240106. rdar://problem/47458403
[JSC] ToThis omission in DFGByteCodeParser is wrong
https://bugs.webkit.org/show_bug.cgi?id=193513
<rdar://problem/45842236>
Reviewed by Saam Barati.
JSTests:
* stress/to-this-omission-with-different-strict-modes.js: Added.
(thisA):
(thisAStrictWrapper):
Source/_javascript_Core:
DFGByteCodeParser omitted ToThis node when we have `ToThis(ToThis(value))`. This semantics is wrong if ToThis has different semantics
in the sloppy mode and the strict mode. If we convert `ToThisInSloppyMode(ToThisInStrictMode(boolean))` to `ToThisInStrictMode(boolean)`,
we get boolean instead of BooleanObject.
This optimization is introduced more than 7 years ago, and from that, we have several optimizations that can remove such ToThis nodes
in BytecodeParser, AI, and Fixup. Furthermore, this optimization is simply wrong since `toThis()` function of JSCell can be defined
as they want. Before ensuring all the toThis function is safe, we should not fold `ToThis(ToThis(value))` => `ToThis(value)`.
This patch just removes the problematic optimization. The performance numbers look neutral.
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240106 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-607-branch/JSTests/ChangeLog (240397 => 240398)
--- branches/safari-607-branch/JSTests/ChangeLog 2019-01-24 01:22:44 UTC (rev 240397)
+++ branches/safari-607-branch/JSTests/ChangeLog 2019-01-24 01:22:48 UTC (rev 240398)
@@ -1,5 +1,52 @@
2019-01-23 Alan Coon <[email protected]>
+ Cherry-pick r240106. rdar://problem/47458403
+
+ [JSC] ToThis omission in DFGByteCodeParser is wrong
+ https://bugs.webkit.org/show_bug.cgi?id=193513
+ <rdar://problem/45842236>
+
+ Reviewed by Saam Barati.
+
+ JSTests:
+
+ * stress/to-this-omission-with-different-strict-modes.js: Added.
+ (thisA):
+ (thisAStrictWrapper):
+
+ Source/_javascript_Core:
+
+ DFGByteCodeParser omitted ToThis node when we have `ToThis(ToThis(value))`. This semantics is wrong if ToThis has different semantics
+ in the sloppy mode and the strict mode. If we convert `ToThisInSloppyMode(ToThisInStrictMode(boolean))` to `ToThisInStrictMode(boolean)`,
+ we get boolean instead of BooleanObject.
+
+ This optimization is introduced more than 7 years ago, and from that, we have several optimizations that can remove such ToThis nodes
+ in BytecodeParser, AI, and Fixup. Furthermore, this optimization is simply wrong since `toThis()` function of JSCell can be defined
+ as they want. Before ensuring all the toThis function is safe, we should not fold `ToThis(ToThis(value))` => `ToThis(value)`.
+ This patch just removes the problematic optimization. The performance numbers look neutral.
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::parseBlock):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240106 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-01-17 Yusuke Suzuki <[email protected]>
+
+ [JSC] ToThis omission in DFGByteCodeParser is wrong
+ https://bugs.webkit.org/show_bug.cgi?id=193513
+ <rdar://problem/45842236>
+
+ Reviewed by Saam Barati.
+
+ * stress/to-this-omission-with-different-strict-modes.js: Added.
+ (thisA):
+ (thisAStrictWrapper):
+
+2019-01-23 Alan Coon <[email protected]>
+
Cherry-pick r240229. rdar://problem/47458326
DFG: When inlining DataView set* intrinsics we need to set undefined as our result
Added: branches/safari-607-branch/JSTests/stress/to-this-omission-with-different-strict-modes.js (0 => 240398)
--- branches/safari-607-branch/JSTests/stress/to-this-omission-with-different-strict-modes.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/to-this-omission-with-different-strict-modes.js 2019-01-24 01:22:48 UTC (rev 240398)
@@ -0,0 +1,10 @@
+function thisA() {
+ return this.a
+}
+function thisAStrictWrapper() {
+ 'use strict';
+ thisA.apply(this);
+}
+let x = false;
+for (let j=0; j<1e4; j++)
+ thisAStrictWrapper.call(x);
Modified: branches/safari-607-branch/Source/_javascript_Core/ChangeLog (240397 => 240398)
--- branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-01-24 01:22:44 UTC (rev 240397)
+++ branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-01-24 01:22:48 UTC (rev 240398)
@@ -1,5 +1,62 @@
2019-01-23 Alan Coon <[email protected]>
+ Cherry-pick r240106. rdar://problem/47458403
+
+ [JSC] ToThis omission in DFGByteCodeParser is wrong
+ https://bugs.webkit.org/show_bug.cgi?id=193513
+ <rdar://problem/45842236>
+
+ Reviewed by Saam Barati.
+
+ JSTests:
+
+ * stress/to-this-omission-with-different-strict-modes.js: Added.
+ (thisA):
+ (thisAStrictWrapper):
+
+ Source/_javascript_Core:
+
+ DFGByteCodeParser omitted ToThis node when we have `ToThis(ToThis(value))`. This semantics is wrong if ToThis has different semantics
+ in the sloppy mode and the strict mode. If we convert `ToThisInSloppyMode(ToThisInStrictMode(boolean))` to `ToThisInStrictMode(boolean)`,
+ we get boolean instead of BooleanObject.
+
+ This optimization is introduced more than 7 years ago, and from that, we have several optimizations that can remove such ToThis nodes
+ in BytecodeParser, AI, and Fixup. Furthermore, this optimization is simply wrong since `toThis()` function of JSCell can be defined
+ as they want. Before ensuring all the toThis function is safe, we should not fold `ToThis(ToThis(value))` => `ToThis(value)`.
+ This patch just removes the problematic optimization. The performance numbers look neutral.
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::parseBlock):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240106 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-01-17 Yusuke Suzuki <[email protected]>
+
+ [JSC] ToThis omission in DFGByteCodeParser is wrong
+ https://bugs.webkit.org/show_bug.cgi?id=193513
+ <rdar://problem/45842236>
+
+ Reviewed by Saam Barati.
+
+ DFGByteCodeParser omitted ToThis node when we have `ToThis(ToThis(value))`. This semantics is wrong if ToThis has different semantics
+ in the sloppy mode and the strict mode. If we convert `ToThisInSloppyMode(ToThisInStrictMode(boolean))` to `ToThisInStrictMode(boolean)`,
+ we get boolean instead of BooleanObject.
+
+ This optimization is introduced more than 7 years ago, and from that, we have several optimizations that can remove such ToThis nodes
+ in BytecodeParser, AI, and Fixup. Furthermore, this optimization is simply wrong since `toThis()` function of JSCell can be defined
+ as they want. Before ensuring all the toThis function is safe, we should not fold `ToThis(ToThis(value))` => `ToThis(value)`.
+ This patch just removes the problematic optimization. The performance numbers look neutral.
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::parseBlock):
+
+2019-01-23 Alan Coon <[email protected]>
+
Cherry-pick r240041. rdar://problem/47458234
Refactor new bytecode structs so that the fields are prefixed with "m_".
Modified: branches/safari-607-branch/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (240397 => 240398)
--- branches/safari-607-branch/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2019-01-24 01:22:44 UTC (rev 240397)
+++ branches/safari-607-branch/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2019-01-24 01:22:48 UTC (rev 240398)
@@ -2518,31 +2518,27 @@
bool strictMode = m_graph.executableFor(node->origin.semantic)->isStrictMode();
ToThisResult result = isToThisAnIdentity(m_vm, strictMode, source);
- if (result != ToThisResult::Dynamic) {
- switch (result) {
- case ToThisResult::Identity:
- m_state.setFoundConstants(true);
+ switch (result) {
+ case ToThisResult::Identity:
+ m_state.setFoundConstants(true);
+ destination = source;
+ break;
+ case ToThisResult::Undefined:
+ setConstant(node, jsUndefined());
+ break;
+ case ToThisResult::GlobalThis:
+ m_state.setFoundConstants(true);
+ destination.setType(m_graph, SpecObject);
+ break;
+ case ToThisResult::Dynamic:
+ if (strictMode)
+ destination.makeHeapTop();
+ else {
destination = source;
- break;
- case ToThisResult::Undefined:
- setConstant(node, jsUndefined());
- break;
- case ToThisResult::GlobalThis:
- m_state.setFoundConstants(true);
- destination.setType(m_graph, SpecObject);
- break;
- case ToThisResult::Dynamic:
- RELEASE_ASSERT_NOT_REACHED();
+ destination.merge(SpecObject);
}
break;
}
-
- if (strictMode)
- destination.makeHeapTop();
- else {
- destination = source;
- destination.merge(SpecObject);
- }
break;
}
Modified: branches/safari-607-branch/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (240397 => 240398)
--- branches/safari-607-branch/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-01-24 01:22:44 UTC (rev 240397)
+++ branches/safari-607-branch/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-01-24 01:22:48 UTC (rev 240398)
@@ -4746,22 +4746,20 @@
case op_to_this: {
Node* op1 = getThis();
- if (op1->op() != ToThis) {
- auto& metadata = currentInstruction->as<OpToThis>().metadata(codeBlock);
- Structure* cachedStructure = metadata.m_cachedStructure.get();
- if (metadata.m_toThisStatus != ToThisOK
- || !cachedStructure
- || cachedStructure->classInfo()->methodTable.toThis != JSObject::info()->methodTable.toThis
- || m_inlineStackTop->m_profiledBlock->couldTakeSlowCase(m_currentIndex)
- || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)
- || (op1->op() == GetLocal && op1->variableAccessData()->structureCheckHoistingFailed())) {
- setThis(addToGraph(ToThis, OpInfo(), OpInfo(getPrediction()), op1));
- } else {
- addToGraph(
- CheckStructure,
- OpInfo(m_graph.addStructureSet(cachedStructure)),
- op1);
- }
+ auto& metadata = currentInstruction->as<OpToThis>().metadata(codeBlock);
+ Structure* cachedStructure = metadata.m_cachedStructure.get();
+ if (metadata.m_toThisStatus != ToThisOK
+ || !cachedStructure
+ || cachedStructure->classInfo()->methodTable.toThis != JSObject::info()->methodTable.toThis
+ || m_inlineStackTop->m_profiledBlock->couldTakeSlowCase(m_currentIndex)
+ || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)
+ || (op1->op() == GetLocal && op1->variableAccessData()->structureCheckHoistingFailed())) {
+ setThis(addToGraph(ToThis, OpInfo(), OpInfo(getPrediction()), op1));
+ } else {
+ addToGraph(
+ CheckStructure,
+ OpInfo(m_graph.addStructureSet(cachedStructure)),
+ op1);
}
NEXT_OPCODE(op_to_this);
}
