[wasm] Remove bogus Isolate::wasm_caught_exception.

This removes the thread-local field in question. This side-channel for
the "caught exception" is not needed, we can just explicitly pass the
exception value to all support functions. Also, there is an inherent
problem with having this side-channel, as it will not be properly reset
when an exception handler ends up not rethrowing the exception.

R=ahaas@chromium.org
BUG=v8:8097

Change-Id: I2fdaff89f0eb318ce5a33bf56513165185547c1b
Reviewed-on: https://chromium-review.googlesource.com/1194063Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55517}

src/compiler/wasm-compiler.cc

@@ -2042,8 +2042,9 @@ Node* WasmGraphBuilder::Throw(uint32_t tag,
   Node* create_parameters[] = {
       BuildChangeUint31ToSmi(ConvertExceptionTagToRuntimeId(tag)),
       BuildChangeUint31ToSmi(Uint32Constant(encoded_size))};
-  BuildCallToRuntime(Runtime::kWasmThrowCreate, create_parameters,
-                     arraysize(create_parameters));
+  Node* except_obj =
+      BuildCallToRuntime(Runtime::kWasmThrowCreate, create_parameters,
+                         arraysize(create_parameters));
   uint32_t index = 0;
   const wasm::WasmExceptionSig* sig = exception->sig;
   MachineOperatorBuilder* m = mcgraph()->machine();
@@ -2054,7 +2055,7 @@ Node* WasmGraphBuilder::Throw(uint32_t tag,
         value = graph()->NewNode(m->BitcastFloat32ToInt32(), value);
         V8_FALLTHROUGH;
       case wasm::kWasmI32:
-        BuildEncodeException32BitValue(&index, value);
+        BuildEncodeException32BitValue(except_obj, &index, value);
         break;
       case wasm::kWasmF64:
         value = graph()->NewNode(m->BitcastFloat64ToInt64(), value);
@@ -2063,9 +2064,9 @@ Node* WasmGraphBuilder::Throw(uint32_t tag,
         Node* upper32 = graph()->NewNode(
             m->TruncateInt64ToInt32(),
             Binop(wasm::kExprI64ShrU, value, Int64Constant(32)));
-        BuildEncodeException32BitValue(&index, upper32);
+        BuildEncodeException32BitValue(except_obj, &index, upper32);
         Node* lower32 = graph()->NewNode(m->TruncateInt64ToInt32(), value);
-        BuildEncodeException32BitValue(&index, lower32);
+        BuildEncodeException32BitValue(except_obj, &index, lower32);
         break;
       }
       default:
@@ -2073,14 +2074,15 @@ Node* WasmGraphBuilder::Throw(uint32_t tag,
     }
   }
   DCHECK_EQ(encoded_size, index);
-  return BuildCallToRuntime(Runtime::kWasmThrow, nullptr, 0);
+  return BuildCallToRuntime(Runtime::kWasmThrow, &except_obj, 1);
 }
 
