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

This is a reland of 5648aad5.
Previous compile error should be fixed by disabling strict aliasing
assumptions on gyp: https://chromium-review.googlesource.com/c/571806

Original change's description:
> [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/567058
> Reviewed-by: Ben Titzer <titzer@chromium.org>
> Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#46629}

TBR=titzer@chromium.org

Bug: chromium:610330
Change-Id: Ic7836b1b1a044a89f2138f0c76f92acd3a1b2f2b
Reviewed-on: https://chromium-review.googlesource.com/570578
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46679}

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);