[wasm] Cleanup and document {NativeModule} mutex.

This also makes the {AddCodeCopy} method more specific to only apply to
import wrappers, otherwise the use of {set_code} would be unprotected.

R=clemensh@chromium.org
BUG=v8:8015

Change-Id: I62561560f57e4cc235a338c0e769e50ff55ec42d
Reviewed-on: https://chromium-review.googlesource.com/1238477Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56137}

src/wasm/module-compiler.cc

@@ -1532,8 +1532,8 @@ int InstanceBuilder::ProcessImports(Handle<WasmInstanceObject> instance) {
                   .ToHandleChecked();
           RecordStats(*wrapper_code, isolate_->counters());
 
-          WasmCode* wasm_code = native_module->AddCodeCopy(
-              wrapper_code, WasmCode::kWasmToJsWrapper, func_index);
+          WasmCode* wasm_code =
+              native_module->AddImportWrapper(wrapper_code, func_index);
           ImportedFunctionEntry entry(instance, func_index);
           entry.set_wasm_to_js(*js_receiver, wasm_code);
         }

src/wasm/wasm-code-manager.cc

@@ -412,13 +412,12 @@ WasmCode* NativeModule::AddOwnedCode(
   return code;
 }
 
-WasmCode* NativeModule::AddCodeCopy(Handle<Code> code, WasmCode::Kind kind,
-                                    uint32_t index) {
+WasmCode* NativeModule::AddImportWrapper(Handle<Code> code, uint32_t index) {
   // TODO(wasm): Adding instance-specific wasm-to-js wrappers as owned code to
   // this NativeModule is a memory leak until the whole NativeModule dies.
-  WasmCode* ret = AddAnonymousCode(code, kind);
+  WasmCode* ret = AddAnonymousCode(code, WasmCode::kWasmToJsWrapper);
+  DCHECK_LT(index, module_->num_imported_functions);
   ret->index_ = Just(index);
-  if (index >= module_->num_imported_functions) set_code(index, ret);
   return ret;
 }
 
@@ -431,6 +430,11 @@ WasmCode* NativeModule::AddInterpreterEntry(Handle<Code> code, uint32_t index) {
   return ret;
 }
 
+WasmCode* NativeModule::AddCodeForTesting(Handle<Code> code) {
+  WasmCode* ret = AddAnonymousCode(code, WasmCode::kFunction);
+  return ret;
+}
+
 void NativeModule::SetLazyBuiltin(Handle<Code> code) {
   uint32_t num_wasm_functions = module_->num_declared_functions;
   if (num_wasm_functions == 0) return;

src/wasm/wasm-code-manager.h

@@ -216,8 +216,8 @@ class V8_EXPORT_PRIVATE NativeModule final {
   static constexpr bool kCanAllocateMoreMemory = true;
 #endif
 
-  // {AddCode} is thread safe w.r.t. other calls to {AddCode} or {AddCodeCopy},
-  // i.e. it can be called concurrently from background threads.
+  // {AddCode} is thread safe w.r.t. other calls to {AddCode} or methods adding
+  // code below, i.e. it can be called concurrently from background threads.
   WasmCode* AddCode(uint32_t index, const CodeDesc& desc, uint32_t stack_slots,
                     size_t safepoint_table_offset, size_t handler_table_offset,
                     OwnedVector<trap_handler::ProtectedInstructionData>
@@ -234,16 +234,19 @@ class V8_EXPORT_PRIVATE NativeModule final {
       OwnedVector<const byte> reloc_info,
       OwnedVector<const byte> source_position_table, WasmCode::Tier tier);
 
-  // A way to copy over JS-allocated code. This is because we compile
-  // certain wrappers using a different pipeline.
-  WasmCode* AddCodeCopy(Handle<Code> code, WasmCode::Kind kind, uint32_t index);
+  // Add an import wrapper for wasm-to-JS transitions. This method copies over
+  // JS-allocated code, because we compile wrappers using a different pipeline.
+  WasmCode* AddImportWrapper(Handle<Code> code, uint32_t index);
 
-  // Add an interpreter entry. For the same reason as AddCodeCopy, we
+  // Add an interpreter entry. For the same reason as AddImportWrapper, we
   // currently compile these using a different pipeline and we can't get a
   // CodeDesc here. When adding interpreter wrappers, we do not insert them in
   // the code_table, however, we let them self-identify as the {index} function.
   WasmCode* AddInterpreterEntry(Handle<Code> code, uint32_t index);
 
+  // Adds anonymous code for testing purposes.
+  WasmCode* AddCodeForTesting(Handle<Code> code);
+
   // When starting lazy compilation, provide the WasmLazyCompile builtin by
   // calling SetLazyBuiltin. It will be copied into this NativeModule and the
   // jump table will be populated with that copy.
@@ -390,12 +393,6 @@ class V8_EXPORT_PRIVATE NativeModule final {
   // AsyncCompileJob).
   std::shared_ptr<const WasmModule> module_;
 
-  // Holds all allocated code objects, is maintained to be in ascending order
-  // according to the codes instruction start address to allow lookups.
-  std::vector<std::unique_ptr<WasmCode>> owned_code_;
-
-  std::unique_ptr<WasmCode* []> code_table_;
-
   OwnedVector<const byte> wire_bytes_;
 
   WasmCode* runtime_stub_table_[WasmCode::kRuntimeStubCount] = {nullptr};
@@ -408,13 +405,25 @@ class V8_EXPORT_PRIVATE NativeModule final {
   // hence needs to be destructed first when this native module dies.
   std::unique_ptr<CompilationState, CompilationStateDeleter> compilation_state_;
 
-  // This mutex protects concurrent calls to {AddCode} and {AddCodeCopy}.
+  // This mutex protects concurrent calls to {AddCode} and friends.
   mutable base::Mutex allocation_mutex_;
 
+  //////////////////////////////////////////////////////////////////////////////
+  // Protected by {allocation_mutex_}:
+
+  // Holds all allocated code objects, is maintained to be in ascending order
+  // according to the codes instruction start address to allow lookups.
+  std::vector<std::unique_ptr<WasmCode>> owned_code_;
+
+  std::unique_ptr<WasmCode* []> code_table_;
+
   DisjointAllocationPool free_code_space_;
   DisjointAllocationPool allocated_code_space_;
   std::list<VirtualMemory> owned_code_space_;
 
+  // End of fields protected by {allocation_mutex_}.
+  //////////////////////////////////////////////////////////////////////////////
+
   WasmCodeManager* wasm_code_manager_;
   std::atomic<size_t> committed_code_space_{0};
   int modification_scope_depth_ = 0;

test/cctest/compiler/test-multiple-return.cc

@@ -190,9 +190,8 @@ void TestReturnMultipleValues(MachineType type) {
 
       std::unique_ptr<wasm::NativeModule> module = AllocateNativeModule(
           handles.main_isolate(), code->raw_instruction_size());
-      byte* code_start = module->AddCodeCopy(code, wasm::WasmCode::kFunction, 0)
-                             ->instructions()
-                             .start();
+      byte* code_start =
+          module->AddCodeForTesting(code)->instructions().start();
 
       RawMachineAssemblerTester<int32_t> mt;
       const int input_count = 2 + param_count;
@@ -280,9 +279,7 @@ void ReturnLastValue(MachineType type) {
 
     std::unique_ptr<wasm::NativeModule> module = AllocateNativeModule(
         handles.main_isolate(), code->raw_instruction_size());
-    byte* code_start = module->AddCodeCopy(code, wasm::WasmCode::kFunction, 0)
-                           ->instructions()
-                           .start();
+    byte* code_start = module->AddCodeForTesting(code)->instructions().start();
 
     // Generate caller.
     int expect = return_count - 1;
@@ -343,9 +340,7 @@ void ReturnSumOfReturns(MachineType type) {
 
     std::unique_ptr<wasm::NativeModule> module = AllocateNativeModule(
         handles.main_isolate(), code->raw_instruction_size());
-    byte* code_start = module->AddCodeCopy(code, wasm::WasmCode::kFunction, 0)
-                           ->instructions()
-                           .start();
+    byte* code_start = module->AddCodeForTesting(code)->instructions().start();
 
     // Generate caller.
     RawMachineAssemblerTester<int32_t> mt;

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

@@ -46,8 +46,8 @@ TestingModuleBuilder::TestingModuleBuilder(
         maybe_import_index, test_module_->origin,
         trap_handler::IsTrapHandlerEnabled() ? kUseTrapHandler
                                              : kNoTrapHandler);
-    auto wasm_to_js_wrapper = native_module_->AddCodeCopy(
-        code.ToHandleChecked(), WasmCode::kWasmToJsWrapper, maybe_import_index);
+    auto wasm_to_js_wrapper = native_module_->AddImportWrapper(
+        code.ToHandleChecked(), maybe_import_index);
 
     ImportedFunctionEntry(instance_object_, maybe_import_index)
         .set_wasm_to_js(*maybe_import->js_function, wasm_to_js_wrapper);

test/fuzzer/multi-return.cc

@@ -264,9 +264,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
 
   std::unique_ptr<wasm::NativeModule> module =
       AllocateNativeModule(i_isolate, code->raw_instruction_size());
-  byte* code_start = module->AddCodeCopy(code, wasm::WasmCode::kFunction, 0)
-                         ->instructions()
-                         .start();
+  byte* code_start = module->AddCodeForTesting(code)->instructions().start();
   // Generate wrapper.
   int expect = 0;