Title: [262995] trunk
Revision
262995
Author
rmoris...@apple.com
Date
2020-06-12 20:09:21 -0700 (Fri, 12 Jun 2020)

Log Message

The ||= operator (and similar ones) should produce valid bytecode even if the right side is a static error
https://bugs.webkit.org/show_bug.cgi?id=213154

Reviewed by Devin Rousso.

JSTests:

* stress/bytecode-for-rmw-with-invalid-right-side.js: Added.
(catch):

Source/_javascript_Core:

There were two minor issues here that interacted:
- emitThrowReferenceError did not take an optional `dst` argument like everything else, and instead always returned a new temporary.
  As a result, the various functions that sometimes did "return emitThrowReferenceError(..);" could return a different RegisterID than the one
  provided to them through `dst`, breaking the invariant stated at the top of the file.
- ShortCircuitReadModifyResolveNode::emitBytecode used the result of such a function, unnecessarily, and (correctly) relied on the invariant being upheld.
The combination of these led to the bytecode trying to do a move of a temporary that was only defined in one of the predecessors of the basic block it was on,
which was caught by validateBytecode.

I fixed both issues, and verified that either fix is enough to stop the bug.
I fixed the first because other code may depend on that invariant in more subtle ways.
I fixed the second because it was just unnecessary complexity and made the code misleading.

I also reworded the comment at the top of NodesCodegen.cpp based on Keith's explanation and Mark's advice to make it less cryptic.

* bytecompiler/NodesCodegen.cpp:
(JSC::ThrowableExpressionData::emitThrowReferenceError):
(JSC::PostfixNode::emitBytecode):
(JSC::DeleteBracketNode::emitBytecode):
(JSC::DeleteDotNode::emitBytecode):
(JSC::PrefixNode::emitBytecode):
(JSC::ShortCircuitReadModifyResolveNode::emitBytecode):
(JSC::AssignErrorNode::emitBytecode):
* parser/Nodes.h:

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (262994 => 262995)


--- trunk/JSTests/ChangeLog	2020-06-13 02:55:27 UTC (rev 262994)
+++ trunk/JSTests/ChangeLog	2020-06-13 03:09:21 UTC (rev 262995)
@@ -1,3 +1,13 @@
+2020-06-12  Robin Morisset  <rmoris...@apple.com>
+
+        The ||= operator (and similar ones) should produce valid bytecode even if the right side is a static error
+        https://bugs.webkit.org/show_bug.cgi?id=213154
+
+        Reviewed by Devin Rousso.
+
+        * stress/bytecode-for-rmw-with-invalid-right-side.js: Added.
+        (catch):
+
 2020-06-12  Yusuke Suzuki  <ysuz...@apple.com>
 
         [JSC] el(Greek) characters' upper-case conversion is locale-sensitive

Added: trunk/JSTests/stress/bytecode-for-rmw-with-invalid-right-side.js (0 => 262995)


--- trunk/JSTests/stress/bytecode-for-rmw-with-invalid-right-side.js	                        (rev 0)
+++ trunk/JSTests/stress/bytecode-for-rmw-with-invalid-right-side.js	2020-06-13 03:09:21 UTC (rev 262995)
@@ -0,0 +1,9 @@
+//@ runDefault("--validateBytecode=1")
+try {
+    x||=y()=0
+    x||=(y()++)
+    x||=(++y())
+    x||=(y()+=0)
+    x||=(y()||=0)
+} catch(e) {
+}

Modified: trunk/Source/_javascript_Core/ChangeLog (262994 => 262995)


--- trunk/Source/_javascript_Core/ChangeLog	2020-06-13 02:55:27 UTC (rev 262994)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-06-13 03:09:21 UTC (rev 262995)
@@ -1,3 +1,34 @@
+2020-06-12  Robin Morisset  <rmoris...@apple.com>
+
+        The ||= operator (and similar ones) should produce valid bytecode even if the right side is a static error
+        https://bugs.webkit.org/show_bug.cgi?id=213154
+
+        Reviewed by Devin Rousso.
+
+        There were two minor issues here that interacted:
+        - emitThrowReferenceError did not take an optional `dst` argument like everything else, and instead always returned a new temporary.
+          As a result, the various functions that sometimes did "return emitThrowReferenceError(..);" could return a different RegisterID than the one
+          provided to them through `dst`, breaking the invariant stated at the top of the file.
+        - ShortCircuitReadModifyResolveNode::emitBytecode used the result of such a function, unnecessarily, and (correctly) relied on the invariant being upheld.
+        The combination of these led to the bytecode trying to do a move of a temporary that was only defined in one of the predecessors of the basic block it was on,
+        which was caught by validateBytecode.
+
+        I fixed both issues, and verified that either fix is enough to stop the bug.
+        I fixed the first because other code may depend on that invariant in more subtle ways.
+        I fixed the second because it was just unnecessary complexity and made the code misleading.
+
+        I also reworded the comment at the top of NodesCodegen.cpp based on Keith's explanation and Mark's advice to make it less cryptic.
+
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::ThrowableExpressionData::emitThrowReferenceError):
+        (JSC::PostfixNode::emitBytecode):
+        (JSC::DeleteBracketNode::emitBytecode):
+        (JSC::DeleteDotNode::emitBytecode):
+        (JSC::PrefixNode::emitBytecode):
+        (JSC::ShortCircuitReadModifyResolveNode::emitBytecode):
+        (JSC::AssignErrorNode::emitBytecode):
+        * parser/Nodes.h:
+
 2020-06-12  Yusuke Suzuki  <ysuz...@apple.com>
 
         [JSC] el(Greek) characters' upper-case conversion is locale-sensitive

