[wasm] Check the input of grow-memory before calling the runtime.

src/compiler/wasm-compiler.cc

 // Copyright 2015 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.
 
 #include "src/compiler/wasm-compiler.h"
 
 #include <memory>
 
 #include "src/isolate-inl.h"
 
@@ skipping 1650 matching lines @@
                      BuildCCall(sig_builder.Build(), args), position);
   const Operator* load_op = jsgraph()->machine()->Load(result_type);
   Node* load =
       graph()->NewNode(load_op, stack_slot_result, jsgraph()->Int32Constant(0),
                        *effect_, *control_);
   *effect_ = load;
   return load;
 }
 
 Node* WasmGraphBuilder::BuildGrowMemory(Node* input) {
+  Diamond check_input_range(
+      graph(), jsgraph()->common(),
+      graph()->NewNode(
+          jsgraph()->machine()->Uint32LessThanOrEqual(), input,
+          jsgraph()->Uint32Constant(wasm::WasmModule::kMaxMemPages)),
+      BranchHint::kTrue);

[gdeepti, 2016/08/30 02:19:56]

This makes the first check in the GrowMemory runtime function (for 0 initial
memory) redundant, remove the one in wasm-runtime.cc? 

[ahaas, 2016/08/30 06:49:53]

I replaced the check in runtime-wasm.cc with a DCHECK.

+
   Runtime::FunctionId function_id = Runtime::kWasmGrowMemory;
   const Runtime::Function* function = Runtime::FunctionForId(function_id);
   CallDescriptor* desc = Linkage::GetRuntimeCallDescriptor(
       jsgraph()->zone(), function_id, function->nargs, Operator::kNoThrow,
       CallDescriptor::kNoFlags);
-  Node** control_ptr = control_;
-  Node** effect_ptr = effect_;
   wasm::ModuleEnv* module = module_;
   input = BuildChangeUint32ToSmi(input);

[titzer, 2016/08/29 16:57:58]

If I remember correctly this will also have a check inside. Can we avoid that
somehow?

[ahaas, 2016/08/30 06:49:53]

I do not understand this comment, do you mean the check in the GrowMemory
runtime function? I also wonder if it is worth to optimize code here because
GrowMemory should be quite cold.

[titzer, 2016/08/30 08:25:47]

Sorry, I was thinking that BuildChangeUint32ToSmi() had a branch inside, but
apparently it doesn't.

   Node* inputs[] = {
       jsgraph()->CEntryStubConstant(function->result_size), input,  // C entry
       jsgraph()->ExternalConstant(
           ExternalReference(function_id, jsgraph()->isolate())),  // ref
       jsgraph()->Int32Constant(function->nargs),                  // arity
       jsgraph()->HeapConstant(module->instance->context),         // context
-      *effect_ptr,
-      *control_ptr};
-  Node* node = graph()->NewNode(jsgraph()->common()->Call(desc),
+      *effect_,
+      check_input_range.if_true};
+  Node* call = graph()->NewNode(jsgraph()->common()->Call(desc),
                                 static_cast<int>(arraysize(inputs)), inputs);
-  *effect_ptr = node;
-  node = BuildChangeSmiToInt32(node);
-  return node;
+
+  Node* result = BuildChangeSmiToInt32(call);
+
+  result = check_input_range.Phi(MachineRepresentation::kWord32, result,
+                                 jsgraph()->Int32Constant(-1));
+  *effect_ = graph()->NewNode(jsgraph()->common()->EffectPhi(2), call, *effect_,
+                              check_input_range.merge);
+  *control_ = check_input_range.merge;
+  return result;
 }
 
 Node* WasmGraphBuilder::BuildI32DivS(Node* left, Node* right,
                                      wasm::WasmCodePosition position) {
   MachineOperatorBuilder* m = jsgraph()->machine();
   trap_->ZeroCheck32(wasm::kTrapDivByZero, right, position);
   Node* before = *control_;
   Node* denom_is_m1;
   Node* denom_is_not_m1;
   Branch(
@@ skipping 1533 matching lines @@
                             function_->code_start_offset),
            compile_ms);
   }
 
   return code;
 }
 
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8