Always do a full roundtrip in run-roundtrip.py

Even when the result is to be printed rather than compared byte for byte with the first version its still good to process the resulting wat output file so that we know we can parse what we generate. Case in point, this changed caused me to fix two latent bugs: 1. We were not correctly parsing events with inline import/export. 2. We were output element segment names even when bulk memory was not enabled (See #1651) The fix for (2) is a little more involved that we might like since for the first time the wat writer needs to know what features are enabled. Fixes: #1651
WebAssembly · Feb 25, 2023 · 67e40ee · 67e40ee
1 parent adc9ecc
commit 67e40ee
Show file tree

Hide file tree

Showing 8 changed files with 62 additions and 48 deletions.
diff --git a/include/wabt/wat-writer.h b/include/wabt/wat-writer.h
@@ -18,13 +18,17 @@
 #define WABT_WAT_WRITER_H_
 
 #include "wabt/common.h"
+#include "wabt/feature.h"
 
 namespace wabt {
 
 struct Module;
 class Stream;
 
 struct WriteWatOptions {
+  WriteWatOptions() = default;
+  WriteWatOptions(const Features& features) : features(features) {}
+  Features features;
   bool fold_exprs = false;  // Write folded expressions.
   bool inline_export = false;
   bool inline_import = false;

diff --git a/src/tools/wasm2wat.cc b/src/tools/wasm2wat.cc
@@ -38,8 +38,10 @@ static int s_verbose;
 static std::string s_infile;
 static std::string s_outfile;
 static Features s_features;
-static WriteWatOptions s_write_wat_options;
-static bool s_generate_names = false;
+static bool s_generate_names;
+static bool s_fold_exprs;
+static bool s_inline_import;
+static bool s_inline_export;
 static bool s_read_debug_names = true;
 static bool s_fail_on_custom_section_error = true;
 static std::unique_ptr<FileStream> s_log_stream;
@@ -72,12 +74,12 @@ static void ParseOptions(int argc, char** argv) {
         ConvertBackslashToSlash(&s_outfile);
       });
   parser.AddOption('f', "fold-exprs", "Write folded expressions where possible",
-                   []() { s_write_wat_options.fold_exprs = true; });
+                   []() { s_fold_exprs = true; });
   s_features.AddOptions(&parser);
   parser.AddOption("inline-exports", "Write all exports inline",
-                   []() { s_write_wat_options.inline_export = true; });
+                   []() { s_inline_export = true; });
   parser.AddOption("inline-imports", "Write all imports inline",
-                   []() { s_write_wat_options.inline_import = true; });
+                   []() { s_inline_import = true; });
   parser.AddOption("no-debug-names", "Ignore debug names in the binary file",
                    []() { s_read_debug_names = false; });
   parser.AddOption("ignore-custom-section-errors",
@@ -132,9 +134,13 @@ int ProgramMain(int argc, char** argv) {
       }
 
       if (Succeeded(result)) {
+        WriteWatOptions wat_options(s_features);
+        wat_options.fold_exprs = s_fold_exprs;
+        wat_options.inline_import = s_inline_import;
+        wat_options.inline_export = s_inline_export;
         FileStream stream(!s_outfile.empty() ? FileStream(s_outfile)
                                              : FileStream(stdout));
-        result = WriteWat(&stream, &module, s_write_wat_options);
+        result = WriteWat(&stream, &module, wat_options);
       }
     }
     FormatErrorsToFile(errors, Location::Type::Binary);

diff --git a/src/tools/wat-desugar.cc b/src/tools/wat-desugar.cc
@@ -37,9 +37,11 @@ using namespace wabt;
 
 static const char* s_infile;
 static const char* s_outfile;
-static WriteWatOptions s_write_wat_options;
-static bool s_generate_names;
 static bool s_debug_parsing;
+static bool s_fold_exprs;
+static bool s_generate_names;
+static bool s_inline_import;
+static bool s_inline_export;
 static Features s_features;
 
 static const char s_description[] =
