[Exceptions] Fix cross-module Tag handling in interpreter (#7955)

Make ExnData store a `Tag*`, allowing proper identification.

Like FuncData etc., move that logic into the interpreter.

Author: kripken

scripts/test/shared.py

@@ -442,7 +442,6 @@ def get_tests(test_dir, extensions=[], recursive=False):
     'instance.wast',  # Requires wast `module definition` support
     'table64.wast',   # Requires wast `module definition` support
     'table_grow.wast',  # Incorrect table linking semantics in interpreter
-    'try_catch.wast',  # Incorrect imported tag semantics in interpreter
     'tag.wast',      # Non-empty tag results allowed by stack switching
     'try_table.wast',  # Requires try_table interpretation
     'local_init.wast',  # Requires local validation to respect unnamed blocks

src/literal.h

@@ -785,18 +785,6 @@ struct GCData {
     : type(type), values(std::move(values)), desc(desc) {}
 };
 
-// The data of a (ref exn) literal.
-struct ExnData {
-  // The tag of this exn data.
-  // TODO: handle cross-module calls using something other than a Name here.
-  Name tag;
-
-  // The payload of this exn data.
-  Literals payload;
-
-  ExnData(Name tag, Literals payload) : tag(tag), payload(payload) {}
-};
-
 } // namespace wasm
 
 namespace std {

src/shell-interface.h

@@ -165,6 +165,10 @@ struct ShellExternalInterface : ModuleRunner::ExternalInterface {
             << "." << import->name.str;
   }
 
+  Tag* getImportedTag(Tag* tag) override {
+    WASM_UNREACHABLE("missing imported tag");
+  }
+
   int8_t load8s(Address addr, Name memoryName) override {
     auto it = memories.find(memoryName);
     assert(it != memories.end());

src/tools/execution-results.h

@@ -42,13 +42,11 @@ struct LoggingExternalInterface : public ShellExternalInterface {
   Name exportedTable;
   Module& wasm;
 
-  // The name of the imported fuzzing tag for wasm.
-  Name wasmTag;
+  // The imported fuzzing tag for wasm.
+  Tag wasmTag;
 
-  // The name of the imported tag for js exceptions. If it is not imported, we
-  // use a default name here (which should differentiate it from any wasm
-  // exceptions).
-  Name jsTag = "__private";
+  // The imported tag for js exceptions.
+  Tag jsTag;
 
   // The ModuleRunner and this ExternalInterface end up needing links both ways,
   // so we cannot init this in the constructor.
@@ -67,15 +65,26 @@ struct LoggingExternalInterface : public ShellExternalInterface {
       }
     }
 
-    for (auto& tag : wasm.tags) {
-      if (tag->module == "fuzzing-support") {
-        if (tag->base == "wasmtag") {
-          wasmTag = tag->name;
-        } else if (tag->base == "jstag") {
-          jsTag = tag->name;
-        }
+    // Set up tags. (Setting these values is useful for debugging - making the
+    // Tag objects valid - and also appears in fuzz-exec logging.)
+    wasmTag.module = "fuzzing-support";
+    wasmTag.base = "wasmtag";
+    wasmTag.name = "imported-wasm-tag";
+    wasmTag.type = Signature(Type::i32, Type::none);
+
+    jsTag.module = "fuzzing-support";
+    jsTag.base = "jstag";
+    jsTag.name = "imported-js-tag";
+    jsTag.type = Signature(Type(HeapType::ext, Nullable), Type::none);
+  }
+
+  Tag* getImportedTag(Tag* tag) override {
+    for (auto* imported : {&wasmTag, &jsTag}) {
+      if (imported->module == tag->module && imported->base == tag->base) {
+        return imported;
       }
     }
+    Fatal() << "missing host tag " << tag->module << '.' << tag->base;
   }
 
   Literal getImportedFunction(Function* import) override {
@@ -122,7 +131,7 @@ struct LoggingExternalInterface : public ShellExternalInterface {
           if (arguments[0].geti32() == 0) {
             throwJSException();
           } else {
-            auto payload = std::make_shared<ExnData>(wasmTag, arguments);
+            auto payload = std::make_shared<ExnData>(&wasmTag, arguments);
             throwException(WasmException{Literal(payload)});
           }
         } else if (import->base == "table-get") {
@@ -213,7 +222,7 @@ struct LoggingExternalInterface : public ShellExternalInterface {
     auto empty = HeapType(Struct{});
     auto inner = Literal(std::make_shared<GCData>(empty, Literals{}), empty);
     Literals arguments = {inner.externalize()};
-    auto payload = std::make_shared<ExnData>(jsTag, arguments);
+    auto payload = std::make_shared<ExnData>(&jsTag, arguments);
     throwException(WasmException{Literal(payload)});
   }
 

src/tools/wasm-ctor-eval.cpp

@@ -323,6 +323,10 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
                    import->type);
   }
 
+  Tag* getImportedTag(Tag* tag) override {
+    WASM_UNREACHABLE("missing imported tag");
+  }
+
   // We assume the table is not modified FIXME
   Literal tableLoad(Name tableName, Address index) override {
     auto* table = wasm->getTableOrNull(tableName);

src/wasm-interpreter.h

@@ -160,6 +160,19 @@ struct FuncData {
   }
 };
 
+// The data of a (ref exn) literal.
+struct ExnData {
+  // The tag of this exn data.
+  // TODO: Add self, like in FuncData, to handle the case of a module that is
+  //       instantiated multiple times.
+  Tag* tag;
+
+  // The payload of this exn data.
+  Literals payload;
+
+  ExnData(Tag* tag, Literals payload) : tag(tag), payload(payload) {}
+};
+
 // Suspend/resume support.
 //
 // As we operate directly on our structured IR, we do not have a program counter
@@ -324,7 +337,7 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
   }
 
   // Same as makeGCData but for ExnData.
-  Literal makeExnData(Name tag, const Literals& payload) {
+  Literal makeExnData(Tag* tag, const Literals& payload) {
     auto allocation = std::make_shared<ExnData>(tag, payload);
 #if __has_feature(leak_sanitizer) || __has_feature(address_sanitizer)
     __lsan_ignore_object(allocation.get());
@@ -1890,9 +1903,18 @@ class ExpressionRunner : public OverriddenVisitor<SubType, Flow> {
   Flow visitTry(Try* curr) { WASM_UNREACHABLE("unimp"); }
   Flow visitTryTable(TryTable* curr) { WASM_UNREACHABLE("unimp"); }
   Flow visitThrow(Throw* curr) {
+    // Single-module implementation. This is used from Precompute, for example.
+    // It is overriden in ModuleRunner to add logic for finding the proper
+    // imported tag (which single-module cases don't care about).
     Literals arguments;
     VISIT_ARGUMENTS(flow, curr->operands, arguments);
-    throwException(WasmException{makeExnData(curr->tag, arguments)});
+    auto* tag = self()->getModule()->getTag(curr->tag);
+    if (tag->imported()) {
+      // The same tag can be imported twice, so by looking at only the current
+      // module we can't tell if two tags are the same or not.
+      return NONCONSTANT_FLOW;
+    }
+    throwException(WasmException{self()->makeExnData(tag, arguments)});
     WASM_UNREACHABLE("throw");
   }
   Flow visitRethrow(Rethrow* curr) { WASM_UNREACHABLE("unimp"); }
@@ -2908,6 +2930,9 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
     virtual void trap(const char* why) = 0;
     virtual void hostLimit(const char* why) = 0;
     virtual void throwException(const WasmException& exn) = 0;
+    // Get the Tag instance for a tag implemented in the host, that is, not
+    // among the linked ModuleRunner instances, but imported from the host.
+    virtual Tag* getImportedTag(Tag* tag) = 0;
 
     // the default impls for load and store switch on the sizes. you can either
     // customize load/store, or the sub-functions which they call
@@ -3194,6 +3219,18 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
     return iter->second;
   }
 
+  Tag* getExportedTag(Name name) {
+    Export* export_ = wasm.getExportOrNull(name);
+    if (!export_ || export_->kind != ExternalKind::Tag) {
+      externalInterface->trap("exported tag not found");
+    }
+    auto* tag = wasm.getTag(*export_->getInternalName());
+    if (tag->imported()) {
+      tag = externalInterface->getImportedTag(tag);
+    }
+    return tag;
+  }
+
   std::string printFunctionStack() {
     std::string ret = "/== (binaryen interpreter stack trace)\n";
     for (int i = int(functionStack.size()) - 1; i >= 0; i--) {
@@ -3445,12 +3482,15 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
   Tag* getCanonicalTag(Name name) {
     auto* inst = self();
     auto* tag = inst->wasm.getTag(name);
-    while (tag->imported()) {
-      inst = inst->linkedInstances.at(tag->module).get();
-      auto* tagExport = inst->wasm.getExport(tag->base);
-      tag = inst->wasm.getTag(*tagExport->getInternalName());
+    if (!tag->imported()) {
+      return tag;
     }
-    return tag;
+    auto iter = inst->linkedInstances.find(tag->module);
+    if (iter == inst->linkedInstances.end()) {
+      return externalInterface->getImportedTag(tag);
+    }
+    inst = iter->second.get();
+    return inst->getExportedTag(tag->base);
   }
 
 public:
@@ -4354,7 +4394,8 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
 
       auto exnData = e.exn.getExnData();
       for (size_t i = 0; i < curr->catchTags.size(); i++) {
-        if (curr->catchTags[i] == exnData->tag) {
+        auto* tag = self()->getCanonicalTag(curr->catchTags[i]);
+        if (tag == exnData->tag) {
           multiValues.push_back(exnData->payload);
           return processCatchBody(curr->catchBodies[i]);
         }
@@ -4377,7 +4418,8 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
       auto exnData = e.exn.getExnData();
       for (size_t i = 0; i < curr->catchTags.size(); i++) {
         auto catchTag = curr->catchTags[i];
-        if (!catchTag.is() || catchTag == exnData->tag) {
+        if (!catchTag.is() ||
+            self()->getCanonicalTag(catchTag) == exnData->tag) {
           Flow ret;
           ret.breakTo = curr->catchDests[i];
           if (catchTag.is()) {
@@ -4395,6 +4437,13 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
       throw;
     }
   }
+  Flow visitThrow(Throw* curr) {
+    Literals arguments;
+    VISIT_ARGUMENTS(flow, curr->operands, arguments);
+    throwException(WasmException{
+      self()->makeExnData(self()->getCanonicalTag(curr->tag), arguments)});
+    WASM_UNREACHABLE("throw");
+  }
   Flow visitRethrow(Rethrow* curr) {
     for (int i = exceptionStack.size() - 1; i >= 0; i--) {
       if (exceptionStack[i].second == curr->target) {
@@ -4463,9 +4512,8 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
       assert(self()->restoredValuesMap.empty());
       // Throw, if we were resumed by resume_throw;
       if (auto* tag = currContinuation->exceptionTag) {
-        // XXX tag->name lacks cross-module support
         throwException(WasmException{
-          self()->makeExnData(tag->name, currContinuation->resumeArguments)});
+          self()->makeExnData(tag, currContinuation->resumeArguments)});
       }
       return currContinuation->resumeArguments;
     }
@@ -4668,9 +4716,8 @@ class ModuleRunnerBase : public ExpressionRunner<SubType> {
         // set), so resuming is done. (And throw, if resume_throw.)
         self()->continuationStore->resuming = false;
         if (auto* tag = currContinuation->exceptionTag) {
-          // XXX tag->name lacks cross-module support
           throwException(WasmException{
-            self()->makeExnData(tag->name, currContinuation->resumeArguments)});
+            self()->makeExnData(tag, currContinuation->resumeArguments)});
         }
       }
     }

src/wasm/wasm-interpreter.cpp

@@ -4,7 +4,7 @@ namespace wasm {
 
 std::ostream& operator<<(std::ostream& o, const WasmException& exn) {
   auto exnData = exn.exn.getExnData();
-  return o << exnData->tag << " " << exnData->payload;
+  return o << exnData->tag->name << " " << exnData->payload;
 }
 
 } // namespace wasm

test/lit/exec/cont_export.wast

@@ -24,7 +24,7 @@
 
   ;; CHECK:      [fuzz-exec] calling call-call-export
   ;; CHECK-NEXT: [LoggingExternalInterface logging 10]
-  ;; CHECK-NEXT: [exception thrown: __private externref]
+  ;; CHECK-NEXT: [exception thrown: imported-js-tag externref]
   (func $call-call-export (export "call-call-export")
     ;; Call suspend as an export. We cannot suspend through JS, so we throw.
     (call $call-export
@@ -35,7 +35,7 @@
 
   ;; CHECK:      [fuzz-exec] calling handled
   ;; CHECK-NEXT: [LoggingExternalInterface logging 10]
-  ;; CHECK-NEXT: [exception thrown: __private externref]
+  ;; CHECK-NEXT: [exception thrown: imported-js-tag externref]
   (func $handled (export "handled")
     ;; As above, but inside a continuation, so it would be handled - if we could
     ;; suspend though JS. But we can't, so we throw.

test/lit/exec/cont_export_throw.wast

@@ -22,7 +22,7 @@
  )
 
  ;; CHECK:      [fuzz-exec] calling handled
- ;; CHECK-NEXT: [exception thrown: __private externref]
+ ;; CHECK-NEXT: [exception thrown: imported-js-tag externref]
  (func $handled (export "handled")
   (drop
    (block $block (result (ref $cont))

test/lit/exec/tag-cross-module.wast

@@ -0,0 +1,26 @@
+;; RUN: wasm-opt %s -all --fuzz-exec-before --fuzz-exec-second=%s.second -q -o /dev/null 2>&1 | filecheck %s
+
+;; Define a tag in this module, and another tag in the secondary module, with
+;; the same name but different (incompatible) contents. The second module will
+;; call our export, and when we throw our tag, it should not catch it.
+
+(module
+ (tag $tag (param structref))
+
+ (export "primary-tag" (tag $tag))
+
+ (func $func (export "func") (result i32)
+  (throw $tag
+   (ref.null struct)
+  )
+ )
+)
+
+;; CHECK: [fuzz-exec] calling func
+;; CHECK-NEXT: [exception thrown: tag nullref]
+;; CHECK-NEXT: [fuzz-exec] calling func2-internal
+;; CHECK-NEXT: [exception thrown: tag nullref]
+;; CHECK-NEXT: [fuzz-exec] calling func2-imported
+;; CHECK-NEXT: func2-imported => null
+
+