Diff
Modified: trunk/JSTests/ChangeLog (256845 => 256846)
--- trunk/JSTests/ChangeLog 2020-02-18 19:55:39 UTC (rev 256845)
+++ trunk/JSTests/ChangeLog 2020-02-18 20:01:20 UTC (rev 256846)
@@ -1,3 +1,13 @@
+2020-02-18 Ross Kirsling <ross.kirsl...@sony.com>
+
+ [JSC] Computed function properties compute their keys twice
+ https://bugs.webkit.org/show_bug.cgi?id=207297
+
+ Reviewed by Keith Miller.
+
+ * stress/computed-property-key-side-effects.js: Added.
+ * test262/expectations.yaml: Mark 6 test cases as passing.
+
2020-02-17 Yusuke Suzuki <ysuz...@apple.com>
[JSC] JITThunk should be HashSet<Weak<NativeExecutable>> with appropriate GC weakness handling
Added: trunk/JSTests/stress/computed-property-key-side-effects.js (0 => 256846)
--- trunk/JSTests/stress/computed-property-key-side-effects.js (rev 0)
+++ trunk/JSTests/stress/computed-property-key-side-effects.js 2020-02-18 20:01:20 UTC (rev 256846)
@@ -0,0 +1,38 @@
+function shouldBe(actual, expected) {
+ if (actual !== expected)
+ throw new Error(`expected ${expected} but got ${actual}`);
+}
+
+let count;
+const key1 = { toString() { count++; return 'foo'; } };
+const key2 = { toString: null, valueOf() { count++; return 'foo'; } };
+
+function test() {
+ count = 0;
+
+ ({ [key1]() { return 'bar'; } });
+ shouldBe(count, 1);
+ ({ [key1]: function () { return 'bar'; } });
+ shouldBe(count, 2);
+ ({ [key1]: () => 'bar' });
+ shouldBe(count, 3);
+ ({ get [key1]() { return 'bar'; } });
+ shouldBe(count, 4);
+ ({ set [key1](_) {} });
+ shouldBe(count, 5);
+
+ ({ [key2]() { return 'bar'; } });
+ shouldBe(count, 6);
+ ({ [key2]: function () { return 'bar'; } });
+ shouldBe(count, 7);
+ ({ [key2]: () => 'bar' });
+ shouldBe(count, 8);
+ ({ get [key2]() { return 'bar'; } });
+ shouldBe(count, 9);
+ ({ set [key2](_) {} });
+ shouldBe(count, 10);
+}
+noInline(test);
+
+for (let i = 0; i < 1e5; i++)
+ test();
Modified: trunk/JSTests/test262/expectations.yaml (256845 => 256846)
--- trunk/JSTests/test262/expectations.yaml 2020-02-18 19:55:39 UTC (rev 256845)
+++ trunk/JSTests/test262/expectations.yaml 2020-02-18 20:01:20 UTC (rev 256846)
@@ -2203,15 +2203,6 @@
default: 'Test262: This statement should not be evaluated.'
test/language/block-scope/syntax/redeclaration/var-redeclaration-attempt-after-generator.js:
default: 'Test262: This statement should not be evaluated.'
-test/language/computed-property-names/to-name-side-effects/class.js:
- default: 'Test262Error: Expected [0, 1] and [0] to have the same contents. order set for key1'
- strict mode: 'Test262Error: Expected [0, 1] and [0] to have the same contents. order set for key1'
-test/language/computed-property-names/to-name-side-effects/numbers-class.js:
- default: 'Test262Error: Expected [0, 1] and [0] to have the same contents. order set for key1'
- strict mode: 'Test262Error: Expected [0, 1] and [0] to have the same contents. order set for key1'
-test/language/computed-property-names/to-name-side-effects/object.js:
- default: 'Test262Error: The result of `counter++` is `0` Expected SameValue(«1», «0») to be true'
- strict mode: 'Test262Error: The result of `counter++` is `0` Expected SameValue(«1», «0») to be true'
test/language/eval-code/direct/new.target-arrow.js:
default: 'Test262Error: Expected SameValue(«function ReferenceError() {'
strict mode: 'Test262Error: Expected SameValue(«function ReferenceError() {'
Modified: trunk/Source/_javascript_Core/ChangeLog (256845 => 256846)
--- trunk/Source/_javascript_Core/ChangeLog 2020-02-18 19:55:39 UTC (rev 256845)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-02-18 20:01:20 UTC (rev 256846)
@@ -1,3 +1,37 @@
+2020-02-18 Ross Kirsling <ross.kirsl...@sony.com>
+
+ [JSC] Computed function properties compute their keys twice
+ https://bugs.webkit.org/show_bug.cgi?id=207297
+
+ Reviewed by Keith Miller.
+
+ If a pseudo-String is used as the key of a computed function property,
+ any side effects from resolving the string value occur in duplicate.
+
+ The cause has two parts:
+ - We aren't ensuring that the string value is resolved before doing SetFunctionName and PutByVal.
+ - Our implementation of SetFunctionName (https://tc39.es/ecma262/#sec-setfunctionname)
+ calls toString on a non-symbol argument, instead of assuming the type is a string.
+
+ * bytecompiler/BytecodeGenerator.cpp:
+ (JSC::BytecodeGenerator::shouldSetFunctionName): Added.
+ (JSC::BytecodeGenerator::emitSetFunctionName): Added.
+ (JSC::BytecodeGenerator::emitSetFunctionNameIfNeededImpl): Deleted.
+ (JSC::BytecodeGenerator::emitSetFunctionNameIfNeeded): Deleted.
+ * bytecompiler/BytecodeGenerator.h:
+ Split the "if needed" logic out into its own function.
+
+ * bytecompiler/NodesCodegen.cpp:
+ (JSC::PropertyListNode::emitBytecode):
+ (JSC::PropertyListNode::emitPutConstantProperty):
+ (JSC::DefineFieldNode::emitBytecode):
+ Never emit OpSetFunctionName for a name of unknown type.
+ (But also, don't perform a needless ToPropertyKey for non-function computed property keys.)
+
+ * runtime/JSFunction.cpp:
+ (JSC::JSFunction::setFunctionName):
+ Don't call toString, assert isString.
+
2020-02-17 Yusuke Suzuki <ysuz...@apple.com>
[JSC] JITThunk should be HashSet<Weak<NativeExecutable>> with appropriate GC weakness handling
Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp (256845 => 256846)
--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp 2020-02-18 19:55:39 UTC (rev 256845)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp 2020-02-18 20:01:20 UTC (rev 256846)
@@ -3076,39 +3076,40 @@
return dst;
}
-template<typename LazyNameRegisterFn>
-void BytecodeGenerator::emitSetFunctionNameIfNeededImpl(ExpressionNode* valueNode, RegisterID* value, const LazyNameRegisterFn& lazyNameFn)
+bool BytecodeGenerator::shouldSetFunctionName(ExpressionNode* node)
{
- if (valueNode->isBaseFuncExprNode()) {
- FunctionMetadataNode* metadata = static_cast<BaseFuncExprNode*>(valueNode)->metadata();
+ if (node->isBaseFuncExprNode()) {
+ FunctionMetadataNode* metadata = static_cast<BaseFuncExprNode*>(node)->metadata();
if (!metadata->ecmaName().isNull())
- return;
- } else if (valueNode->isClassExprNode()) {
- ClassExprNode* classExprNode = static_cast<ClassExprNode*>(valueNode);
+ return false;
+ } else if (node->isClassExprNode()) {
+ ClassExprNode* classExprNode = static_cast<ClassExprNode*>(node);
if (!classExprNode->ecmaName().isNull())
- return;
+ return false;
if (classExprNode->hasStaticProperty(m_vm.propertyNames->name))
- return;
+ return false;
} else
- return;
+ return false;
- RegisterID* name = lazyNameFn();
+ return true;
+}
+void BytecodeGenerator::emitSetFunctionName(RegisterID* value, const Identifier& ident)
+{
+ RefPtr<RegisterID> name = emitLoad(newTemporary(), ident);
+
// FIXME: We should use an op_call to an internal function here instead.
// https://bugs.webkit.org/show_bug.cgi?id=155547
OpSetFunctionName::emit(this, value, name);
}
-void BytecodeGenerator::emitSetFunctionNameIfNeeded(ExpressionNode* valueNode, RegisterID* value, const Identifier& ident)
+void BytecodeGenerator::emitSetFunctionName(RegisterID* value, RegisterID* name)
{
- emitSetFunctionNameIfNeededImpl(valueNode, value, [=]() { return emitLoad(newTemporary(), ident); });
+ // FIXME: We should use an op_call to an internal function here instead.
+ // https://bugs.webkit.org/show_bug.cgi?id=155547
+ OpSetFunctionName::emit(this, value, name);
}
-void BytecodeGenerator::emitSetFunctionNameIfNeeded(ExpressionNode* valueNode, RegisterID* value, RegisterID* name)
-{
- emitSetFunctionNameIfNeededImpl(valueNode, value, [=]() { return name; });
-}
-
RegisterID* BytecodeGenerator::emitCall(RegisterID* dst, RegisterID* func, ExpectedFunction expectedFunction, CallArguments& callArguments, const JSTextPosition& divot, const JSTextPosition& divotStart, const JSTextPosition& divotEnd, DebuggableCall debuggableCall)
{
return emitCall<OpCall>(dst, func, expectedFunction, callArguments, divot, divotStart, divotEnd, debuggableCall);
Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h (256845 => 256846)
--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h 2020-02-18 19:55:39 UTC (rev 256845)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h 2020-02-18 20:01:20 UTC (rev 256846)
@@ -753,8 +753,9 @@
RegisterID* emitNewMethodDefinition(RegisterID* dst, MethodDefinitionNode*);
RegisterID* emitNewRegExp(RegisterID* dst, RegExp*);
- void emitSetFunctionNameIfNeeded(ExpressionNode* valueNode, RegisterID* value, RegisterID* name);
- void emitSetFunctionNameIfNeeded(ExpressionNode* valueNode, RegisterID* value, const Identifier&);
+ bool shouldSetFunctionName(ExpressionNode*);
+ void emitSetFunctionName(RegisterID* value, RegisterID* name);
+ void emitSetFunctionName(RegisterID* value, const Identifier&);
RegisterID* moveLinkTimeConstant(RegisterID* dst, LinkTimeConstant);
RegisterID* moveEmptyValue(RegisterID* dst);
@@ -979,9 +980,6 @@
bool emitReturnViaFinallyIfNeeded(RegisterID* returnRegister);
void emitFinallyCompletion(FinallyContext&, Label& normalCompletionLabel);
- template<typename LazyNameRegisterFn>
- void emitSetFunctionNameIfNeededImpl(ExpressionNode*, RegisterID*, const LazyNameRegisterFn&);
-
public:
void pushFinallyControlFlowScope(FinallyContext&);
void popFinallyControlFlowScope();
Modified: trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp (256845 => 256846)
--- trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp 2020-02-18 19:55:39 UTC (rev 256845)
+++ trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp 2020-02-18 20:01:20 UTC (rev 256846)
@@ -644,7 +644,10 @@
// Computed accessors.
if (node->m_type & PropertyNode::Computed) {
RefPtr<RegisterID> propertyName = generator.emitNode(node->m_expression);
- generator.emitSetFunctionNameIfNeeded(node->m_assign, value.get(), propertyName.get());
+ if (generator.shouldSetFunctionName(node->m_assign)) {
+ propertyName = generator.emitToPropertyKey(generator.newTemporary(), propertyName.get());
+ generator.emitSetFunctionName(value.get(), propertyName.get());
+ }
if (node->m_type & PropertyNode::Getter)
generator.emitPutGetterByVal(dst, propertyName.get(), attributes, value.get());
else
@@ -722,7 +725,10 @@
else
propertyNameRegister = generator.emitNode(node.m_expression);
- generator.emitSetFunctionNameIfNeeded(node.m_assign, value.get(), propertyNameRegister.get());
+ if (generator.shouldSetFunctionName(node.m_assign)) {
+ propertyNameRegister = generator.emitToPropertyKey(generator.newTemporary(), propertyNameRegister.get());
+ generator.emitSetFunctionName(value.get(), propertyNameRegister.get());
+ }
generator.emitCallDefineProperty(newObj, propertyNameRegister.get(), value.get(), nullptr, nullptr, BytecodeGenerator::PropertyConfigurable | BytecodeGenerator::PropertyWritable, m_position);
return;
}
@@ -738,7 +744,10 @@
return;
}
RefPtr<RegisterID> propertyName = generator.emitNode(node.m_expression);
- generator.emitSetFunctionNameIfNeeded(node.m_assign, value.get(), propertyName.get());
+ if (generator.shouldSetFunctionName(node.m_assign)) {
+ propertyName = generator.emitToPropertyKey(generator.newTemporary(), propertyName.get());
+ generator.emitSetFunctionName(value.get(), propertyName.get());
+ }
generator.emitDirectPutByVal(newObj, propertyName.get(), value.get());
}
@@ -4325,8 +4334,8 @@
generator.emitLoad(value.get(), jsUndefined());
else {
generator.emitNode(value.get(), m_assign);
- if (m_ident)
- generator.emitSetFunctionNameIfNeeded(m_assign, value.get(), *m_ident);
+ if (m_ident && generator.shouldSetFunctionName(m_assign))
+ generator.emitSetFunctionName(value.get(), *m_ident);
}
switch (m_type) {
Modified: trunk/Source/_javascript_Core/runtime/JSFunction.cpp (256845 => 256846)
--- trunk/Source/_javascript_Core/runtime/JSFunction.cpp 2020-02-18 19:55:39 UTC (rev 256845)
+++ trunk/Source/_javascript_Core/runtime/JSFunction.cpp 2020-02-18 20:01:20 UTC (rev 256846)
@@ -766,10 +766,9 @@
else
name = makeString('[', String(&uid), ']');
} else {
- JSString* jsStr = value.toString(globalObject);
+ ASSERT(value.isString());
+ name = asString(value)->value(globalObject);
RETURN_IF_EXCEPTION(scope, void());
- name = jsStr->value(globalObject);
- RETURN_IF_EXCEPTION(scope, void());
}
reifyName(vm, globalObject, name);
}