@@ -64,11 +66,11 @@ static void ParseOptions(int argc, char** argv) {
   parser.AddOption("debug-parser", "Turn on debugging the parser of wat files",
                    []() { s_debug_parsing = true; });
   parser.AddOption('f', "fold-exprs", "Write folded expressions where possible",
-                   []() { s_write_wat_options.fold_exprs = true; });
+                   []() { s_fold_exprs = true; });
   parser.AddOption("inline-exports", "Write all exports inline",
-                   []() { s_write_wat_options.inline_export = true; });
+                   []() { s_inline_export = true; });
   parser.AddOption("inline-imports", "Write all imports inline",
-                   []() { s_write_wat_options.inline_import = true; });
+                   []() { s_inline_import = true; });
   s_features.AddOptions(&parser);
   parser.AddOption(
       "generate-names",
@@ -115,8 +117,12 @@ int ProgramMain(int argc, char** argv) {
     }
 
     if (Succeeded(result)) {
+      WriteWatOptions wat_options(s_features);
+      wat_options.fold_exprs = s_fold_exprs;
+      wat_options.inline_import = s_inline_import;
+      wat_options.inline_export = s_inline_export;
       FileStream stream(s_outfile ? FileStream(s_outfile) : FileStream(stdout));
-      result = WriteWat(&stream, module, s_write_wat_options);
+      result = WriteWat(&stream, module, wat_options);
     }
   }
 

diff --git a/src/wast-parser.cc b/src/wast-parser.cc
@@ -1352,6 +1352,8 @@ Result WastParser::ParseTagModuleField(Module* module) {
   }
   EXPECT(Lpar);
   EXPECT(Tag);
+  Location loc = GetLocation();
+
   std::string name;
   ParseBindVarOpt(&name);
 
@@ -1361,20 +1363,23 @@ Result WastParser::ParseTagModuleField(Module* module) {
   if (PeekMatchLpar(TokenType::Import)) {
     CheckImportOrdering(module);
     auto import = MakeUnique<TagImport>(name);
+    Tag& tag = import->tag;
     CHECK_RESULT(ParseInlineImport(import.get()));
-    CHECK_RESULT(ParseTypeUseOpt(&import->tag.decl));
-    CHECK_RESULT(ParseUnboundFuncSignature(&import->tag.decl.sig));
+    CHECK_RESULT(ParseTypeUseOpt(&tag.decl));
+    CHECK_RESULT(ParseUnboundFuncSignature(&tag.decl.sig));
+    CHECK_RESULT(ErrorIfLpar({"type", "param", "result"}));
     auto field =
         MakeUnique<ImportModuleField>(std::move(import), GetLocation());
     module->AppendField(std::move(field));
   } else {
-    auto field = MakeUnique<TagModuleField>(GetLocation(), name);
+    auto field = MakeUnique<TagModuleField>(loc, name);
     CHECK_RESULT(ParseTypeUseOpt(&field->tag.decl));
     CHECK_RESULT(ParseUnboundFuncSignature(&field->tag.decl.sig));
     module->AppendField(std::move(field));
   }
 
   AppendInlineExportFields(module, &export_fields, module->tags.size() - 1);
+
   EXPECT(Rpar);
   return Result::Ok;
 }

