From 4fafd076ae4b515dddfe8926e47441a0c625d002 Mon Sep 17 00:00:00 2001 From: Jakob Kummerow Date: Thu, 22 Jul 2021 16:10:51 +0200 Subject: [PATCH] [wasm-gc] Fix lifetime of off-heap type information... ...while on-heap objects are referring to it. This is accomplished by storing a reference to its associated WasmInstanceObject on every WasmTypeInfo object. Details: https://bit.ly/2UxD4hW Fixed: v8:11953 Change-Id: Ifb6f976142356021393d41c50717d210d525d521 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3043959 Commit-Queue: Jakob Kummerow Reviewed-by: Michael Lippautz Reviewed-by: Manos Koukoutos Cr-Commit-Position: refs/heads/master@{#75863} --- src/diagnostics/objects-printer.cc | 1 + src/heap/factory.cc | 7 ++++--- src/heap/factory.h | 3 ++- src/objects/objects-body-descriptors-inl.h | 5 +++++ src/wasm/module-instantiate.cc | 22 +++++++++++++-------- src/wasm/wasm-objects.h | 6 ++++-- src/wasm/wasm-objects.tq | 9 +++++++++ test/mjsunit/wasm/wasm-array-js-interop.js | 4 ---- test/mjsunit/wasm/wasm-struct-js-interop.js | 4 ---- 9 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/diagnostics/objects-printer.cc b/src/diagnostics/objects-printer.cc index 40b10afcb5..adc84d71ff 100644 --- a/src/diagnostics/objects-printer.cc +++ b/src/diagnostics/objects-printer.cc @@ -1858,6 +1858,7 @@ void WasmTypeInfo::WasmTypeInfoPrint(std::ostream& os) { os << "\n - type address: " << reinterpret_cast(foreign_address()); os << "\n - supertypes: " << Brief(supertypes()); os << "\n - subtypes: " << Brief(subtypes()); + os << "\n - instance: " << Brief(instance()); os << "\n"; } diff --git a/src/heap/factory.cc b/src/heap/factory.cc index d39c2f2750..dececbc1a5 100644 --- a/src/heap/factory.cc +++ b/src/heap/factory.cc @@ -1466,9 +1466,9 @@ Handle Factory::NewForeign(Address addr) { } #if V8_ENABLE_WEBASSEMBLY -Handle Factory::NewWasmTypeInfo(Address type_address, - Handle opt_parent, - int instance_size_bytes) { +Handle Factory::NewWasmTypeInfo( + Address type_address, Handle opt_parent, int instance_size_bytes, + Handle instance) { // We pretenure WasmTypeInfo objects because they are refererenced by Maps, // which are assumed to be long-lived. The supertypes list is constant // after initialization, so we pretenure that too. @@ -1493,6 +1493,7 @@ Handle Factory::NewWasmTypeInfo(Address type_address, result.set_supertypes(*supertypes, SKIP_WRITE_BARRIER); result.set_subtypes(*subtypes); result.set_instance_size(instance_size_bytes); + result.set_instance(*instance); return handle(result, isolate()); } diff --git a/src/heap/factory.h b/src/heap/factory.h index 366f441b05..6abe59b987 100644 --- a/src/heap/factory.h +++ b/src/heap/factory.h @@ -561,7 +561,8 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase { #if V8_ENABLE_WEBASSEMBLY Handle NewWasmTypeInfo(Address type_address, Handle opt_parent, - int instance_size_bytes); + int instance_size_bytes, + Handle instance); Handle NewWasmCapiFunctionData( Address call_target, Handle embedder_data, Handle wrapper_code, diff --git a/src/objects/objects-body-descriptors-inl.h b/src/objects/objects-body-descriptors-inl.h index a0b2256b12..9be9739b1c 100644 --- a/src/objects/objects-body-descriptors-inl.h +++ b/src/objects/objects-body-descriptors-inl.h @@ -617,6 +617,7 @@ class WasmTypeInfo::BodyDescriptor final : public BodyDescriptorBase { v); IteratePointer(obj, kSupertypesOffset, v); IteratePointer(obj, kSubtypesOffset, v); + IteratePointer(obj, kInstanceOffset, v); } static inline int SizeOf(Map map, HeapObject object) { return kSize; } @@ -719,6 +720,8 @@ class WasmArray::BodyDescriptor final : public BodyDescriptorBase { template static inline void IterateBody(Map map, HeapObject obj, int object_size, ObjectVisitor* v) { + // The type is safe to use because it's kept alive by the {map}'s + // WasmTypeInfo. if (!WasmArray::GcSafeType(map)->element_type().is_reference()) return; IteratePointers(obj, WasmArray::kHeaderSize, object_size, v); } @@ -741,6 +744,8 @@ class WasmStruct::BodyDescriptor final : public BodyDescriptorBase { static inline void IterateBody(Map map, HeapObject obj, int object_size, ObjectVisitor* v) { WasmStruct wasm_struct = WasmStruct::cast(obj); + // The {type} is safe to use because it's kept alive by the {map}'s + // WasmTypeInfo. wasm::StructType* type = WasmStruct::GcSafeType(map); for (uint32_t i = 0; i < type->field_count(); i++) { if (!type->field(i).is_reference()) continue; diff --git a/src/wasm/module-instantiate.cc b/src/wasm/module-instantiate.cc index e7d6ba55c8..22e6511d7c 100644 --- a/src/wasm/module-instantiate.cc +++ b/src/wasm/module-instantiate.cc @@ -139,7 +139,8 @@ Handle CreateArrayDescriptorArray( // TODO(jkummerow): Move these elsewhere. Handle CreateStructMap(Isolate* isolate, const WasmModule* module, - int struct_index, Handle opt_rtt_parent) { + int struct_index, Handle opt_rtt_parent, + Handle instance) { const wasm::StructType* type = module->struct_type(struct_index); const int inobject_properties = 0; // We have to use the variable size sentinel because the instance size @@ -150,7 +151,8 @@ Handle CreateStructMap(Isolate* isolate, const WasmModule* module, // TODO(jkummerow): If NO_ELEMENTS were supported, we could use that here. const ElementsKind elements_kind = TERMINAL_FAST_ELEMENTS_KIND; Handle type_info = isolate->factory()->NewWasmTypeInfo( - reinterpret_cast
(type), opt_rtt_parent, real_instance_size); + reinterpret_cast
(type), opt_rtt_parent, real_instance_size, + instance); Handle descriptors = CreateStructDescriptorArray(isolate, type); Handle map = isolate->factory()->NewMap( @@ -163,7 +165,8 @@ Handle CreateStructMap(Isolate* isolate, const WasmModule* module, } Handle CreateArrayMap(Isolate* isolate, const WasmModule* module, - int array_index, Handle opt_rtt_parent) { + int array_index, Handle opt_rtt_parent, + Handle instance) { const wasm::ArrayType* type = module->array_type(array_index); const int inobject_properties = 0; const int instance_size = kVariableSizeSentinel; @@ -172,7 +175,8 @@ Handle CreateArrayMap(Isolate* isolate, const WasmModule* module, const InstanceType instance_type = WASM_ARRAY_TYPE; const ElementsKind elements_kind = TERMINAL_FAST_ELEMENTS_KIND; Handle type_info = isolate->factory()->NewWasmTypeInfo( - reinterpret_cast
(type), opt_rtt_parent, cached_instance_size); + reinterpret_cast
(type), opt_rtt_parent, cached_instance_size, + instance); // TODO(ishell): get canonical descriptor array for WasmArrays from roots. Handle descriptors = CreateArrayDescriptorArray(isolate, type); @@ -242,10 +246,10 @@ Handle AllocateSubRtt(Isolate* isolate, // Allocate a fresh RTT otherwise. Handle rtt; if (module->has_struct(type)) { - rtt = wasm::CreateStructMap(isolate, module, type, parent); + rtt = wasm::CreateStructMap(isolate, module, type, parent, instance); } else { DCHECK(module->has_array(type)); - rtt = wasm::CreateArrayMap(isolate, module, type, parent); + rtt = wasm::CreateArrayMap(isolate, module, type, parent, instance); } if (mode == WasmRttSubMode::kCanonicalize) { @@ -658,10 +662,12 @@ MaybeHandle InstanceBuilder::Build() { Handle map; switch (module_->type_kinds[map_index]) { case kWasmStructTypeCode: - map = CreateStructMap(isolate_, module_, map_index, Handle()); + map = CreateStructMap(isolate_, module_, map_index, Handle(), + instance); break; case kWasmArrayTypeCode: - map = CreateArrayMap(isolate_, module_, map_index, Handle()); + map = CreateArrayMap(isolate_, module_, map_index, Handle(), + instance); break; case kWasmFunctionTypeCode: // TODO(7748): Think about canonicalizing rtts to make them work for diff --git a/src/wasm/wasm-objects.h b/src/wasm/wasm-objects.h index 4f03948a55..f4f5338483 100644 --- a/src/wasm/wasm-objects.h +++ b/src/wasm/wasm-objects.h @@ -943,9 +943,11 @@ class WasmArray : public TorqueGeneratedWasmArray { namespace wasm { Handle CreateStructMap(Isolate* isolate, const WasmModule* module, - int struct_index, MaybeHandle rtt_parent); + int struct_index, MaybeHandle rtt_parent, + Handle instance); Handle CreateArrayMap(Isolate* isolate, const WasmModule* module, - int array_index, MaybeHandle rtt_parent); + int array_index, MaybeHandle rtt_parent, + Handle instance); Handle AllocateSubRtt(Isolate* isolate, Handle instance, uint32_t type, Handle parent, WasmRttSubMode mode); diff --git a/src/wasm/wasm-objects.tq b/src/wasm/wasm-objects.tq index dd5b2082d3..f97d1bf691 100644 --- a/src/wasm/wasm-objects.tq +++ b/src/wasm/wasm-objects.tq @@ -130,6 +130,15 @@ extern class WasmTypeInfo extends Foreign { subtypes: ArrayList; // In bytes, used for struct allocation. instance_size: Smi; + // We must make sure that the StructType/ArrayType, which is allocated in + // the WasmModule's "signature_zone", stays around as long as there are + // HeapObjects referring to it. Short term, we simply keep a reference to + // the instance, which in turn keeps the entire WasmModule alive. + // TODO(jkummerow): Possible optimization: manage the "signature_zone"'s + // lifetime separately by having WasmModule refer to it via std::shared_ptr, + // and introduce a new link from here to just that zone using a Managed<...>. + // Details: https://bit.ly/2UxD4hW + instance: WasmInstanceObject; } // WasmObject corresponds to data ref types which are WasmStruct and WasmArray. diff --git a/test/mjsunit/wasm/wasm-array-js-interop.js b/test/mjsunit/wasm/wasm-array-js-interop.js index 7f7c411c34..e3f0891256 100644 --- a/test/mjsunit/wasm/wasm-array-js-interop.js +++ b/test/mjsunit/wasm/wasm-array-js-interop.js @@ -9,9 +9,6 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); const kIterationsCountForICProgression = 20; -// TODO(ishell): remove once leaked maps could keep NativeModule alive. -let instances = []; - function createArray_i() { let builder = new WasmModuleBuilder(); @@ -49,7 +46,6 @@ function createArray_i() { .exportAs("array_set"); let instance = builder.instantiate(); - instances.push(instance); let new_array = instance.exports.new_array; let array_get = instance.exports.array_get; let array_set = instance.exports.array_set; diff --git a/test/mjsunit/wasm/wasm-struct-js-interop.js b/test/mjsunit/wasm/wasm-struct-js-interop.js index 2545fa68d8..ec53dbe370 100644 --- a/test/mjsunit/wasm/wasm-struct-js-interop.js +++ b/test/mjsunit/wasm/wasm-struct-js-interop.js @@ -9,9 +9,6 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); const kIterationsCountForICProgression = 20; -// TODO(ishell): remove once leaked maps could keep NativeModule alive. -let instances = []; - function createSimpleStruct(field_type, value1, value2) { const builder = new WasmModuleBuilder(); @@ -52,7 +49,6 @@ function createSimpleStruct(field_type, value1, value2) { .exportAs("set_field"); let instance = builder.instantiate(); - instances.push(instance); let new_struct = instance.exports.new_struct; let get_field = instance.exports.get_field; let set_field = instance.exports.set_field;