Changeset 262995 in webkit for trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

Timestamp:

Jun 12, 2020, 8:09:21 PM (5 years ago)

Author:

[email protected]

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/JavaScriptCore:

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:

File:

• trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp (modified) (9 diffs)

trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

@@ -56 +56 @@
     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,
@@ -63 +64 @@
 
     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".
 */
@@ -85 +86 @@
 // ------------------------------ 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();
 }
@@ -2248 +2251 @@
     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);
 }
 
@@ -2280 +2284 @@
     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());
 }
@@ -2296 +2300 @@
     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);
 }
@@ -2485 +2489 @@
     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);
 }
 
@@ -3200 +3205 @@
     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;
@@ -3353 +3358 @@
 // ------------------------------ AssignErrorNode -----------------------------------
 
-RegisterID* AssignErrorNode::emitBytecode(BytecodeGenerator& generator, RegisterID*)
-{
-    return emitThrowReferenceError(generator, "Left side of assignment is not a reference."_s);
+RegisterID* AssignErrorNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
+{
+    return emitThrowReferenceError(generator, "Left side of assignment is not a reference."_s, dst);
 }