diff --git a/src/wat-writer.cc b/src/wat-writer.cc
@@ -1435,7 +1435,15 @@ void WatWriter::WriteTable(const Table& table) {
 
 void WatWriter::WriteElemSegment(const ElemSegment& segment) {
   WriteOpenSpace("elem");
-  WriteNameOrIndex(segment.name, elem_segment_index_, NextChar::Space);
+  // The first name we encounter here, pre-bulk-memory, was intended to refer to
+  // the table while segment names were not supported at all.  For this reason
+  // we cannot emit a segment name here without bulk-memory enabled, otherwise
+  // the name will be assumed to be the name of a table and parsing will fail.
+  if (options_.features.bulk_memory_enabled()) {
+    WriteNameOrIndex(segment.name, elem_segment_index_, NextChar::Space);
+  } else {
+    Writef("(;%u;)", elem_segment_index_);
+  }
 
   uint8_t flags = segment.GetFlags(&module);
 

diff --git a/test/roundtrip/fold-function-references.txt b/test/roundtrip/fold-function-references.txt
@@ -1,4 +1,5 @@
 ;;; TOOL: run-roundtrip
+;;; SKIP: requires fix to parser for typed function refs (#1889)
 ;;; ARGS: --stdout --fold-exprs --enable-function-references
 (module
   (type $f32-f32 (func (param f32) (result f32)))

diff --git a/test/roundtrip/inline-export-func.txt b/test/roundtrip/inline-export-func.txt
@@ -1,12 +1,12 @@
 ;;; TOOL: run-roundtrip
 ;;; ARGS: --stdout --inline-export
 (module
-  (func $foo
+  (func $foo (param i32)
     nop)
   (export "foo" (func $foo)))
 (;; STDOUT ;;;
 (module
-  (type (;0;) (func))
-  (func (;0;) (export "foo") (type 0)
+  (type (;0;) (func (param i32)))
+  (func (;0;) (export "foo") (type 0) (param i32)
     nop))
 ;;; STDOUT ;;)
diff --git a/test/run-roundtrip.py b/test/run-roundtrip.py
@@ -55,11 +55,11 @@ def FilesAreEqual(filename1, filename2, verbose=False):
     return (OK, '')
 
 
-def TwoRoundtrips(wat2wasm, wasm2wat, out_dir, filename, verbose):
+def DoRoundtrip(wat2wasm, wasm2wat, out_dir, filename, verbose, stdout):
     basename = os.path.basename(filename)
     basename_noext = os.path.splitext(basename)[0]
     wasm1_file = os.path.join(out_dir, basename_noext + '-1.wasm')
-    wast2_file = os.path.join(out_dir, basename_noext + '-2.wast')
+    wat2_file = os.path.join(out_dir, basename_noext + '-2.wat')
     wasm3_file = os.path.join(out_dir, basename_noext + '-3.wasm')
     try:
         wat2wasm.RunWithArgs('-o', wasm1_file, filename)
@@ -68,28 +68,16 @@ def TwoRoundtrips(wat2wasm, wasm2wat, out_dir, filename, verbose):
         # test)
         return (SKIPPED, None)
     try:
-        wasm2wat.RunWithArgs('-o', wast2_file, wasm1_file)
-        wat2wasm.RunWithArgs('-o', wasm3_file, wast2_file)
+        wasm2wat.RunWithArgs('-o', wat2_file, wasm1_file)
+        wat2wasm.RunWithArgs('-o', wasm3_file, wat2_file)
     except Error as e:
         return (ERROR, str(e))
-    return FilesAreEqual(wasm1_file, wasm3_file, verbose)
-
-
-def OneRoundtripToStdout(wat2wasm, wasm2wat, out_dir, filename, verbose):
-    basename = os.path.basename(filename)
-    basename_noext = os.path.splitext(basename)[0]
-    wasm_file = os.path.join(out_dir, basename_noext + '.wasm')
-    try:
-        wat2wasm.RunWithArgs('-o', wasm_file, filename)
-    except Error:
-        # if the file doesn't parse properly, just skip it (it may be a "bad-*"
-        # test)
-        return (SKIPPED, None)
-    try:
-        wasm2wat.RunWithArgs(wasm_file)
-    except Error as e:
-        return (ERROR, str(e))
-    return (OK, '')
+    if stdout:
+        with open(wat2_file) as f:
+            sys.stdout.write(f.read())
+        return (OK, '')
+    else:
+        return FilesAreEqual(wasm1_file, wasm3_file, verbose)
 
 
 def main(args):
@@ -102,7 +90,7 @@ def main(args):
                         default=find_exe.GetDefaultPath(),
                         help='directory to search for all executables.')
     parser.add_argument('--stdout', action='store_true',
-                        help='do one roundtrip and write wast output to stdout')
+                        help='do one roundtrip and write wat output to stdout')
     parser.add_argument('--no-error-cmdline',
                         help='don\'t display the subprocess\'s commandline when '
                         'an error occurs', dest='error_cmdline',
@@ -188,12 +176,8 @@ def main(args):
         return ERROR
 
     with utils.TempDirectory(options.out_dir, 'roundtrip-') as out_dir:
-        if options.stdout:
-            result, msg = OneRoundtripToStdout(wat2wasm, wasm2wat, out_dir,
-                                               filename, options.verbose)
-        else:
-            result, msg = TwoRoundtrips(wat2wasm, wasm2wat, out_dir, filename,
-                                        options.verbose)
+        result, msg = DoRoundtrip(wat2wasm, wasm2wat, out_dir, filename,
+                                  options.verbose, options.stdout)
         if result == ERROR:
             sys.stderr.write(msg)
         return result