Modified: trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp (262994 => 262995)


--- trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp	2020-06-13 02:55:27 UTC (rev 262994)
+++ trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp	2020-06-13 03:09:21 UTC (rev 262995)
@@ -55,7 +55,8 @@
 
     Return value: The register holding the production's value.
              dst: An optional parameter specifying the most efficient destination at
-                  which to store the production's value. The callee must honor dst.
+                  which to store the production's value.
+                  If dst is null, you may return whatever VirtualRegister you want. Otherwise you have to return dst.
 
     The dst argument provides for a crude form of copy propagation. For example,
 
@@ -62,14 +63,14 @@
         x = 1
 
     becomes
-    
+
         load r[x], 1
-    
+
     instead of 
 
         load r0, 1
         mov r[x], r0
-    
+
     because the assignment node, "x =", passes r[x] as dst to the number node, "1".
 */
 
@@ -84,10 +85,12 @@
 
 // ------------------------------ ThrowableExpressionData --------------------------------
 
-RegisterID* ThrowableExpressionData::emitThrowReferenceError(BytecodeGenerator& generator, const String& message)
+RegisterID* ThrowableExpressionData::emitThrowReferenceError(BytecodeGenerator& generator, const String& message, RegisterID* dst)
 {
     generator.emitExpressionInfo(divot(), divotStart(), divotEnd());
     generator.emitThrowReferenceError(message);
+    if (dst)
+        return dst;
     return generator.newTemporary();
 }
 
@@ -2247,7 +2250,8 @@
     ASSERT(m_expr->isFunctionCall());
     return emitThrowReferenceError(generator, m_operator == Operator::PlusPlus
         ? "Postfix ++ operator applied to value that is not a reference."_s
-        : "Postfix -- operator applied to value that is not a reference."_s);
+        : "Postfix -- operator applied to value that is not a reference."_s,
+        dst);
 }
 
 // ------------------------------ DeleteResolveNode -----------------------------------
@@ -2279,7 +2283,7 @@
     RefPtr<RegisterID> r1 = generator.emitNode(m_subscript);
     generator.emitExpressionInfo(divot(), divotStart(), divotEnd());
     if (m_base->isSuperNode())
-        return emitThrowReferenceError(generator, "Cannot delete a super property");
+        return emitThrowReferenceError(generator, "Cannot delete a super property", dst);
     return generator.emitDeleteByVal(finalDest.get(), r0.get(), r1.get());
 }
 
@@ -2295,7 +2299,7 @@
 
     generator.emitExpressionInfo(divot(), divotStart(), divotEnd());
     if (m_base->isSuperNode())
-        return emitThrowReferenceError(generator, "Cannot delete a super property");
+        return emitThrowReferenceError(generator, "Cannot delete a super property", dst);
     return generator.emitDeleteById(finalDest.get(), r0.get(), m_ident);
 }
 
@@ -2484,7 +2488,8 @@
     ASSERT(m_expr->isFunctionCall());
     return emitThrowReferenceError(generator, m_operator == Operator::PlusPlus
         ? "Prefix ++ operator applied to value that is not a reference."_s
-        : "Prefix -- operator applied to value that is not a reference."_s);
+        : "Prefix -- operator applied to value that is not a reference."_s,
+        dst);
 }
 
 // ------------------------------ Unary Operation Nodes -----------------------------------
@@ -3199,7 +3204,7 @@
     Ref<Label> afterAssignment = generator.newLabel();
     emitShortCircuitAssignment(generator, result.get(), m_operator, afterAssignment.get());
 
-    result = generator.emitNode(result.get(), m_right); // Execute side effects first.
+    generator.emitNode(result.get(), m_right); // Execute side effects first.
 
     bool threwException = isReadOnly ? generator.emitReadOnlyExceptionIfNeeded(var) : false;
 
@@ -3352,9 +3357,9 @@
 
 // ------------------------------ AssignErrorNode -----------------------------------
 
-RegisterID* AssignErrorNode::emitBytecode(BytecodeGenerator& generator, RegisterID*)
+RegisterID* AssignErrorNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
 {
-    return emitThrowReferenceError(generator, "Left side of assignment is not a reference."_s);
+    return emitThrowReferenceError(generator, "Left side of assignment is not a reference."_s, dst);
 }
 
 // ------------------------------ AssignBracketNode -----------------------------------

Modified: trunk/Source/_javascript_Core/parser/Nodes.h (262994 => 262995)


--- trunk/Source/_javascript_Core/parser/Nodes.h	2020-06-13 02:55:27 UTC (rev 262994)
+++ trunk/Source/_javascript_Core/parser/Nodes.h	2020-06-13 03:09:21 UTC (rev 262995)
@@ -412,7 +412,7 @@
             ASSERT(m_divotEnd.offset >= m_divot.offset);
         }
     protected:
-        RegisterID* emitThrowReferenceError(BytecodeGenerator&, const String& message);
+        RegisterID* emitThrowReferenceError(BytecodeGenerator&, const String& message, RegisterID* dst = nullptr);
 
     private:
         JSTextPosition m_divot;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to