[wasm] Don't store global handles in the interpreter

Storing global handles in the interpreter is dangerous, because the
global handles are strong roots into the heap. The interpreter itself is
referenced from the heap via a Managed. Hence the interpreter keeps the
instance alive, while the instance keeps the Managed alive. So the GC
will never collect them.

This CL refactors this to only store the handle to the instance object
while executing in the interpreter, and clearing it when returning.
It also removes the cache of import wrappers, as it should not be
performance critical, but keeps lots of objects alive. If it turns out
to be performance critical, we will have to reintroduce such a cache
stored in the WasmDebugInfo object.

R=titzer@chromium.org
CC=ahaas@chromium.org

Bug: chromium:610330
Change-Id: I54b489dadc16685887c0c1a98da6fd0df5ad7cbb
Reviewed-on: https://chromium-review.googlesource.com/567058Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46629}

src/wasm/wasm-debug.cc

@@ -144,9 +144,6 @@ class InterpreterHandle {
 
     WasmInstanceObject* instance = debug_info->wasm_instance();
 
-    // Store a global handle to the wasm instance in the interpreter.
-    interpreter_.SetInstanceObject(instance);
-
     // Set memory start pointer and size.
     instance_.mem_start = nullptr;
     instance_.mem_size = 0;
@@ -193,7 +190,8 @@ class InterpreterHandle {
   // Returns true if exited regularly, false if a trap/exception occured and was
   // not handled inside this activation. In the latter case, a pending exception
   // will have been set on the isolate.
-  bool Execute(Address frame_pointer, uint32_t func_index,
+  bool Execute(Handle<WasmInstanceObject> instance_object,
+               Address frame_pointer, uint32_t func_index,
                uint8_t* arg_buffer) {
     DCHECK_GE(module()->functions.size(), func_index);
     FunctionSig* sig = module()->functions[func_index].sig;
@@ -222,6 +220,8 @@ class InterpreterHandle {
 
     uint32_t activation_id = StartActivation(frame_pointer);
 
+    WasmInterpreter::HeapObjectsScope heap_objects_scope(&interpreter_,
+                                                         instance_object);
     WasmInterpreter::Thread* thread = interpreter_.GetThread(0);
     thread->InitFrame(&module()->functions[func_index], wasm_args.start());
     bool finished = false;
@@ -705,8 +705,9 @@ void WasmDebugInfo::PrepareStep(StepAction step_action) {
 bool WasmDebugInfo::RunInterpreter(Address frame_pointer, int func_index,
                                    uint8_t* arg_buffer) {
   DCHECK_LE(0, func_index);
+  Handle<WasmInstanceObject> instance(wasm_instance());
   return GetInterpreterHandle(this)->Execute(
-      frame_pointer, static_cast<uint32_t>(func_index), arg_buffer);
+      instance, frame_pointer, static_cast<uint32_t>(func_index), arg_buffer);
 }
 
 std::vector<std::pair<uint32_t, int>> WasmDebugInfo::GetInterpretedStack(

src/wasm/wasm-interpreter.cc

@@ -936,21 +936,14 @@ class CodeMap {
   Zone* zone_;
   const WasmModule* module_;
   ZoneVector<InterpreterCode> interpreter_code_;
-  // Global handle to the wasm instance.
+  // This handle is set and reset by the SetInstanceObject() /
+  // ClearInstanceObject() method, which is used by the HeapObjectsScope.
   Handle<WasmInstanceObject> instance_;
-  // Global handle to array of unwrapped imports.
-  Handle<FixedArray> imported_functions_;
-  // Map from WASM_TO_JS wrappers to unwrapped imports (indexes into
-  // imported_functions_).
-  IdentityMap<int, ZoneAllocationPolicy> unwrapped_imports_;
 
  public:
   CodeMap(Isolate* isolate, const WasmModule* module,
           const uint8_t* module_start, Zone* zone)
-      : zone_(zone),
-        module_(module),
-        interpreter_code_(zone),
-        unwrapped_imports_(isolate->heap(), ZoneAllocationPolicy(zone)) {
+      : zone_(zone), module_(module), interpreter_code_(zone) {
     if (module == nullptr) return;
     interpreter_code_.reserve(module->functions.size());
     for (const WasmFunction& function : module->functions) {
@@ -964,37 +957,28 @@ class CodeMap {
     }
   }
 
-  ~CodeMap() {
-    // Destroy the global handles.
-    // Cast the location, not the handle, because the handle cast might access
-    // the object behind the handle.
-    GlobalHandles::Destroy(reinterpret_cast<Object**>(instance_.location()));
-    GlobalHandles::Destroy(
-        reinterpret_cast<Object**>(imported_functions_.location()));
+  void SetInstanceObject(Handle<WasmInstanceObject> instance) {
+    DCHECK(instance_.is_null());
+    instance_ = instance;
   }
 
+  void ClearInstanceObject() { instance_ = Handle<WasmInstanceObject>::null(); }
+
   const WasmModule* module() const { return module_; }
   bool has_instance() const { return !instance_.is_null(); }
-  Handle<WasmInstanceObject> instance() const {
+  WasmInstanceObject* instance() const {
     DCHECK(has_instance());
-    return instance_;
+    return *instance_;
   }
   MaybeHandle<WasmInstanceObject> maybe_instance() const {
-    return has_instance() ? instance_ : MaybeHandle<WasmInstanceObject>();
-  }
-
-  void SetInstanceObject(WasmInstanceObject* instance) {
-    // Only set the instance once (otherwise we have to destroy the global
-    // handle first).
-    DCHECK(instance_.is_null());
-    DCHECK_EQ(instance->module(), module_);
-    instance_ = instance->GetIsolate()->global_handles()->Create(instance);
+    return has_instance() ? handle(instance())
+                          : MaybeHandle<WasmInstanceObject>();
   }
 
   Code* GetImportedFunction(uint32_t function_index) {
-    DCHECK(!instance_.is_null());
+    DCHECK(has_instance());
     DCHECK_GT(module_->num_imported_functions, function_index);
-    FixedArray* code_table = instance_->compiled_module()->ptr_to_code_table();
+    FixedArray* code_table = instance()->compiled_module()->ptr_to_code_table();
     return Code::cast(code_table->get(static_cast<int>(function_index)));
   }
 
@@ -1051,48 +1035,6 @@ class CodeMap {
     code->side_table = nullptr;
     Preprocess(code);
   }
-
-  // Returns a callable object if the imported function has a JS-compatible
-  // signature, or a null handle otherwise.
-  Handle<HeapObject> GetCallableObjectForJSImport(Isolate* isolate,
-                                                  Handle<Code> code) {
-    DCHECK_EQ(Code::WASM_TO_JS_FUNCTION, code->kind());
-    int* unwrapped_index = unwrapped_imports_.Find(code);
-    if (unwrapped_index) {
-      return handle(
-          HeapObject::cast(imported_functions_->get(*unwrapped_index)),
-          isolate);
-    }
-    Handle<HeapObject> called_obj = UnwrapWasmToJSWrapper(isolate, code);
-    if (!called_obj.is_null()) {
-      // Cache the unwrapped callable object.
-      if (imported_functions_.is_null()) {
-        // This is the first call to an imported function. Allocate the
-        // FixedArray to cache unwrapped objects.
-        constexpr int kInitialCacheSize = 8;
-        Handle<FixedArray> new_imported_functions =
-            isolate->factory()->NewFixedArray(kInitialCacheSize, TENURED);
-        // First entry: Number of occupied slots.
-        new_imported_functions->set(0, Smi::kZero);
-        imported_functions_ =
-            isolate->global_handles()->Create(*new_imported_functions);
-      }
-      int this_idx = Smi::ToInt(imported_functions_->get(0)) + 1;
-      if (this_idx == imported_functions_->length()) {
-        Handle<FixedArray> new_imported_functions =
-            isolate->factory()->CopyFixedArrayAndGrow(imported_functions_,
-                                                      this_idx / 2, TENURED);
-        // Update the existing global handle:
-        *imported_functions_.location() = *new_imported_functions;
-      }
-      DCHECK_GT(imported_functions_->length(), this_idx);
-      DCHECK(imported_functions_->get(this_idx)->IsUndefined(isolate));
-      imported_functions_->set(0, Smi::FromInt(this_idx));
-      imported_functions_->set(this_idx, *called_obj);
-      unwrapped_imports_.Set(code, this_idx);
-    }
-    return called_obj;
-  }
 };
 
 Handle<Object> WasmValToNumber(Factory* factory, WasmVal val,
@@ -2140,7 +2082,7 @@ class ThreadImpl {
       DCHECK_EQ(2, deopt_data->length());
       WasmInstanceObject* target_instance =
           WasmInstanceObject::cast(WeakCell::cast(deopt_data->get(0))->value());
-      if (target_instance != *codemap()->instance()) {
+      if (target_instance != codemap()->instance()) {
         // TODO(wasm): Implement calling functions of other instances/modules.
         UNIMPLEMENTED();
       }
@@ -2150,8 +2092,7 @@ class ThreadImpl {
               codemap()->GetCode(target_func_idx)};
     }
 
-    Handle<HeapObject> target =
-        codemap()->GetCallableObjectForJSImport(isolate, code);
+    Handle<HeapObject> target = UnwrapWasmToJSWrapper(isolate, code);
 
     if (target.is_null()) {
       isolate->Throw(*isolate->factory()->NewTypeError(
@@ -2372,6 +2313,37 @@ const InterpretedFrameImpl* ToImpl(const InterpretedFrame* frame) {
   return reinterpret_cast<const InterpretedFrameImpl*>(frame);
 }
 
+//============================================================================
+// Implementation details of the heap objects scope.
+//============================================================================
+class HeapObjectsScopeImpl {
+ public:
+  HeapObjectsScopeImpl(CodeMap* codemap, Handle<WasmInstanceObject> instance)
+      : codemap_(codemap), needs_reset(!codemap_->has_instance()) {
+    if (needs_reset) {
+      instance_ = handle(*instance);
+      codemap_->SetInstanceObject(instance_);
+    } else {
+      DCHECK_EQ(*instance, codemap_->instance());
+      return;
+    }
+  }
+
+  ~HeapObjectsScopeImpl() {
+    if (!needs_reset) return;
+    DCHECK_EQ(*instance_, codemap_->instance());
+    codemap_->ClearInstanceObject();
+    // Clear the handle, such that anyone who accidentally copied them will
+    // notice.
+    *instance_.location() = nullptr;
+  }
+
+ private:
+  CodeMap* codemap_;
+  Handle<WasmInstanceObject> instance_;
+  bool needs_reset;
+};
+
 }  // namespace
 
 //============================================================================
@@ -2512,10 +2484,6 @@ bool WasmInterpreter::SetTracing(const WasmFunction* function, bool enabled) {
   return false;
 }
 
-void WasmInterpreter::SetInstanceObject(WasmInstanceObject* instance) {
-  internals_->codemap_.SetInstanceObject(instance);
-}
-
 int WasmInterpreter::GetThreadCount() {
   return 1;  // only one thread for now.
 }
@@ -2590,6 +2558,19 @@ WasmVal InterpretedFrame::GetStackValue(int index) const {
   return ToImpl(this)->GetStackValue(index);
 }
 
+//============================================================================
+// Public API of the heap objects scope.
+//============================================================================
+WasmInterpreter::HeapObjectsScope::HeapObjectsScope(
+    WasmInterpreter* interpreter, Handle<WasmInstanceObject> instance) {
+  static_assert(sizeof(data) == sizeof(HeapObjectsScopeImpl), "Size mismatch");
+  new (data) HeapObjectsScopeImpl(&interpreter->internals_->codemap_, instance);
+}
+
+WasmInterpreter::HeapObjectsScope::~HeapObjectsScope() {
+  reinterpret_cast<HeapObjectsScopeImpl*>(data)->~HeapObjectsScopeImpl();
+}
+
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

src/wasm/wasm-interpreter.h

@@ -140,6 +140,19 @@ class InterpretedFrame {
 // An interpreter capable of executing WebAssembly.
 class V8_EXPORT_PRIVATE WasmInterpreter {
  public:
+  // Open a HeapObjectsScope before running any code in the interpreter which
+  // needs access to the instance object or needs to call to JS functions.
+  class V8_EXPORT_PRIVATE HeapObjectsScope {
+   public:
+    HeapObjectsScope(WasmInterpreter* interpreter,
+                     Handle<WasmInstanceObject> instance);
+    ~HeapObjectsScope();
+
+   private:
+    char data[3 * sizeof(void*)];  // must match sizeof(HeapObjectsScopeImpl).
+    DISALLOW_COPY_AND_ASSIGN(HeapObjectsScope);
+  };
+
   // State machine for a Thread:
   //                         +---------Run()/Step()--------+
   //                         V                             |
@@ -236,13 +249,6 @@ class V8_EXPORT_PRIVATE WasmInterpreter {
   // Enable or disable tracing for {function}. Return the previous state.
   bool SetTracing(const WasmFunction* function, bool enabled);
 
-  // Set the associated wasm instance object.
-  // If the instance object has been set, some tables stored inside it are used
-  // instead of the tables stored in the WasmModule struct. This allows to call
-  // back and forth between the interpreter and outside code (JS or wasm
-  // compiled) without repeatedly copying information.
-  void SetInstanceObject(WasmInstanceObject*);
-
   //==========================================================================
   // Thread iteration and inspection.
   //==========================================================================

src/wasm/wasm-objects.h

@@ -592,11 +592,11 @@ class WasmDebugInfo : public FixedArray {
   DECL_GETTER(wasm_instance, WasmInstanceObject)
   DECL_OPTIONAL_ACCESSORS(locals_names, FixedArray)
 
-  enum {  // --
-    kInstanceIndex,
-    kInterpreterHandleIndex,
-    kInterpretedFunctionsIndex,
-    kLocalsNamesIndex,
+  enum {
+    kInstanceIndex,              // instance object.
+    kInterpreterHandleIndex,     // managed object containing the interpreter.
+    kInterpretedFunctionsIndex,  // array of interpreter entry code objects.
+    kLocalsNamesIndex,           // array of array of local names.
     kFieldCount
   };
 

test/cctest/wasm/wasm-run-utils.h

@@ -800,6 +800,8 @@ class WasmRunner : public WasmRunnerBase {
     thread->Reset();
     std::array<WasmVal, sizeof...(p)> args{{WasmVal(p)...}};
     thread->InitFrame(function(), args.data());
+    WasmInterpreter::HeapObjectsScope heap_objects_scope(
+        interpreter(), module().instance_object());
     if (thread->Run() == WasmInterpreter::FINISHED) {
       WasmVal val = thread->GetReturnValue();
       possible_nondeterminism_ |= thread->PossibleNondeterminism();

test/common/wasm/wasm-module-runner.cc

@@ -93,6 +93,7 @@ int32_t InterpretWasmModule(Isolate* isolate,
 
   WasmInterpreter* interpreter =
       WasmDebugInfo::SetupForTesting(instance, nullptr);
+  WasmInterpreter::HeapObjectsScope heap_objects_scope(interpreter, instance);
   WasmInterpreter::Thread* thread = interpreter->GetThread(0);
   thread->Reset();
   thread->InitFrame(&(instance->module()->functions[function_index]), args);