[wasm] Fix ToNumber conversion

There were two bugs, one partly hiding the other one:
1) We generate the ToNumber conversion for each WASM_TO_JS wrapper,
   even if the expected return type is void.
2) The return node in the WASM_TO_JS wrapper did not use the effect of
   the ToNumber conversion.

This CL fixes both, and adds test cases to check that we do throw an
error trying to convert (e.g.) Symbol to a number, but only if the
return type is not void.
Additional test check that a user-provided valueOf method is actually
called the correct number of times.

R=titzer@chromium.org, bradnelson@chromium.org
BUG=v8:4203

Review-Url: https://codereview.chromium.org/2552123004
Cr-Commit-Position: refs/heads/master@{#41552}

src/compiler/wasm-compiler.cc

@@ -2376,8 +2376,7 @@ Node* WasmGraphBuilder::ToJS(Node* node, wasm::LocalType type) {
   }
 }
 
-Node* WasmGraphBuilder::BuildJavaScriptToNumber(Node* node, Node* context,
-                                                Node* effect, Node* control) {
+Node* WasmGraphBuilder::BuildJavaScriptToNumber(Node* node, Node* context) {
   Callable callable = CodeFactory::ToNumber(jsgraph()->isolate());
   CallDescriptor* desc = Linkage::GetStubCallDescriptor(
       jsgraph()->isolate(), jsgraph()->zone(), callable.descriptor(), 0,
@@ -2385,7 +2384,7 @@ Node* WasmGraphBuilder::BuildJavaScriptToNumber(Node* node, Node* context,
   Node* stub_code = jsgraph()->HeapConstant(callable.code());
 
   Node* result = graph()->NewNode(jsgraph()->common()->Call(desc), stub_code,
-                                  node, context, effect, control);
+                                  node, context, *effect_, *control_);
 
   *effect_ = result;
 
@@ -2505,8 +2504,10 @@ Node* WasmGraphBuilder::BuildChangeTaggedToFloat64(Node* value) {
 
 Node* WasmGraphBuilder::FromJS(Node* node, Node* context,
                                wasm::LocalType type) {
+  DCHECK_NE(wasm::kAstStmt, type);
+
   // Do a JavaScript ToNumber.
-  Node* num = BuildJavaScriptToNumber(node, context, *effect_, *control_);
+  Node* num = BuildJavaScriptToNumber(node, context);
 
   // Change representation.
   SimplifiedOperatorBuilder simplified(jsgraph()->zone());
@@ -2531,9 +2532,6 @@ Node* WasmGraphBuilder::FromJS(Node* node, Node* context,
       break;
     case wasm::kAstF64:
       break;
-    case wasm::kAstStmt:
-      num = jsgraph()->Int32Constant(0);
-      break;
     default:
       UNREACHABLE();
       return nullptr;
@@ -2786,14 +2784,16 @@ void WasmGraphBuilder::BuildWasmToJSWrapper(Handle<JSReceiver> target,
     call = graph()->NewNode(jsgraph()->common()->Call(desc), pos, args);
   }
 
+  *effect_ = call;
+
   // Convert the return value back.
-  Node* ret;
-  Node* val =
-      FromJS(call, HeapConstant(isolate->native_context()),
-             sig->return_count() == 0 ? wasm::kAstStmt : sig->GetReturn());
-  Node* pop_size = jsgraph()->Int32Constant(0);
-    ret = graph()->NewNode(jsgraph()->common()->Return(), pop_size, val, call,
-                           start);
+  Node* i32_zero = jsgraph()->Int32Constant(0);
+  Node* val = sig->return_count() == 0
+                  ? i32_zero
+                  : FromJS(call, HeapConstant(isolate->native_context()),
+                           sig->GetReturn());
+  Node* ret = graph()->NewNode(jsgraph()->common()->Return(), i32_zero, val,
+                               *effect_, start);
 
   MergeControlToEnd(jsgraph(), ret);
 }

src/compiler/wasm-compiler.h

@@ -326,8 +326,7 @@ class WasmGraphBuilder {
                        MachineType result_type, int trap_zero,
                        wasm::WasmCodePosition position);
 
-  Node* BuildJavaScriptToNumber(Node* node, Node* context, Node* effect,
-                                Node* control);
+  Node* BuildJavaScriptToNumber(Node* node, Node* context);
 
   Node* BuildChangeInt32ToTagged(Node* value);
   Node* BuildChangeFloat64ToTagged(Node* value);

test/mjsunit/wasm/ffi-error.js

@@ -115,3 +115,14 @@ assertThrows(function() {
     main(13);
   }, TypeError);
 })();
+
+(function ImportSymbolToNumberThrows() {
+  var builder = new WasmModuleBuilder();
+  var index = builder.addImport("func", kSig_i_v);
+  builder.addFunction("main", kSig_i_v)
+      .addBody([kExprCallFunction, 0])
+      .exportFunc();
+  var func = () => Symbol();
+  var main = builder.instantiate({func: func}).exports.main;
+  assertThrows(() => main(), TypeError);
+})();

test/mjsunit/wasm/ffi.js

@@ -338,3 +338,50 @@ testCallBinopVoid(kAstF64);
     '-3': {18: print}
   });
 })();
+
+(function ImportSymbolAsVoidDoesNotThrow() {
+  var builder = new WasmModuleBuilder();
+  // Return type is void, so there should be no ToNumber conversion.
+  var index = builder.addImport("func", kSig_v_v);
+  builder.addFunction("main", kSig_v_v)
+      .addBody([kExprCallFunction, 0])
+      .exportFunc();
+  var func = () => Symbol();
+  var main = builder.instantiate({func: func}).exports.main;
+  main();
+})();
+
+(function ToNumberCalledOnImport() {
+  var builder = new WasmModuleBuilder();
+  // Return type is int, so there should be a ToNumber conversion.
+  var index = builder.addImport("func", kSig_i_v);
+  builder.addFunction("main", kSig_i_v)
+      .addBody([kExprCallFunction, 0])
+      .exportFunc();
+  var num_valueOf = 0;
+  function Foo() {}
+  Foo.prototype.valueOf = () => ++num_valueOf;
+  var func = () => new Foo();
+  var main = builder.instantiate({func: func}).exports.main;
+  main();
+  assertEquals(1, num_valueOf);
+  main();
+  assertEquals(2, num_valueOf);
+})();
+
+(function ToNumberNotCalledOnVoidImport() {
+  var builder = new WasmModuleBuilder();
+  // Return type is void, so there should be no ToNumber conversion.
+  var index = builder.addImport("func", kSig_v_v);
+  builder.addFunction("main", kSig_v_v)
+      .addBody([kExprCallFunction, 0])
+      .exportFunc();
+  var num_valueOf = 0;
+  function Foo() {}
+  Foo.prototype.valueOf = () => ++num_valueOf;
+  var func = () => new Foo();
+  var main = builder.instantiate({func: func}).exports.main;
+  main();
+  main();
+  assertEquals(0, num_valueOf);
+})();