From f2b9c5005c191e307323b631c63ad206586c413d Mon Sep 17 00:00:00 2001
From: Michael Starzinger <mstarzinger@chromium.org>
Date: Mon, 15 May 2017 14:46:00 +0200
Subject: [PATCH] [asm.js] Fix evaluation of first for-statement expression.

This makes sure that the evaluation result of the first expression in
for-statements is properly dropped, to leave the stack in a balanced
state after the statement. It also makes sure validation failures in
said expression are handled correctly.

R=clemensh@chromium.org
TEST=mjsunit/regress/regress-crbug-721835
BUG=chromium:721835

Change-Id: I7e6cff4cea0bbf5aad6a3459e27a08ea814dbdbe
Reviewed-on: https://chromium-review.googlesource.com/506148
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45299}
---
 src/asmjs/asm-parser.cc                      |  9 ++++--
 src/asmjs/asm-parser.h                       |  9 +++---
 test/mjsunit/regress/regress-crbug-721835.js | 31 ++++++++++++++++++++
 3 files changed, 42 insertions(+), 7 deletions(-)
 create mode 100644 test/mjsunit/regress/regress-crbug-721835.js

diff --git a/src/asmjs/asm-parser.cc b/src/asmjs/asm-parser.cc
index 753b8ab722..07f01f72af 100644
--- a/src/asmjs/asm-parser.cc
+++ b/src/asmjs/asm-parser.cc
@@ -988,7 +988,7 @@ void AsmJsParser::ValidateFunctionLocals(
   }
 }
 
-// ValidateStatement
+// 6.5 ValidateStatement
 void AsmJsParser::ValidateStatement() {
   call_coercion_ = nullptr;
   if (Peek('{')) {
@@ -1172,7 +1172,11 @@ void AsmJsParser::ForStatement() {
   EXPECT_TOKEN(TOK(for));
   EXPECT_TOKEN('(');
   if (!Peek(';')) {
-    Expression(nullptr);
+    AsmType* ret;
+    RECURSE(ret = Expression(nullptr));
+    if (!ret->IsA(AsmType::Void())) {
+      current_function_builder_->Emit(kExprDrop);
+    }
   }
   EXPECT_TOKEN(';');
   // a: block {
@@ -1198,6 +1202,7 @@ void AsmJsParser::ForStatement() {
   scanner_.Seek(increment_position);
   if (!Peek(')')) {
     RECURSE(Expression(nullptr));
+    // NOTE: No explicit drop because below break is an implicit drop.
   }
   current_function_builder_->EmitWithU8(kExprBr, 0);
   scanner_.Seek(end_position);
diff --git a/src/asmjs/asm-parser.h b/src/asmjs/asm-parser.h
index 07b7b5d6d6..171aab29d5 100644
--- a/src/asmjs/asm-parser.h
+++ b/src/asmjs/asm-parser.h
@@ -271,10 +271,9 @@ class AsmJsParser {
   FunctionSig* ConvertSignature(AsmType* return_type,
                                 const std::vector<AsmType*>& params);
 
-  // 6.1 ValidateModule
-  void ValidateModule();
-  void ValidateModuleParameters();
-  void ValidateModuleVars();
+  void ValidateModule();            // 6.1 ValidateModule
+  void ValidateModuleParameters();  // 6.1 ValidateModule - parameters
+  void ValidateModuleVars();        // 6.1 ValidateModule - variables
   void ValidateModuleVar(bool mutable_variable);
   void ValidateModuleVarImport(VarInfo* info, bool mutable_variable);
   void ValidateModuleVarStdlib(VarInfo* info);
@@ -287,7 +286,7 @@ class AsmJsParser {
   void ValidateFunctionParams(std::vector<AsmType*>* params);
   void ValidateFunctionLocals(size_t param_count,
                               std::vector<ValueType>* locals);
-  void ValidateStatement();              // ValidateStatement
+  void ValidateStatement();              // 6.5 ValidateStatement
   void Block();                          // 6.5.1 Block
   void ExpressionStatement();            // 6.5.2 ExpressionStatement
   void EmptyStatement();                 // 6.5.3 EmptyStatement
diff --git a/test/mjsunit/regress/regress-crbug-721835.js b/test/mjsunit/regress/regress-crbug-721835.js
new file mode 100644
index 0000000000..80f99e6dd5
--- /dev/null
+++ b/test/mjsunit/regress/regress-crbug-721835.js
@@ -0,0 +1,31 @@
+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --validate-asm --allow-natives-syntax
+
+(function TestValidationFailureInForStatement() {
+  function Module() {
+    "use asm"
+    function f() {
+      var a = 0;
+      for (a = b; 0; 0) {};
+      return 0;
+    }
+    return { f:f };
+  }
+  assertThrows(() => Module().f(), ReferenceError);
+  assertFalse(%IsAsmWasmCode(Module));
+})();
+
+(function TestForStatementInVoidFunction() {
+  function Module() {
+    "use asm"
+    function f() {
+      for (1; 0; 0) {};
+    }
+    return { f:f };
+  }
+  assertDoesNotThrow(() => Module().f());
+  assertTrue(%IsAsmWasmCode(Module));
+})();