From bcc055c1589f6d3506c646fd4ef66636e1e56476 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Mon, 23 Mar 2020 11:46:51 +0100 Subject: [PATCH] [wasm] Make br_on_exn trap on nullptr The behaviour was clarified in the spec: https://github.com/WebAssembly/exception-handling/pull/97 br_on_exn (and also rethrow, which will be added in another CL) should trap on nullptr. This CL implements this by an explicit check on each br_on_exn (within {GetExceptionTag}). This check will be redundant if several br_on_exn follow each other. Since also the runtime call for {GetExceptionTag} is redundant, and also the fact that we do a runtime call is suboptimal, I consider the whole implementation prototypical for now anyway. R=jkummerow@chromium.org CC=aheejin@chromium.org Bug: v8:10128 Change-Id: I234c3183f93fe0884aadd2ab6dbd6c2b7a07c660 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2113381 Commit-Queue: Clemens Backes Reviewed-by: Jakob Kummerow Cr-Commit-Position: refs/heads/master@{#66826} --- src/builtins/builtins-definitions.h | 1 + src/common/globals.h | 3 +- src/common/message-template.h | 1 + src/compiler/wasm-compiler.cc | 6 ++- src/compiler/wasm-compiler.h | 2 +- src/wasm/graph-builder-interface.cc | 3 +- src/wasm/wasm-interpreter.cc | 1 + test/mjsunit/mjsunit.js | 11 +++--- test/mjsunit/wasm/exceptions-anyref.js | 50 ++++++++++++++++++++++++ test/mjsunit/wasm/wasm-module-builder.js | 4 +- 10 files changed, 71 insertions(+), 11 deletions(-) diff --git a/src/builtins/builtins-definitions.h b/src/builtins/builtins-definitions.h index 939691ba70..8e6084aa7b 100644 --- a/src/builtins/builtins-definitions.h +++ b/src/builtins/builtins-definitions.h @@ -878,6 +878,7 @@ namespace internal { TFS(ThrowWasmTrapDataSegmentDropped) \ TFS(ThrowWasmTrapElemSegmentDropped) \ TFS(ThrowWasmTrapTableOutOfBounds) \ + TFS(ThrowWasmTrapBrOnExnNullRef) \ \ /* WeakMap */ \ TFJ(WeakMapConstructor, kDontAdaptArgumentsSentinel) \ diff --git a/src/common/globals.h b/src/common/globals.h index 40912c7196..bced323586 100644 --- a/src/common/globals.h +++ b/src/common/globals.h @@ -1596,7 +1596,8 @@ enum class LoadSensitivity { V(TrapFuncSigMismatch) \ V(TrapDataSegmentDropped) \ V(TrapElemSegmentDropped) \ - V(TrapTableOutOfBounds) + V(TrapTableOutOfBounds) \ + V(TrapBrOnExnNullRef) enum KeyedAccessLoadMode { STANDARD_LOAD, diff --git a/src/common/message-template.h b/src/common/message-template.h index 1690a9f8b0..e22a0c8021 100644 --- a/src/common/message-template.h +++ b/src/common/message-template.h @@ -551,6 +551,7 @@ namespace internal { T(WasmTrapDataSegmentDropped, "data segment has been dropped") \ T(WasmTrapElemSegmentDropped, "element segment has been dropped") \ T(WasmTrapTableOutOfBounds, "table access out of bounds") \ + T(WasmTrapBrOnExnNullRef, "br_on_exn on nullref value") \ T(WasmExceptionError, "wasm exception") \ /* Asm.js validation related */ \ T(AsmJsInvalid, "Invalid asm.js: %") \ diff --git a/src/compiler/wasm-compiler.cc b/src/compiler/wasm-compiler.cc index 8002d2736c..368c878132 100644 --- a/src/compiler/wasm-compiler.cc +++ b/src/compiler/wasm-compiler.cc @@ -2150,8 +2150,10 @@ Node* WasmGraphBuilder::LoadExceptionTagFromTable(uint32_t exception_index) { return tag; } -Node* WasmGraphBuilder::GetExceptionTag(Node* except_obj) { - needs_stack_check_ = true; +Node* WasmGraphBuilder::GetExceptionTag(Node* except_obj, + wasm::WasmCodePosition position) { + TrapIfTrue(wasm::kTrapBrOnExnNullRef, gasm_->WordEqual(RefNull(), except_obj), + position); return BuildCallToRuntime(Runtime::kWasmExceptionGetTag, &except_obj, 1); } diff --git a/src/compiler/wasm-compiler.h b/src/compiler/wasm-compiler.h index e405a3c910..4e59136ae2 100644 --- a/src/compiler/wasm-compiler.h +++ b/src/compiler/wasm-compiler.h @@ -213,7 +213,7 @@ class WasmGraphBuilder { Node* Rethrow(Node* except_obj); Node* ExceptionTagEqual(Node* caught_tag, Node* expected_tag); Node* LoadExceptionTagFromTable(uint32_t exception_index); - Node* GetExceptionTag(Node* except_obj); + Node* GetExceptionTag(Node* except_obj, wasm::WasmCodePosition); Node* GetExceptionValues(Node* except_obj, const wasm::WasmException* exception, Vector values_out); diff --git a/src/wasm/graph-builder-interface.cc b/src/wasm/graph-builder-interface.cc index 4baba6e891..5f73f27200 100644 --- a/src/wasm/graph-builder-interface.cc +++ b/src/wasm/graph-builder-interface.cc @@ -492,7 +492,8 @@ class WasmGraphBuildingInterface { TFNode* if_no_match = nullptr; // Get the exception tag and see if it matches the expected one. - TFNode* caught_tag = BUILD(GetExceptionTag, exception.node); + TFNode* caught_tag = + BUILD(GetExceptionTag, exception.node, decoder->position()); TFNode* exception_tag = BUILD(LoadExceptionTagFromTable, imm.index); TFNode* compare = BUILD(ExceptionTagEqual, caught_tag, exception_tag); BUILD(BranchNoHint, compare, &if_match, &if_no_match); diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 3f9f48fe63..f7ba08234c 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/src/wasm/wasm-interpreter.cc @@ -3132,6 +3132,7 @@ class ThreadImpl { HandleScope handle_scope(isolate_); // Avoid leaking handles. WasmValue ex = Pop(); Handle exception = ex.to_anyref(); + if (exception->IsNull()) return DoTrap(kTrapBrOnExnNullRef, pc); if (MatchingExceptionTag(exception, imm.index.index)) { imm.index.exception = &module()->exceptions[imm.index.index]; DoUnpackException(imm.index.exception, exception); diff --git a/test/mjsunit/mjsunit.js b/test/mjsunit/mjsunit.js index fd45ebacbd..58dcd6c9ed 100644 --- a/test/mjsunit/mjsunit.js +++ b/test/mjsunit/mjsunit.js @@ -548,13 +548,14 @@ var prettyPrinted; assertInstanceof = function assertInstanceof(obj, type) { if (!(obj instanceof type)) { var actualTypeName = null; - var actualConstructor = Object.getPrototypeOf(obj).constructor; - if (typeof actualConstructor === "function") { + var actualConstructor = obj && Object.getPrototypeOf(obj).constructor; + if (typeof actualConstructor === 'function') { actualTypeName = actualConstructor.name || String(actualConstructor); } - failWithMessage("Object <" + prettyPrinted(obj) + "> is not an instance of <" + - (type.name || type) + ">" + - (actualTypeName ? " but of <" + actualTypeName + ">" : "")); + failWithMessage( + 'Object <' + prettyPrinted(obj) + '> is not an instance of <' + + (type.name || type) + '>' + + (actualTypeName ? ' but of <' + actualTypeName + '>' : '')); } }; diff --git a/test/mjsunit/wasm/exceptions-anyref.js b/test/mjsunit/wasm/exceptions-anyref.js index b18b11aac0..074b63a44c 100644 --- a/test/mjsunit/wasm/exceptions-anyref.js +++ b/test/mjsunit/wasm/exceptions-anyref.js @@ -146,6 +146,7 @@ load("test/mjsunit/wasm/exceptions-utils.js"); // Test storing and loading to/from an exception type table. (function TestTableExnRef() { + print(arguments.callee.name); let kSig_e_i = makeSig([kWasmI32], [kWasmExnRef]); let kSig_v_ie = makeSig([kWasmI32, kWasmExnRef], []); let builder = new WasmModuleBuilder(); @@ -182,3 +183,52 @@ load("test/mjsunit/wasm/exceptions-utils.js"); assertSame(e0, instance.exports.table.get(0)); assertSame(e1, instance.exports.table.get(1)); })(); + +// 'br_on_exn' on a null-ref value should trap. +(function TestBrOnExnNullRefSimple() { + print(arguments.callee.name); + let builder = new WasmModuleBuilder(); + let except = builder.addException(kSig_v_r); + builder.addFunction('br_on_exn_nullref', kSig_v_v) + .addBody([ + kExprRefNull, + kExprBrOnExn, 0, except, + kExprDrop + ]).exportFunc(); + let instance = builder.instantiate(); + + assertTraps(kTrapBrOnExnNullRef, () => instance.exports.br_on_exn_nullref()); +})(); + +(function TestBrOnExnNullRefFromJS() { + print(arguments.callee.name); + let builder = new WasmModuleBuilder(); + let except = builder.addException(kSig_v_i); + let imp = builder.addImport('imp', 'ort', kSig_i_i); + let kConstant0 = 11; + let kNoMatch = 13; + builder.addFunction('call_import', kSig_i_i) + .addBody([ + kExprTry, kWasmI32, + kExprLocalGet, 0, + kExprCallFunction, imp, + kExprCatch, + kExprBrOnExn, 0, except, + kExprDrop, + kExprI32Const, kNoMatch, + kExprEnd + ]).exportFunc(); + let instance; + function js_import(i) { + if (i == 0) return kConstant0; // Will return kConstant0. + if (i == 1) throw new Error('1'); // Will not match. + if (i == 2) throw null; // Will trap. + throw undefined; // Will not match. + } + instance = builder.instantiate({imp: {ort: js_import}}); + + assertEquals(kConstant0, instance.exports.call_import(0)); + assertEquals(kNoMatch, instance.exports.call_import(1)); + assertTraps(kTrapBrOnExnNullRef, () => instance.exports.call_import(2)); + assertEquals(kNoMatch, instance.exports.call_import(3)); +})(); diff --git a/test/mjsunit/wasm/wasm-module-builder.js b/test/mjsunit/wasm/wasm-module-builder.js index 8f30a39a26..a05a3daacc 100644 --- a/test/mjsunit/wasm/wasm-module-builder.js +++ b/test/mjsunit/wasm/wasm-module-builder.js @@ -501,6 +501,7 @@ let kTrapUnalignedAccess = 9; let kTrapDataSegmentDropped = 10; let kTrapElemSegmentDropped = 11; let kTrapTableOutOfBounds = 12; +let kTrapBrOnExnNullRef = 13; let kTrapMsgs = [ "unreachable", @@ -515,7 +516,8 @@ let kTrapMsgs = [ "operation does not support unaligned accesses", "data segment has been dropped", "element segment has been dropped", - "table access out of bounds" + "table access out of bounds", + "br_on_exn on nullref value" ]; function assertTraps(trap, code) {