Title: [257034] trunk
Revision
257034
Author
ross.kirsl...@sony.com
Date
2020-02-19 19:01:03 -0800 (Wed, 19 Feb 2020)

Log Message

Computed Properties with increment sometimes produces incorrect results
https://bugs.webkit.org/show_bug.cgi?id=170934

Reviewed by Yusuke Suzuki.

JSTests:

* stress/computed-property-increment.js: Added.
* test262/expectations.yaml: Mark two test cases as passing.

Source/_javascript_Core:

When the key and value of a computed property each have side effects, the eval order should be key-before-value.
Not only have we had this backwards, we've also been giving them both the same target register.

* bytecompiler/NodesCodegen.cpp:
(JSC::PropertyListNode::emitPutConstantProperty):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (257033 => 257034)


--- trunk/JSTests/ChangeLog	2020-02-20 03:00:03 UTC (rev 257033)
+++ trunk/JSTests/ChangeLog	2020-02-20 03:01:03 UTC (rev 257034)
@@ -1,3 +1,13 @@
+2020-02-19  Ross Kirsling  <ross.kirsl...@sony.com>
+
+        Computed Properties with increment sometimes produces incorrect results
+        https://bugs.webkit.org/show_bug.cgi?id=170934
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/computed-property-increment.js: Added.
+        * test262/expectations.yaml: Mark two test cases as passing.
+
 2020-02-19  Keith Miller  <keith_mil...@apple.com>
 
         Disable Wasm reference types by default

Added: trunk/JSTests/stress/computed-property-increment.js (0 => 257034)


--- trunk/JSTests/stress/computed-property-increment.js	                        (rev 0)
+++ trunk/JSTests/stress/computed-property-increment.js	2020-02-20 03:01:03 UTC (rev 257034)
@@ -0,0 +1,18 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error(`expected ${expected} but got ${actual}`);
+}
+
+function test() {
+    let c = 0;
+    shouldBe(`${Object.entries({ [++c]: ++c })}`, '1,2');
+
+    {
+        let c = 0;
+        shouldBe(`${Object.entries({ [++c]: ++c })}`, '1,2');
+    }
+}
+noInline(test);
+
+for (let i = 0; i < 10000; i++)
+  test();

Modified: trunk/JSTests/test262/expectations.yaml (257033 => 257034)


--- trunk/JSTests/test262/expectations.yaml	2020-02-20 03:00:03 UTC (rev 257033)
+++ trunk/JSTests/test262/expectations.yaml	2020-02-20 03:01:03 UTC (rev 257034)
@@ -2808,9 +2808,6 @@
 test/language/expressions/object/__proto__-permitted-dup.js:
   default: 'SyntaxError: Attempted to redefine __proto__ property.'
   strict mode: 'SyntaxError: Attempted to redefine __proto__ property.'
-test/language/expressions/object/computed-property-evaluation-order.js:
-  default: 'Test262Error: Expected SameValue(«2», «1») to be true'
-  strict mode: 'Test262Error: Expected SameValue(«2», «1») to be true'
 test/language/expressions/object/covered-ident-name-prop-name-literal-break-escaped.js:
   default: "SyntaxError: Unexpected escaped characters in keyword token: 'bre\\u0061k'"
   strict mode: "SyntaxError: Unexpected escaped characters in keyword token: 'bre\\u0061k'"

Modified: trunk/Source/_javascript_Core/ChangeLog (257033 => 257034)


--- trunk/Source/_javascript_Core/ChangeLog	2020-02-20 03:00:03 UTC (rev 257033)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-02-20 03:01:03 UTC (rev 257034)
@@ -1,3 +1,16 @@
+2020-02-19  Ross Kirsling  <ross.kirsl...@sony.com>
+
+        Computed Properties with increment sometimes produces incorrect results
+        https://bugs.webkit.org/show_bug.cgi?id=170934
+
+        Reviewed by Yusuke Suzuki.
+
+        When the key and value of a computed property each have side effects, the eval order should be key-before-value.
+        Not only have we had this backwards, we've also been giving them both the same target register.
+
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::PropertyListNode::emitPutConstantProperty):
+
 2020-02-19  Keith Miller  <keith_mil...@apple.com>
 
         Disable Wasm reference types by default

Modified: trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp (257033 => 257034)


--- trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp	2020-02-20 03:00:03 UTC (rev 257033)
+++ trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp	2020-02-20 03:01:03 UTC (rev 257034)
@@ -713,6 +713,17 @@
 
 void PropertyListNode::emitPutConstantProperty(BytecodeGenerator& generator, RegisterID* newObj, PropertyNode& node)
 {
+    bool shouldSetFunctionName = generator.shouldSetFunctionName(node.m_assign);
+
+    RefPtr<RegisterID> propertyName;
+    if (!node.name()) {
+        propertyName = generator.newTemporary();
+        if (shouldSetFunctionName)
+            generator.emitToPropertyKey(propertyName.get(), generator.emitNode(node.m_expression));
+        else
+            generator.emitNode(propertyName.get(), node.m_expression);
+    }
+
     RefPtr<RegisterID> value = generator.emitNode(node.m_assign);
     if (node.needsSuperBinding())
         emitPutHomeObject(generator, value.get(), newObj);
@@ -719,20 +730,17 @@
 
     if (node.isClassProperty()) {
         ASSERT(node.needsSuperBinding());
-        RefPtr<RegisterID> propertyNameRegister;
         if (node.name())
-            propertyNameRegister = generator.emitLoad(nullptr, *node.name());
-        else
-            propertyNameRegister = generator.emitNode(node.m_expression);
+            propertyName = generator.emitLoad(nullptr, *node.name());
 
-        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);
+        if (shouldSetFunctionName)
+            generator.emitSetFunctionName(value.get(), propertyName.get());
+        generator.emitCallDefineProperty(newObj, propertyName.get(), value.get(), nullptr, nullptr, BytecodeGenerator::PropertyConfigurable | BytecodeGenerator::PropertyWritable, m_position);
         return;
     }
+
     if (const auto* identifier = node.name()) {
+        ASSERT(!propertyName);
         Optional<uint32_t> optionalIndex = parseIndex(*identifier);
         if (!optionalIndex) {
             generator.emitDirectPutById(newObj, *identifier, value.get(), node.putType());
@@ -739,15 +747,13 @@
             return;
         }
 
-        RefPtr<RegisterID> index = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));
-        generator.emitDirectPutByVal(newObj, index.get(), value.get());
+        propertyName = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));
+        generator.emitDirectPutByVal(newObj, propertyName.get(), value.get());
         return;
     }
-    RefPtr<RegisterID> propertyName = generator.emitNode(node.m_expression);
-    if (generator.shouldSetFunctionName(node.m_assign)) {
-        propertyName = generator.emitToPropertyKey(generator.newTemporary(), propertyName.get());
+
+    if (shouldSetFunctionName)
         generator.emitSetFunctionName(value.get(), propertyName.get());
-    }
     generator.emitDirectPutByVal(newObj, propertyName.get(), value.get());
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to