Support (module definition ...) and (module instance ...) in WAST spec tests (#8058)

* Add support for `(module definition ...)` and `(module instance ...)`
in WAST spec tests
* [Syntax
reference](https://github.com/WebAssembly/spec/blob/main/interpreter/README.md#scripts)
* These statements are skipped by the WAST splitter, since wasm-opt
can't understand them. In the future, we need some extra work to handle
explicit module instantiations correctly here.
* This fixes the memory.wast spec test. The remaining 3 that currently
depend on this syntax still break for other reasons (mentioned in
shared.py)
 * Remove extra unneeded call to run_spec_test

Author: stevenfontanella

check.py

@@ -221,8 +221,6 @@ def check_expected(actual, expected):
 
         check_expected(actual, expected)
 
-        run_spec_test(wast)
-
         # check binary format. here we can verify execution of the final
         # result, no need for an output verification
         actual = ''

scripts/test/shared.py

@@ -443,13 +443,14 @@ def get_tests(test_dir, extensions=[], recursive=False):
     'imports.wast',  # Missing validation of missing function on instantiation
     'proposals/threads/imports.wast',  # Missing memory type validation on instantiation
     'linking.wast',  # Missing function type validation on instantiation
-    'memory.wast',   # Requires wast `module definition` support
     'proposals/threads/memory.wast',  # Missing memory type validation on instantiation
     'memory64-imports.wast',  # Missing validation on instantiation
     'annotations.wast',  # String annotations IDs should be allowed
     'id.wast',       # Empty IDs should be disallowed
-    'instance.wast',  # Requires wast `module definition` support
-    'table64.wast',   # Requires wast `module definition` support
+    # Requires correct handling of tag imports from different instances of the same module,
+    # ref.null wast constants, and splitting for module instances
+    'instance.wast',
+    'table64.wast',   # Requires validations for table size
     'table_grow.wast',  # Incorrect table linking semantics in interpreter
     'tag.wast',      # Non-empty tag results allowed by stack switching
     'try_table.wast',  # Requires try_table interpretation
@@ -472,7 +473,7 @@ def get_tests(test_dir, extensions=[], recursive=False):
     'type-rec.wast',  # Missing function type validation on instantiation
     'type-subtyping.wast',  # ShellExternalInterface::callTable does not handle subtyping
     'call_indirect.wast',   # Bug with 64-bit inline element segment parsing
-    'memory64.wast',        # Requires wast `module definition` support
+    'memory64.wast',        # Requires validations for memory size
     'imports0.wast',        # Missing memory type validation on instantiation
     'imports2.wast',        # Missing memory type validation on instantiation
     'imports3.wast',        # Missing memory type validation on instantiation

scripts/test/support.py

@@ -90,8 +90,14 @@ def untar(tarfile, outdir):
 
 QUOTED = re.compile(r'\(module\s*(\$\S*)?\s+(quote|binary)')
 
+MODULE_DEFINITION_OR_INSTANCE = re.compile(r'(?m)\(module\s+(instance|definition)')
+
 
 def split_wast(wastFile):
+    '''
+    Returns a list of pairs of module definitions and assertions.
+    Module invalidity tests, as well as (module definition ...) and (module instance ...) are skipped.
+    '''
     # if it's a binary, leave it as is, we can't split it
     wast = None
     if not wastFile.endswith('.wasm'):
@@ -128,7 +134,7 @@ def to_end(j):
         return j
 
     i = 0
-    ignoring_quoted = False
+    ignoring_assertions = False
     while i >= 0:
         start = wast.find('(', i)
         if start >= 0 and wast[start + 1] == ';':
@@ -146,17 +152,17 @@ def to_end(j):
             break
         i = to_end(start + 1)
         chunk = wast[start:i]
-        if QUOTED.match(chunk):
+        if QUOTED.match(chunk) or MODULE_DEFINITION_OR_INSTANCE.match(chunk):
             # There may be assertions after this quoted module, but we aren't
             # returning the module, so we need to skip the assertions as well.
-            ignoring_quoted = True
+            ignoring_assertions = True
             continue
         if chunk.startswith('(module'):
-            ignoring_quoted = False
+            ignoring_assertions = False
             ret += [(chunk, [])]
         elif chunk.startswith('(assert_invalid'):
             continue
-        elif chunk.startswith(('(assert', '(invoke', '(register')) and not ignoring_quoted:
+        elif chunk.startswith(('(assert', '(invoke', '(register')) and not ignoring_assertions:
             # ret may be empty if there are some asserts before the first
             # module. in that case these are asserts *without* a module, which
             # are valid (they may check something that doesn't refer to a module
@@ -190,14 +196,13 @@ def run_command(cmd, expected_status=0, stderr=None,
     out, err = proc.communicate()
     code = proc.returncode
     if expected_status is not None and code != expected_status:
-        raise Exception(('run_command failed (%s)' % code, out + str(err or '')))
+        raise Exception(f"run_command `{' '.join(cmd)}` failed ({code}) {err or ''}")
     if expected_err is not None:
         if err_ignore is not None:
             err = "\n".join([line for line in err.split('\n') if err_ignore not in line])
         err_correct = expected_err in err if err_contains else expected_err == err
         if not err_correct:
-            raise Exception(('run_command unexpected stderr',
-                             "expected '%s', actual '%s'" % (expected_err, err)))
+            raise Exception(f"run_command unexpected stderr. Expected '{expected_err}', actual '{err}'")
     return out
 
 

src/parser/wast-parser.cpp

@@ -88,16 +88,21 @@ Result<Action> action(Lexer& in) {
   return in.err("expected action");
 }
 
-// (module id? binary string*)
-// (module id? quote string*)
-// (module ...)
+// (module id binary string*)
+// (module id quote string*)
+// (module definition id? ...)
+// (module definition id? binary ...)
 Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
   Lexer reset = in;
   if (!in.takeSExprStart("module"sv)) {
     return in.err("expected module");
   }
-  // TODO: use ID?
-  [[maybe_unused]] auto id = in.takeID();
+
+  bool isDefinition = in.takeKeyword("definition"sv);
+  std::optional<Name> id = in.takeID();
+
+  Lexer moduleBody = in;
+
   QuotedModuleType type;
   if (in.takeKeyword("quote"sv)) {
     type = QuotedModuleType::Text;
@@ -118,15 +123,24 @@ Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
       }
     }
     std::string mod(reset.next().substr(0, in.getPos() - reset.getPos()));
-    return QuotedModule{QuotedModuleType::Text, mod};
+    return WASTModule{isDefinition, QuotedModule{QuotedModuleType::Text, mod}};
   } else {
-    // This is a normal inline module that should be parseable. Reset to the
-    // start and parse it normally.
-    in = std::move(reset);
+    // In this case the module is mostly valid WAT, unless it is a module
+    // definition in which case it will begin with (module definition ...)
+    in = std::move(moduleBody);
+
     auto wasm = std::make_shared<Module>();
+    if (id) {
+      wasm->name = *id;
+    }
+
     wasm->features = FeatureSet::All;
-    CHECK_ERR(parseModule(*wasm, in));
-    return wasm;
+    CHECK_ERR(parseModuleBody(*wasm, in));
+    if (!in.takeRParen()) {
+      return in.err("expected end of module");
+    }
+
+    return WASTModule{isDefinition, wasm};
   }
 
   // We have a quote or binary module. Collect its contents.
@@ -139,7 +153,7 @@ Result<WASTModule> wastModule(Lexer& in, bool maybeInvalid = false) {
     return in.err("expected end of module");
   }
 
-  return QuotedModule{type, ss.str()};
+  return WASTModule{isDefinition, QuotedModule{type, ss.str()}};
 }
 
 Result<NaNKind> nan(Lexer& in) {
@@ -447,17 +461,42 @@ MaybeResult<Register> register_(Lexer& in) {
     return in.err("expected name");
   }
 
-  // TODO: Do we need to use this optional id?
-  in.takeID();
+  auto instanceName = in.takeID();
 
   if (!in.takeRParen()) {
-    // TODO: handle optional module id.
     return in.err("expected end of register command");
   }
-  return Register{*name};
+
+  return Register{*name, instanceName};
 }
 
