From 2fa5551932786d79c14980df4d6dfdd43359c373 Mon Sep 17 00:00:00 2001 From: Manos Koukoutos Date: Fri, 26 Nov 2021 13:16:13 +0000 Subject: [PATCH] [wasm] Keep external function reference for externref tables/globals See crrev.com/c/3277878 for context. We should only transform extenral to internal function references when passing a function value to a function-typed global or table. For their externref counterparts, we should preserve the reference unchanged. Bug: v8:11510, chromium:1273705 Change-Id: Ic1719c4d31e175f3a37ced6e4e4dfcd61a19ae57 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3302790 Commit-Queue: Manos Koukoutos Reviewed-by: Jakob Kummerow Cr-Commit-Position: refs/heads/main@{#78108} --- src/wasm/c-api.cc | 21 +++--- src/wasm/module-instantiate.cc | 11 ++-- src/wasm/wasm-js.cc | 29 +++++---- test/mjsunit/regress/wasm/regress-1273705.js | 7 ++ test/mjsunit/wasm/externref-table.js | 68 +++++++++++++++++++- 5 files changed, 107 insertions(+), 29 deletions(-) create mode 100644 test/mjsunit/regress/wasm/regress-1273705.js diff --git a/src/wasm/c-api.cc b/src/wasm/c-api.cc index 5c9d0acf21..4a62a27ed5 100644 --- a/src/wasm/c-api.cc +++ b/src/wasm/c-api.cc @@ -2010,11 +2010,12 @@ auto Table::set(size_t index, const Ref* ref) -> bool { i::Isolate* isolate = table->GetIsolate(); i::HandleScope handle_scope(isolate); i::Handle obj = WasmRefToV8(isolate, ref); - i::Handle entry; - if (!i::WasmInternalFunction::FromExternal(obj, isolate).ToHandle(&entry)) { - entry = obj; + // TODO(7748): Generalize the condition if other table types are allowed. + if ((table->type() == i::wasm::kWasmFuncRef || table->type().has_index()) && + !obj->IsNull()) { + obj = i::WasmInternalFunction::FromExternal(obj, isolate).ToHandleChecked(); } - i::WasmTableObject::Set(isolate, table, static_cast(index), entry); + i::WasmTableObject::Set(isolate, table, static_cast(index), obj); return true; } @@ -2028,13 +2029,13 @@ auto Table::grow(size_t delta, const Ref* ref) -> bool { i::Isolate* isolate = table->GetIsolate(); i::HandleScope scope(isolate); i::Handle obj = WasmRefToV8(isolate, ref); - i::Handle init_value; - if (!i::WasmInternalFunction::FromExternal(obj, isolate) - .ToHandle(&init_value)) { - init_value = obj; + // TODO(7748): Generalize the condition if other table types are allowed. + if ((table->type() == i::wasm::kWasmFuncRef || table->type().has_index()) && + !obj->IsNull()) { + obj = i::WasmInternalFunction::FromExternal(obj, isolate).ToHandleChecked(); } - int result = i::WasmTableObject::Grow( - isolate, table, static_cast(delta), init_value); + int result = i::WasmTableObject::Grow(isolate, table, + static_cast(delta), obj); return result >= 0; } diff --git a/src/wasm/module-instantiate.cc b/src/wasm/module-instantiate.cc index 44a22c67d8..f95d378f96 100644 --- a/src/wasm/module-instantiate.cc +++ b/src/wasm/module-instantiate.cc @@ -1474,12 +1474,11 @@ bool InstanceBuilder::ProcessImportedGlobal(Handle instance, ReportLinkError(error_message, global_index, module_name, import_name); return false; } - auto stored_value = - WasmExternalFunction::IsWasmExternalFunction(*value) - ? WasmInternalFunction::FromExternal(value, isolate_) - .ToHandleChecked() - : value; - WriteGlobalValue(global, WasmValue(stored_value, global.type)); + if (IsSubtypeOf(global.type, kWasmFuncRef, module_) && !value->IsNull()) { + value = + WasmInternalFunction::FromExternal(value, isolate_).ToHandleChecked(); + } + WriteGlobalValue(global, WasmValue(value, global.type)); return true; } diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc index 3b958a6084..6744aab748 100644 --- a/src/wasm/wasm-js.cc +++ b/src/wasm/wasm-js.cc @@ -1206,7 +1206,8 @@ void WebAssemblyTable(const v8::FunctionCallbackInfo& args) { "with the type of the new table."); return; } - if (element->IsJSFunction()) { + // TODO(7748): Generalize this if other table types are allowed. + if (type == i::wasm::kWasmFuncRef && !element->IsNull()) { element = i::WasmInternalFunction::FromExternal(element, i_isolate) .ToHandleChecked(); } @@ -1980,14 +1981,16 @@ void WebAssemblyTableGrow(const v8::FunctionCallbackInfo& args) { init_value = DefaultReferenceValue(i_isolate, receiver->type()); } - i::Handle internal_init_value; - if (!i::WasmInternalFunction::FromExternal(init_value, i_isolate) - .ToHandle(&internal_init_value)) { - internal_init_value = init_value; + // TODO(7748): Generalize this if other table types are allowed. + bool has_function_type = + receiver->type() == i::wasm::kWasmFuncRef || receiver->type().has_index(); + if (has_function_type && !init_value->IsNull()) { + init_value = i::WasmInternalFunction::FromExternal(init_value, i_isolate) + .ToHandleChecked(); } - int old_size = i::WasmTableObject::Grow(i_isolate, receiver, grow_by, - internal_init_value); + int old_size = + i::WasmTableObject::Grow(i_isolate, receiver, grow_by, init_value); if (old_size < 0) { thrower.RangeError("failed to grow table by %u", grow_by); @@ -2059,13 +2062,15 @@ void WebAssemblyTableSet(const v8::FunctionCallbackInfo& args) { return; } - i::Handle value; - if (!i::WasmInternalFunction::FromExternal(element, i_isolate) - .ToHandle(&value)) { - value = element; + // TODO(7748): Generalize this if other table types are allowed. + bool has_function_type = table_object->type() == i::wasm::kWasmFuncRef || + table_object->type().has_index(); + if (has_function_type && !element->IsNull()) { + element = i::WasmInternalFunction::FromExternal(element, i_isolate) + .ToHandleChecked(); } - i::WasmTableObject::Set(i_isolate, table_object, index, value); + i::WasmTableObject::Set(i_isolate, table_object, index, element); } // WebAssembly.Table.type() -> TableType diff --git a/test/mjsunit/regress/wasm/regress-1273705.js b/test/mjsunit/regress/wasm/regress-1273705.js new file mode 100644 index 0000000000..e671b56d39 --- /dev/null +++ b/test/mjsunit/regress/wasm/regress-1273705.js @@ -0,0 +1,7 @@ +// Copyright 2021 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. + +const initial = function () {}; +const descriptor = {"element": "externref", "initial": 3}; +new WebAssembly.Table(descriptor, initial); diff --git a/test/mjsunit/wasm/externref-table.js b/test/mjsunit/wasm/externref-table.js index 4b9463781b..e481c329e8 100644 --- a/test/mjsunit/wasm/externref-table.js +++ b/test/mjsunit/wasm/externref-table.js @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --experimental-wasm-reftypes +// Flags: --experimental-wasm-typed-funcref --experimental-wasm-type-reflection d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js"); @@ -136,3 +136,69 @@ function getDummy(val) { table.set(1); assertEquals(table.get(1), undefined); })(); + +(function TestFunctionExternRefTableRoundtrip() { + // Test that + // - initialization, setting, and growing an externref table, and + // - (imported) externref globals + // preserve function references. + print(arguments.callee.name); + + const js_function = function (i) { return i + 1; }; + const wasm_js_function = new WebAssembly.Function( + {parameters:['i32', 'i32'], results: ['i32']}, + function(a, b) { return a * b; }) + + let extern_type = wasmRefType(kWasmExternRef); + + let builder = new WasmModuleBuilder(); + let imported_global = builder.addImportedGlobal('m', 'n', extern_type, false); + let global = builder.addGlobal(kWasmExternRef, true).exportAs('global'); + let table = builder.addTable(extern_type, 2, 10, + WasmInitExpr.GlobalGet(imported_global)) + builder.addFunction( + 'setup', makeSig([extern_type, extern_type], [])) + .addBody([ + kExprLocalGet, 0, kExprGlobalSet, global.index, + kExprI32Const, 1, kExprLocalGet, 0, kExprTableSet, table.index, + kExprLocalGet, 1, kExprI32Const, 1, kNumericPrefix, + kExprTableGrow, table.index, kExprDrop]) + .exportFunc(); + builder.addFunction('get', makeSig([kWasmI32], [kWasmExternRef])) + .addBody([kExprLocalGet, 0, kExprTableGet, table.index]) + .exportFunc(); + let instance = builder.instantiate({m : {n : js_function}}); + + instance.exports.setup(wasm_js_function, instance.exports.setup); + + assertEquals(instance.exports.global.value, wasm_js_function); + assertEquals(instance.exports.get(0), js_function); + assertEquals(instance.exports.get(1), wasm_js_function); + assertEquals(instance.exports.get(2), instance.exports.setup); +})(); + +(function TestFunctionExternRefTableRoundtrip2() { + // Test that initialization, setting, and growing an externref table in the JS + // API preserves function references. + print(arguments.callee.name); + + let builder = new WasmModuleBuilder(); + builder.addFunction('dummy', kSig_i_v) + .addBody([kExprI32Const, 0]) + .exportAs('dummy'); + let instance = builder.instantiate(); + const js_function = function (i) { return i + 1; }; + const wasm_js_function = new WebAssembly.Function( + {parameters:['i32', 'i32'], results: ['i32']}, + function(a, b) { return a * b; }) + + const argument = { "element": "externref", "initial": 3 }; + const table = new WebAssembly.Table(argument, js_function); + table.set(1, wasm_js_function); + table.set(2, instance.exports.dummy); + table.grow(1, wasm_js_function); + assertEquals(table.get(0), js_function); + assertEquals(table.get(1), wasm_js_function); + assertEquals(table.get(2), instance.exports.dummy); + assertEquals(table.get(3), wasm_js_function); +})();