From cae9aaeb329e225391ba46f4dfac2479fa8aaa76 Mon Sep 17 00:00:00 2001 From: Michael Starzinger Date: Thu, 26 Sep 2019 11:20:27 +0200 Subject: [PATCH] [wasm] Improve {WasmExceptionPackage} type safety. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This uses Handle where applicable to increase type safety. Note that {WasmExceptionPackage} is not a full-fledged instance type though. The {HeapObject::IsWasmExceptionPackage} predicate is an approximation because a precise version could only be implemented using handlified code performing a property lookup. R=clemensb@chromium.org Change-Id: I061e3eea201a0e9909ba67ae33db81d14aaefe4b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1477673 Commit-Queue: Michael Starzinger Reviewed-by: Clemens Backes [né Hammacher] Cr-Commit-Position: refs/heads/master@{#63987} --- src/compiler/code-assembler.h | 1 + src/objects/object-list-macros.h | 1 + src/objects/objects-inl.h | 6 +++++ src/runtime/runtime-test.cc | 4 ++-- src/runtime/runtime-wasm.cc | 14 ++++++++++-- src/wasm/wasm-interpreter.cc | 12 +++++----- src/wasm/wasm-objects-inl.h | 4 ++++ src/wasm/wasm-objects.cc | 38 ++++++++++++++------------------ src/wasm/wasm-objects.h | 18 ++++++++------- 9 files changed, 59 insertions(+), 39 deletions(-) diff --git a/src/compiler/code-assembler.h b/src/compiler/code-assembler.h index 4b29d247a9..700a063320 100644 --- a/src/compiler/code-assembler.h +++ b/src/compiler/code-assembler.h @@ -131,6 +131,7 @@ class Undetectable; class UniqueName; class WasmCapiFunctionData; class WasmExceptionObject; +class WasmExceptionPackage; class WasmExceptionTag; class WasmExportedFunctionData; class WasmGlobalObject; diff --git a/src/objects/object-list-macros.h b/src/objects/object-list-macros.h index 1bcda344d8..b1654b545e 100644 --- a/src/objects/object-list-macros.h +++ b/src/objects/object-list-macros.h @@ -226,6 +226,7 @@ class ZoneForwardList; V(Undetectable) \ V(UniqueName) \ V(WasmExceptionObject) \ + V(WasmExceptionPackage) \ V(WasmGlobalObject) \ V(WasmInstanceObject) \ V(WasmMemoryObject) \ diff --git a/src/objects/objects-inl.h b/src/objects/objects-inl.h index df00e0e7dc..d58e3f8f07 100644 --- a/src/objects/objects-inl.h +++ b/src/objects/objects-inl.h @@ -411,6 +411,12 @@ DEF_GETTER(HeapObject, IsSmallOrderedHashTable, bool) { IsSmallOrderedNameDictionary(isolate); } +DEF_GETTER(HeapObject, IsWasmExceptionPackage, bool) { + // It is not possible to check for the existence of certain properties on the + // underlying {JSReceiver} here because that requires calling handlified code. + return IsJSReceiver(isolate); +} + bool Object::IsPrimitive() const { if (IsSmi()) return true; HeapObject this_heap_object = HeapObject::cast(*this); diff --git a/src/runtime/runtime-test.cc b/src/runtime/runtime-test.cc index e45cdcddf6..48845fc60b 100644 --- a/src/runtime/runtime-test.cc +++ b/src/runtime/runtime-test.cc @@ -1049,7 +1049,7 @@ RUNTIME_FUNCTION(Runtime_GetWasmRecoveredTrapCount) { RUNTIME_FUNCTION(Runtime_GetWasmExceptionId) { HandleScope scope(isolate); DCHECK_EQ(2, args.length()); - CONVERT_ARG_HANDLE_CHECKED(JSReceiver, exception, 0); + CONVERT_ARG_HANDLE_CHECKED(WasmExceptionPackage, exception, 0); CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 1); Handle tag = WasmExceptionPackage::GetExceptionTag(isolate, exception); @@ -1065,7 +1065,7 @@ RUNTIME_FUNCTION(Runtime_GetWasmExceptionId) { RUNTIME_FUNCTION(Runtime_GetWasmExceptionValues) { HandleScope scope(isolate); DCHECK_EQ(1, args.length()); - CONVERT_ARG_HANDLE_CHECKED(JSReceiver, exception, 0); + CONVERT_ARG_HANDLE_CHECKED(WasmExceptionPackage, exception, 0); Handle values_obj = WasmExceptionPackage::GetExceptionValues(isolate, exception); CHECK(values_obj->IsFixedArray()); // Only called with correct input. diff --git a/src/runtime/runtime-wasm.cc b/src/runtime/runtime-wasm.cc index 30ae996e04..241fb020ad 100644 --- a/src/runtime/runtime-wasm.cc +++ b/src/runtime/runtime-wasm.cc @@ -150,7 +150,12 @@ RUNTIME_FUNCTION(Runtime_WasmExceptionGetTag) { CONVERT_ARG_CHECKED(Object, except_obj_raw, 0); // TODO(mstarzinger): Manually box because parameters are not visited yet. Handle except_obj(except_obj_raw, isolate); - return *WasmExceptionPackage::GetExceptionTag(isolate, except_obj); + if (!except_obj->IsWasmExceptionPackage(isolate)) { + return ReadOnlyRoots(isolate).undefined_value(); + } + Handle exception = + Handle::cast(except_obj); + return *WasmExceptionPackage::GetExceptionTag(isolate, exception); } RUNTIME_FUNCTION(Runtime_WasmExceptionGetValues) { @@ -162,7 +167,12 @@ RUNTIME_FUNCTION(Runtime_WasmExceptionGetValues) { CONVERT_ARG_CHECKED(Object, except_obj_raw, 0); // TODO(mstarzinger): Manually box because parameters are not visited yet. Handle except_obj(except_obj_raw, isolate); - return *WasmExceptionPackage::GetExceptionValues(isolate, except_obj); + if (!except_obj->IsWasmExceptionPackage(isolate)) { + return ReadOnlyRoots(isolate).undefined_value(); + } + Handle exception = + Handle::cast(except_obj); + return *WasmExceptionPackage::GetExceptionValues(isolate, exception); } RUNTIME_FUNCTION(Runtime_WasmRunInterpreter) { diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 3abacdcf6f..099ffcb817 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/src/wasm/wasm-interpreter.cc @@ -2727,7 +2727,7 @@ class ThreadImpl { WasmExceptionTag::cast(instance_object_->exceptions_table().get(index)), isolate_); uint32_t encoded_size = WasmExceptionPackage::GetEncodedSize(exception); - Handle exception_object = + Handle exception_object = WasmExceptionPackage::New(isolate_, exception_tag, encoded_size); Handle encoded_values = Handle::cast( WasmExceptionPackage::GetExceptionValues(isolate_, exception_object)); @@ -2796,8 +2796,9 @@ class ThreadImpl { // Determines whether the given exception has a tag matching the expected tag // for the given index within the exception table of the current instance. bool MatchingExceptionTag(Handle exception_object, uint32_t index) { - Handle caught_tag = - WasmExceptionPackage::GetExceptionTag(isolate_, exception_object); + if (!exception_object->IsWasmExceptionPackage(isolate_)) return false; + Handle caught_tag = WasmExceptionPackage::GetExceptionTag( + isolate_, Handle::cast(exception_object)); Handle expected_tag = handle(instance_object_->exceptions_table().get(index), isolate_); DCHECK(expected_tag->IsWasmExceptionTag()); @@ -2824,8 +2825,9 @@ class ThreadImpl { // the encoded values match the expected signature of the exception. void DoUnpackException(const WasmException* exception, Handle exception_object) { - Handle encoded_values = Handle::cast( - WasmExceptionPackage::GetExceptionValues(isolate_, exception_object)); + Handle encoded_values = + Handle::cast(WasmExceptionPackage::GetExceptionValues( + isolate_, Handle::cast(exception_object))); // Decode the exception values from the given exception package and push // them onto the operand stack. This encoding has to be in sync with other // backends so that exceptions can be passed between them. diff --git a/src/wasm/wasm-objects-inl.h b/src/wasm/wasm-objects-inl.h index fa58dd0f3a..bfe7e3c6b7 100644 --- a/src/wasm/wasm-objects-inl.h +++ b/src/wasm/wasm-objects-inl.h @@ -309,6 +309,10 @@ ACCESSORS(WasmExceptionObject, serialized_signature, PodArray, kSerializedSignatureOffset) ACCESSORS(WasmExceptionObject, exception_tag, HeapObject, kExceptionTagOffset) +// WasmExceptionPackage +OBJECT_CONSTRUCTORS_IMPL(WasmExceptionPackage, JSReceiver) +CAST_ACCESSOR(WasmExceptionPackage) + // WasmExportedFunction WasmExportedFunction::WasmExportedFunction(Address ptr) : JSFunction(ptr) { SLOW_DCHECK(IsWasmExportedFunction(*this)); diff --git a/src/wasm/wasm-objects.cc b/src/wasm/wasm-objects.cc index 7619f3c11a..9b375b7e3d 100644 --- a/src/wasm/wasm-objects.cc +++ b/src/wasm/wasm-objects.cc @@ -1930,7 +1930,7 @@ bool WasmCapiFunction::IsSignatureEqual(const wasm::FunctionSig* sig) const { } // static -Handle WasmExceptionPackage::New( +Handle WasmExceptionPackage::New( Isolate* isolate, Handle exception_tag, int size) { Handle exception = isolate->factory()->NewWasmRuntimeError( MessageTemplate::kWasmExceptionError); @@ -1945,37 +1945,31 @@ Handle WasmExceptionPackage::New( values, StoreOrigin::kMaybeKeyed, Just(ShouldThrow::kThrowOnError)) .is_null()); - return Handle::cast(exception); + return Handle::cast(exception); } // static Handle WasmExceptionPackage::GetExceptionTag( - Isolate* isolate, Handle exception_object) { - if (exception_object->IsJSReceiver()) { - Handle exception = Handle::cast(exception_object); - Handle tag; - if (JSReceiver::GetProperty(isolate, exception, - isolate->factory()->wasm_exception_tag_symbol()) - .ToHandle(&tag)) { - return tag; - } + Isolate* isolate, Handle exception_package) { + Handle tag; + if (JSReceiver::GetProperty(isolate, exception_package, + isolate->factory()->wasm_exception_tag_symbol()) + .ToHandle(&tag)) { + return tag; } return ReadOnlyRoots(isolate).undefined_value_handle(); } // static Handle WasmExceptionPackage::GetExceptionValues( - Isolate* isolate, Handle exception_object) { - if (exception_object->IsJSReceiver()) { - Handle exception = Handle::cast(exception_object); - Handle values; - if (JSReceiver::GetProperty( - isolate, exception, - isolate->factory()->wasm_exception_values_symbol()) - .ToHandle(&values)) { - DCHECK(values->IsFixedArray()); - return values; - } + Isolate* isolate, Handle exception_package) { + Handle values; + if (JSReceiver::GetProperty( + isolate, exception_package, + isolate->factory()->wasm_exception_values_symbol()) + .ToHandle(&values)) { + DCHECK(values->IsFixedArray()); + return values; } return ReadOnlyRoots(isolate).undefined_value_handle(); } diff --git a/src/wasm/wasm-objects.h b/src/wasm/wasm-objects.h index 086aa61a36..9957a75563 100644 --- a/src/wasm/wasm-objects.h +++ b/src/wasm/wasm-objects.h @@ -626,20 +626,22 @@ class WasmExceptionObject : public JSObject { // A Wasm exception that has been thrown out of Wasm code. class WasmExceptionPackage : public JSReceiver { public: - // TODO(mstarzinger): Ideally this interface would use {WasmExceptionPackage} - // instead of {JSReceiver} throughout. For now a type-check implies doing a - // property lookup however, which would result in casts being handlified. - static Handle New(Isolate* isolate, - Handle exception_tag, - int encoded_size); + static Handle New( + Isolate* isolate, Handle exception_tag, + int encoded_size); // The below getters return {undefined} in case the given exception package // does not carry the requested values (i.e. is of a different type). - static Handle GetExceptionTag(Isolate*, Handle exception); - static Handle GetExceptionValues(Isolate*, Handle exception); + static Handle GetExceptionTag( + Isolate* isolate, Handle exception_package); + static Handle GetExceptionValues( + Isolate* isolate, Handle exception_package); // Determines the size of the array holding all encoded exception values. static uint32_t GetEncodedSize(const wasm::WasmException* exception); + + DECL_CAST(WasmExceptionPackage) + OBJECT_CONSTRUCTORS(WasmExceptionPackage, JSReceiver); }; // A Wasm function that is wrapped and exported to JavaScript.