-// module | register | action | assertion
+// (module instance instance_name? module_name?)
+MaybeResult<ModuleInstantiation> instantiation(Lexer& in) {
+  Lexer reset = in;
+  if (!in.takeSExprStart("module"sv)) {
+    std::optional<std::string_view> actual = in.peekKeyword();
+    return in.err((std::stringstream() << "expected `module` keyword but got "
+                                       << actual.value_or("<not a keyword>"))
+                    .str());
+  }
+
+  if (!in.takeKeyword("instance"sv)) {
+    // This is not a module instance and probably a module instead.
+    in = reset;
+    return {};
+  }
+
+  auto instanceId = in.takeID();
+  auto moduleId = in.takeID();
+
+  if (!in.takeRParen()) {
+    return in.err("expected end of module instantiation");
+  }
+
+  return ModuleInstantiation{moduleId, instanceId};
+}
+
+// instantiate | module | register | action | assertion
 Result<WASTCommand> command(Lexer& in) {
   if (auto cmd = register_(in)) {
     CHECK_ERR(cmd);
@@ -471,9 +510,14 @@ Result<WASTCommand> command(Lexer& in) {
     CHECK_ERR(cmd);
     return *cmd;
   }
-  auto mod = wastModule(in);
-  CHECK_ERR(mod);
-  return *mod;
+  if (auto cmd = instantiation(in)) {
+    CHECK_ERR(cmd);
+    return *cmd;
+  }
+
+  auto module = wastModule(in);
+  CHECK_ERR(module);
+  return *module;
 }
 
 #pragma GCC diagnostic push
@@ -494,7 +538,8 @@ Result<WASTScript> wast(Lexer& in) {
         // No, that wasn't the problem. Return the original error.
         return Err{err->msg};
       }
-      cmds.push_back({WASTModule{std::move(wasm)}, line});
+      cmds.push_back(
+        {WASTModule{/*isDefinition=*/false, std::move(wasm)}, line});
       return cmds;
     }
     CHECK_ERR(cmd);

src/parser/wat-parser.h

@@ -100,7 +100,10 @@ struct QuotedModule {
   std::string module;
 };
 
-using WASTModule = std::variant<QuotedModule, std::shared_ptr<Module>>;
+struct WASTModule {
+  bool isDefinition = false;
+  std::variant<QuotedModule, std::shared_ptr<Module>> module;
+};
 
 enum class ModuleAssertionType { Trap, Malformed, Invalid, Unlinkable };
 
@@ -112,10 +115,21 @@ struct AssertModule {
 using Assertion = std::variant<AssertReturn, AssertAction, AssertModule>;
 
 struct Register {
+  // TODO: Rename this to distinguish it from instanceName.
   Name name;
+  std::optional<Name> instanceName = std::nullopt;
+};
+
+struct ModuleInstantiation {
+  // If not specified, instantiate the most recent module definition.
+  std::optional<Name> moduleName;
+
+  // If not specified, use the moduleName
+  std::optional<Name> instanceName;
 };
 
-using WASTCommand = std::variant<WASTModule, Register, Action, Assertion>;
+using WASTCommand =
+  std::variant<WASTModule, Register, Action, Assertion, ModuleInstantiation>;
 
 struct ScriptEntry {
   WASTCommand cmd;