-void WasmGraphBuilder::BuildEncodeException32BitValue(uint32_t* index,
+void WasmGraphBuilder::BuildEncodeException32BitValue(Node* except_obj,
+                                                      uint32_t* index,
                                                       Node* value) {
   MachineOperatorBuilder* machine = mcgraph()->machine();
   Node* upper_parameters[] = {
-      BuildChangeUint31ToSmi(Int32Constant(*index)),
+      except_obj, BuildChangeUint31ToSmi(Int32Constant(*index)),
       BuildChangeUint31ToSmi(
           graph()->NewNode(machine->Word32Shr(), value, Int32Constant(16))),
   };
@@ -2088,7 +2090,7 @@ void WasmGraphBuilder::BuildEncodeException32BitValue(uint32_t* index,
                      arraysize(upper_parameters));
   ++(*index);
   Node* lower_parameters[] = {
-      BuildChangeUint31ToSmi(Int32Constant(*index)),
+      except_obj, BuildChangeUint31ToSmi(Int32Constant(*index)),
       BuildChangeUint31ToSmi(graph()->NewNode(machine->Word32And(), value,
                                               Int32Constant(0xFFFFu))),
   };
@@ -2109,9 +2111,9 @@ Node* WasmGraphBuilder::BuildDecodeException32BitValue(Node* const* values,
   return value;
 }
 
-Node* WasmGraphBuilder::Rethrow() {
+Node* WasmGraphBuilder::Rethrow(Node* except_obj) {
   SetNeedsStackCheck();
-  Node* result = BuildCallToRuntime(Runtime::kWasmThrow, nullptr, 0);
+  Node* result = BuildCallToRuntime(Runtime::kWasmThrow, &except_obj, 1);
   return result;
 }
 
@@ -2121,14 +2123,14 @@ Node* WasmGraphBuilder::ConvertExceptionTagToRuntimeId(uint32_t tag) {
   return Uint32Constant(tag);
 }
 
-Node* WasmGraphBuilder::GetExceptionRuntimeId() {
+Node* WasmGraphBuilder::GetExceptionRuntimeId(Node* except_obj) {
   SetNeedsStackCheck();
   return BuildChangeSmiToInt32(
-      BuildCallToRuntime(Runtime::kWasmGetExceptionRuntimeId, nullptr, 0));
+      BuildCallToRuntime(Runtime::kWasmGetExceptionRuntimeId, &except_obj, 1));
 }
 
 Node** WasmGraphBuilder::GetExceptionValues(
-    const wasm::WasmException* except_decl) {
+    Node* except_obj, const wasm::WasmException* except_decl) {
   // TODO(kschimpf): We need to move this code to the function-body-decoder.cc
   // in order to build landing-pad (exception) edges in case the runtime
   // call causes an exception.
@@ -2137,7 +2139,8 @@ Node** WasmGraphBuilder::GetExceptionValues(
   uint32_t encoded_size = GetExceptionEncodedSize(except_decl);
   Node** values = Buffer(encoded_size);
   for (uint32_t i = 0; i < encoded_size; ++i) {
-    Node* parameters[] = {BuildChangeUint31ToSmi(Uint32Constant(i))};
+    Node* parameters[] = {except_obj,
+                          BuildChangeUint31ToSmi(Uint32Constant(i))};
     values[i] = BuildCallToRuntime(Runtime::kWasmExceptionGetElement,
                                    parameters, arraysize(parameters));
   }

src/compiler/wasm-compiler.h

@@ -166,10 +166,11 @@ class WasmGraphBuilder {
   Node* GrowMemory(Node* input);
   Node* Throw(uint32_t tag, const wasm::WasmException* exception,
               const Vector<Node*> values);
-  Node* Rethrow();
+  Node* Rethrow(Node* except_obj);
   Node* ConvertExceptionTagToRuntimeId(uint32_t tag);
-  Node* GetExceptionRuntimeId();
-  Node** GetExceptionValues(const wasm::WasmException* except_decl);
+  Node* GetExceptionRuntimeId(Node* except_obj);
+  Node** GetExceptionValues(Node* except_obj,
+                            const wasm::WasmException* except_decl);
   bool IsPhiWithMerge(Node* phi, Node* merge);
   bool ThrowsException(Node* node, Node** if_success, Node** if_exception);
   void AppendToMerge(Node* merge, Node* from);
@@ -449,7 +450,8 @@ class WasmGraphBuilder {
   Node* BuildAsmjsStoreMem(MachineType type, Node* index, Node* val);
 
   uint32_t GetExceptionEncodedSize(const wasm::WasmException* exception) const;
-  void BuildEncodeException32BitValue(uint32_t* index, Node* value);
+  void BuildEncodeException32BitValue(Node* except_obj, uint32_t* index,
+                                      Node* value);
   Node* BuildDecodeException32BitValue(Node* const* values, uint32_t* index);
 
   Node** Realloc(Node* const* buffer, size_t old_count, size_t new_count) {

src/isolate-inl.h

@@ -58,17 +58,6 @@ bool Isolate::has_pending_exception() {
   return !thread_local_top_.pending_exception_->IsTheHole(this);
 }
 
-Object* Isolate::get_wasm_caught_exception() {
-  return thread_local_top_.wasm_caught_exception_;
-}
-
-void Isolate::set_wasm_caught_exception(Object* exception) {
-  thread_local_top_.wasm_caught_exception_ = exception;
-}
-
-void Isolate::clear_wasm_caught_exception() {
-  thread_local_top_.wasm_caught_exception_ = nullptr;
-}
 
 void Isolate::clear_pending_message() {
   thread_local_top_.pending_message_obj_ = ReadOnlyRoots(this).the_hole_value();

src/isolate.cc

@@ -161,7 +161,6 @@ void ThreadLocalTop::Initialize(Isolate* isolate) {
 }
 
 void ThreadLocalTop::Free() {
-  wasm_caught_exception_ = nullptr;
   // Match unmatched PopPromise calls.
   while (promise_on_stack_) isolate_->PopPromise();
 }
@@ -252,7 +251,6 @@ void Isolate::IterateThread(ThreadVisitor* v, char* t) {
 void Isolate::Iterate(RootVisitor* v, ThreadLocalTop* thread) {
   // Visit the roots from the top for a given thread.
   v->VisitRootPointer(Root::kTop, nullptr, &thread->pending_exception_);
-  v->VisitRootPointer(Root::kTop, nullptr, &thread->wasm_caught_exception_);
   v->VisitRootPointer(Root::kTop, nullptr, &thread->pending_message_obj_);
   v->VisitRootPointer(Root::kTop, nullptr,
                       bit_cast<Object**>(&(thread->context_)));
@@ -1313,7 +1311,7 @@ Object* Isolate::UnwindAndFindHandler() {
         // again. It was cleared above assuming the frame would be unwound.
         trap_handler::SetThreadInWasm();
 
-        set_wasm_caught_exception(exception);
+        // Gather information from the frame.
         wasm::WasmCode* wasm_code =
             wasm_engine()->code_manager()->LookupCode(frame->pc());
         return FoundHandler(nullptr, wasm_code->instruction_start(), offset,

src/isolate.h

@@ -435,9 +435,6 @@ class ThreadLocalTop BASE_EMBEDDED {
   Context* context_ = nullptr;
   ThreadId thread_id_ = ThreadId::Invalid();
   Object* pending_exception_ = nullptr;
-  // TODO(kschimpf): Change this to a stack of caught exceptions (rather than
-  // just innermost catching try block).
-  Object* wasm_caught_exception_ = nullptr;
 
   // Communication channel between Isolate::FindHandler and the CEntry.
   Context* pending_handler_context_ = nullptr;
@@ -716,11 +713,6 @@ class Isolate : private HiddenFactory {
   inline void set_pending_exception(Object* exception_obj);
   inline void clear_pending_exception();
 
-  // Interface to wasm caught exception.
-  inline Object* get_wasm_caught_exception();
-  inline void set_wasm_caught_exception(Object* exception);
-  inline void clear_wasm_caught_exception();
-
   bool AreWasmThreadsEnabled(Handle<Context> context);
 
   THREAD_LOCAL_TOP_ADDRESS(Object*, pending_exception)

src/runtime/runtime-wasm.cc

@@ -116,13 +116,12 @@ RUNTIME_FUNCTION(Runtime_WasmThrowTypeError) {
 RUNTIME_FUNCTION(Runtime_WasmThrowCreate) {
   // TODO(kschimpf): Can this be replaced with equivalent TurboFan code/calls.
   HandleScope scope(isolate);
+  DCHECK_EQ(2, args.length());
   DCHECK_NULL(isolate->context());
   isolate->set_context(GetNativeContextFromWasmInstanceOnStackTop(isolate));
-  DCHECK_EQ(2, args.length());
   Handle<Object> exception = isolate->factory()->NewWasmRuntimeError(
       static_cast<MessageTemplate::Template>(
           MessageTemplate::kWasmExceptionError));
-  isolate->set_wasm_caught_exception(*exception);
   CONVERT_ARG_HANDLE_CHECKED(Smi, id, 0);
   CHECK(!JSReceiver::SetProperty(isolate, exception,
                                  isolate->factory()->InternalizeUtf8String(
@@ -137,27 +136,27 @@ RUNTIME_FUNCTION(Runtime_WasmThrowCreate) {
                                      wasm::WasmException::kRuntimeValuesStr),
                                  values, LanguageMode::kStrict)
              .is_null());
-  return ReadOnlyRoots(isolate).undefined_value();
+  return *exception;
 }
 
 RUNTIME_FUNCTION(Runtime_WasmThrow) {
   // TODO(kschimpf): Can this be replaced with equivalent TurboFan code/calls.
   HandleScope scope(isolate);
+  DCHECK_EQ(1, args.length());
   DCHECK_NULL(isolate->context());
   isolate->set_context(GetNativeContextFromWasmInstanceOnStackTop(isolate));
-  DCHECK_EQ(0, args.length());
-  Handle<Object> exception(isolate->get_wasm_caught_exception(), isolate);
-  CHECK(!exception.is_null());
-  isolate->clear_wasm_caught_exception();
-  return isolate->Throw(*exception);
+  CONVERT_ARG_HANDLE_CHECKED(Object, except_obj, 0);
+  CHECK(!except_obj.is_null());
+  return isolate->Throw(*except_obj);
 }
 
 RUNTIME_FUNCTION(Runtime_WasmGetExceptionRuntimeId) {
   // TODO(kschimpf): Can this be replaced with equivalent TurboFan code/calls.
   HandleScope scope(isolate);
+  DCHECK_EQ(1, args.length());
   DCHECK_NULL(isolate->context());
   isolate->set_context(GetNativeContextFromWasmInstanceOnStackTop(isolate));
-  Handle<Object> except_obj(isolate->get_wasm_caught_exception(), isolate);
+  CONVERT_ARG_HANDLE_CHECKED(Object, except_obj, 0);
   if (!except_obj.is_null() && except_obj->IsJSReceiver()) {
     Handle<JSReceiver> exception(JSReceiver::cast(*except_obj), isolate);
     Handle<Object> tag;
@@ -176,10 +175,10 @@ RUNTIME_FUNCTION(Runtime_WasmGetExceptionRuntimeId) {
 RUNTIME_FUNCTION(Runtime_WasmExceptionGetElement) {
   // TODO(kschimpf): Can this be replaced with equivalent TurboFan code/calls.
   HandleScope scope(isolate);
+  DCHECK_EQ(2, args.length());
   DCHECK_NULL(isolate->context());
   isolate->set_context(GetNativeContextFromWasmInstanceOnStackTop(isolate));
-  DCHECK_EQ(1, args.length());
-  Handle<Object> except_obj(isolate->get_wasm_caught_exception(), isolate);
+  CONVERT_ARG_HANDLE_CHECKED(Object, except_obj, 0);
   if (!except_obj.is_null() && except_obj->IsJSReceiver()) {
     Handle<JSReceiver> exception(JSReceiver::cast(*except_obj), isolate);
     Handle<Object> values_obj;
@@ -190,7 +189,7 @@ RUNTIME_FUNCTION(Runtime_WasmExceptionGetElement) {
       if (values_obj->IsJSTypedArray()) {
         Handle<JSTypedArray> values = Handle<JSTypedArray>::cast(values_obj);
         CHECK_EQ(values->type(), kExternalUint16Array);
-        CONVERT_SMI_ARG_CHECKED(index, 0);
+        CONVERT_SMI_ARG_CHECKED(index, 1);
         CHECK_LT(index, Smi::ToInt(values->length()));
         auto* vals =
             reinterpret_cast<uint16_t*>(values->GetBuffer()->backing_store());
@@ -204,10 +203,10 @@ RUNTIME_FUNCTION(Runtime_WasmExceptionGetElement) {
 RUNTIME_FUNCTION(Runtime_WasmExceptionSetElement) {
   // TODO(kschimpf): Can this be replaced with equivalent TurboFan code/calls.
   HandleScope scope(isolate);
-  DCHECK_EQ(2, args.length());
+  DCHECK_EQ(3, args.length());
   DCHECK_NULL(isolate->context());
   isolate->set_context(GetNativeContextFromWasmInstanceOnStackTop(isolate));
-  Handle<Object> except_obj(isolate->get_wasm_caught_exception(), isolate);
+  CONVERT_ARG_HANDLE_CHECKED(Object, except_obj, 0);
   if (!except_obj.is_null() && except_obj->IsJSReceiver()) {
     Handle<JSReceiver> exception(JSReceiver::cast(*except_obj), isolate);
     Handle<Object> values_obj;
@@ -218,9 +217,9 @@ RUNTIME_FUNCTION(Runtime_WasmExceptionSetElement) {
       if (values_obj->IsJSTypedArray()) {
         Handle<JSTypedArray> values = Handle<JSTypedArray>::cast(values_obj);
         CHECK_EQ(values->type(), kExternalUint16Array);
-        CONVERT_SMI_ARG_CHECKED(index, 0);
+        CONVERT_SMI_ARG_CHECKED(index, 1);
         CHECK_LT(index, Smi::ToInt(values->length()));
-        CONVERT_SMI_ARG_CHECKED(value, 1);
+        CONVERT_SMI_ARG_CHECKED(value, 2);
         auto* vals =
             reinterpret_cast<uint16_t*>(values->GetBuffer()->backing_store());
         vals[index] = static_cast<uint16_t>(value);

src/runtime/runtime.h

@@ -557,13 +557,13 @@ namespace internal {
 #define FOR_EACH_INTRINSIC_WASM(F)   \
   F(ThrowWasmError, 1, 1)            \
   F(ThrowWasmStackOverflow, 0, 1)    \
-  F(WasmExceptionGetElement, 1, 1)   \
-  F(WasmExceptionSetElement, 2, 1)   \
-  F(WasmGetExceptionRuntimeId, 0, 1) \
+  F(WasmExceptionGetElement, 2, 1)   \
+  F(WasmExceptionSetElement, 3, 1)   \
+  F(WasmGetExceptionRuntimeId, 1, 1) \
   F(WasmGrowMemory, 2, 1)            \
   F(WasmRunInterpreter, 2, 1)        \
   F(WasmStackGuard, 0, 1)            \
-  F(WasmThrow, 0, 1)                 \
+  F(WasmThrow, 1, 1)                 \
   F(WasmThrowCreate, 2, 1)           \
   F(WasmThrowTypeError, 0, 1)        \
   F(WasmCompileLazy, 2, 1)

src/wasm/function-body-decoder.cc

@@ -427,12 +427,13 @@ class WasmGraphBuildingInterface {
                       const ExceptionIndexImmediate<validate>& imm,
                       Control* block, Vector<Value> values) {
     DCHECK(block->is_try_catch());
+    TFNode* exception = block->try_info->exception;
     current_catch_ = block->previous_catch;
     SsaEnv* catch_env = block->try_info->catch_env;
     SetEnv(catch_env);
 
     TFNode* compare_i32 = nullptr;
-    if (block->try_info->exception == nullptr) {
+    if (exception == nullptr) {
       // Catch not applicable, no possible throws in the try
       // block. Create dummy code so that body of catch still
       // compiles. Note: This only happens because the current
@@ -443,7 +444,7 @@ class WasmGraphBuildingInterface {
       compare_i32 = BUILD(Int32Constant, 0);
     } else {
       // Get the exception and see if wanted exception.
-      TFNode* caught_tag = BUILD(GetExceptionRuntimeId);
+      TFNode* caught_tag = BUILD(GetExceptionRuntimeId, exception);
       TFNode* exception_tag = BUILD(ConvertExceptionTagToRuntimeId, imm.index);
       compare_i32 = BUILD(Binop, kExprI32Eq, caught_tag, exception_tag);
     }
@@ -460,13 +461,13 @@ class WasmGraphBuildingInterface {
     // TODO(kschimpf): Generalize to allow more catches. Will force
     // moving no_catch code to END opcode.
     SetEnv(if_no_catch_env);
-    BUILD(Rethrow);
+    BUILD(Rethrow, exception);
     Unreachable(decoder);
     EndControl(decoder, block);
 
     SetEnv(if_catch_env);
 
-    if (block->try_info->exception == nullptr) {
+    if (exception == nullptr) {
       // No caught value, make up filler nodes so that catch block still
       // compiles.
       for (Value& value : values) {
@@ -475,7 +476,8 @@ class WasmGraphBuildingInterface {
     } else {
       // TODO(kschimpf): Can't use BUILD() here, GetExceptionValues() returns
       // TFNode** rather than TFNode*. Fix to add landing pads.
-      TFNode** caught_values = builder_->GetExceptionValues(imm.exception);
+      TFNode** caught_values =
+          builder_->GetExceptionValues(exception, imm.exception);
       for (size_t i = 0, e = values.size(); i < e; ++i) {
         values[i].node = caught_values[i];
       }