[wasm-split] Make option validation declarative (#3916)

In anticipation of adding a third wasm-split mode, merge-profiles, in addition
to the existing split and instrument modes, refactor wasm-split's option
validation to let the valid modes be declared for each option. This approach is
more scalable and robust than the ad-hoc validation we had previously.

4 files changed, 248 insertions, 102 deletions

diff --git a/src/tools/wasm-split.cpp b/src/tools/wasm-split.cpp
index 7f3fe65c4..f12a6dee6 100644
--- a/src/tools/wasm-split.cpp
+++ b/src/tools/wasm-split.cpp
@@ -48,12 +48,18 @@ std::set<Name> parseNameList(const std::string& list) {
 }
 
 struct WasmSplitOptions : ToolOptions {
+  enum class Mode : unsigned {
+    Split,
+    Instrument,
+  };
+  Mode mode = Mode::Split;
+  constexpr static size_t NumModes =
+    static_cast<unsigned>(Mode::Instrument) + 1;
+
   bool verbose = false;
   bool emitBinary = true;
   bool symbolMap = false;
 
-  bool instrument = false;
-
   // TODO: Remove this. See the comment in wasm-binary.h.
   bool emitModuleNames = false;
 
@@ -77,7 +83,22 @@ struct WasmSplitOptions : ToolOptions {
   // Figure out a more elegant solution for that use case and remove this.
   int initialTableSize = -1;
 
+  // The options that are valid for each mode.
+  std::array<std::unordered_set<std::string>, NumModes> validOptions;
+  std::vector<std::string> usedOptions;
+
   WasmSplitOptions();
+  WasmSplitOptions& add(const std::string& longName,
+                        const std::string& shortName,
+                        const std::string& description,
+                        std::vector<Mode>&& modes,
+                        Arguments arguments,
+                        const Action& action);
+  WasmSplitOptions& add(const std::string& longName,
+                        const std::string& shortName,
+                        const std::string& description,
+                        Arguments arguments,
+                        const Action& action);
   bool validate();
   void parse(int argc, const char* argv[]);
 };
@@ -85,35 +106,35 @@ struct WasmSplitOptions : ToolOptions {
 WasmSplitOptions::WasmSplitOptions()
   : ToolOptions("wasm-split",
                 "Split a module into a primary module and a secondary "
-                "module or instrument a module to gather a profile that "
-                "can inform future splitting.") {
+                "module, or instrument a module to gather a profile that "
+                "can inform future splitting, or manage such profiles. Options "
+                "that are only accepted in particular modes are marked with "
+                "the accepted \"[<modes>]\" in their descriptions.") {
   (*this)
-    .add("--instrument",
+    .add("--split",
          "",
-         "Instrument the module to generate a profile that can be used to "
-         "guide splitting",
+         "Split an input module into two output modules. The default mode.",
          Options::Arguments::Zero,
-         [&](Options* o, const std::string& argument) { instrument = true; })
+         [&](Options* o, const std::string& arugment) { mode = Mode::Split; })
+    .add(
+      "--instrument",
+      "",
+      "Instrument an input module to allow it to generate a profile that can"
+      " be used to guide splitting.",
+      Options::Arguments::Zero,
+      [&](Options* o, const std::string& argument) { mode = Mode::Instrument; })
     .add(
       "--profile",
       "",
-      "The profile to use to guide splitting. May not be used with "
-      "--instrument.",
+      "The profile to use to guide splitting.",
+      {Mode::Split},
       Options::Arguments::One,
       [&](Options* o, const std::string& argument) { profileFile = argument; })
-    .add("--profile-export",
-         "",
-         "The export name of the function the embedder calls to write the "
-         "profile into memory. Defaults to `__write_profile`. Must be used "
-         "with --instrument.",
-         Options::Arguments::One,
-         [&](Options* o, const std::string& argument) {
-           profileExport = argument;
-         })
     .add("--keep-funcs",
          "",
          "Comma-separated list of functions to keep in the primary module, "
          "regardless of any profile.",
+         {Mode::Split},
          Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
            keepFuncs = parseNameList(argument);
@@ -123,39 +144,38 @@ WasmSplitOptions::WasmSplitOptions()
          "Comma-separated list of functions to split into the secondary "
          "module, regardless of any profile. If there is no profile, then "
          "this defaults to all functions defined in the module.",
+         {Mode::Split},
          Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
            splitFuncs = parseNameList(argument);
          })
-    .add("--output",
-         "-o",
-         "Output file. Only usable with --instrument.",
-         Options::Arguments::One,
-         [&](Options* o, const std::string& argument) { output = argument; })
     .add("--primary-output",
          "-o1",
-         "Output file for the primary module. Not usable with --instrument.",
+         "Output file for the primary module.",
+         {Mode::Split},
          Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
            primaryOutput = argument;
          })
     .add("--secondary-output",
          "-o2",
-         "Output file for the secondary module. Not usable with --instrument.",
+         "Output file for the secondary module.",
+         {Mode::Split},
          Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
            secondaryOutput = argument;
          })
     .add("--symbolmap",
          "",
-         "Write a symbol map file for each of the output modules. Not usable "
-         "with --instrument.",
+         "Write a symbol map file for each of the output modules.",
+         {Mode::Split},
          Options::Arguments::Zero,
          [&](Options* o, const std::string& argument) { symbolMap = true; })
     .add("--import-namespace",
          "",
          "The namespace from which to import objects from the primary "
          "module into the secondary module.",
+         {Mode::Split},
          Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
            importNamespace = argument;
@@ -164,6 +184,7 @@ WasmSplitOptions::WasmSplitOptions()
          "",
          "The namespace from which to import placeholder functions into "
          "the primary module.",
+         {Mode::Split},
          Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
            placeholderNamespace = argument;
@@ -173,28 +194,23 @@ WasmSplitOptions::WasmSplitOptions()
       "",
       "An identifying prefix to prepend to new export names created "
       "by module splitting.",
+      {Mode::Split},
       Options::Arguments::One,
       [&](Options* o, const std::string& argument) { exportPrefix = argument; })
-    .add("--verbose",
-         "-v",
-         "Verbose output mode. Prints the functions that will be kept "
-         "and split out when splitting a module.",
-         Options::Arguments::Zero,
+    .add("--output",
+         "-o",
+         "Output file.",
+         {Mode::Instrument},
+         Options::Arguments::One,
+         [&](Options* o, const std::string& argument) { output = argument; })
+    .add("--profile-export",
+         "",
+         "The export name of the function the embedder calls to write the "
+         "profile into memory. Defaults to `__write_profile`.",
+         {Mode::Instrument},
+         Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
-           verbose = true;
-           quiet = false;
-         })
-    .add("--emit-text",
-         "-S",
-         "Emit text instead of binary for the output file or files.",
-         Options::Arguments::Zero,
-         [&](Options* o, const std::string& argument) { emitBinary = false; })
-    .add("--debuginfo",
-         "-g",
-         "Emit names section in wasm binary (or full debuginfo in wast)",
-         Options::Arguments::Zero,
-         [&](Options* o, const std::string& arguments) {
-           passOptions.debugInfo = true;
+           profileExport = argument;
          })
     .add(
       "--emit-module-names",
@@ -203,6 +219,7 @@ WasmSplitOptions::WasmSplitOptions()
       "Can help differentiate the modules in stack traces. This option will be "
       "removed once simpler ways of naming modules are widely available. See "
       "https://bugs.chromium.org/p/v8/issues/detail?id=11808.",
+      {Mode::Split, Mode::Instrument},
       Options::Arguments::Zero,
       [&](Options* o, const std::string& arguments) { emitModuleNames = true; })
     .add("--initial-table",
@@ -211,16 +228,93 @@ WasmSplitOptions::WasmSplitOptions()
          "table size when using Emscripten's SPLIT_MODULE mode with dynamic "
          "linking. TODO: Figure out a more elegant solution for that use "
          "case and remove this.",
+         {Mode::Split, Mode::Instrument},
          Options::Arguments::One,
          [&](Options* o, const std::string& argument) {
            initialTableSize = std::stoi(argument);
          })
+    .add("--verbose",
+         "-v",
+         "Verbose output mode. Prints the functions that will be kept "
+         "and split out when splitting a module.",
+         Options::Arguments::Zero,
+         [&](Options* o, const std::string& argument) {
+           verbose = true;
+           quiet = false;
+         })
+    .add("--emit-text",
+         "-S",
+         "Emit text instead of binary for the output file or files.",
+         Options::Arguments::Zero,
+         [&](Options* o, const std::string& argument) { emitBinary = false; })
+    .add("--debuginfo",
+         "-g",
+         "Emit names section in wasm binary (or full debuginfo in wast)",
+         Options::Arguments::Zero,
+         [&](Options* o, const std::string& arguments) {
+           passOptions.debugInfo = true;
+         })
     .add_positional(
       "INFILE",
       Options::Arguments::One,
       [&](Options* o, const std::string& argument) { input = argument; });
 }
 
+std::ostream& operator<<(std::ostream& o, WasmSplitOptions::Mode& mode) {
+  switch (mode) {
+    case WasmSplitOptions::Mode::Split:
+      o << "split";
+      break;
+    case WasmSplitOptions::Mode::Instrument:
+      o << "instrument";
+      break;
+  }
+  return o;
+}
+
+WasmSplitOptions& WasmSplitOptions::add(const std::string& longName,
+                                        const std::string& shortName,
+                                        const std::string& description,
+                                        std::vector<Mode>&& modes,
+                                        Arguments arguments,
+                                        const Action& action) {
+  // Insert the valid modes at the beginning of the description.
+  std::stringstream desc;
+  if (modes.size()) {
+    desc << '[';
+    std::string sep = "";
+    for (Mode m : modes) {
+      validOptions[static_cast<unsigned>(m)].insert(longName);
+      desc << sep << m;
+      sep = ", ";
+    }
+    desc << "] ";
+  }
+  desc << description;
+  ToolOptions::add(
+    longName,
+    shortName,
+    desc.str(),
+    arguments,
+    [&, action, longName](Options* o, const std::string& argument) {
+      usedOptions.push_back(longName);
+      action(o, argument);
+    });
+  return *this;
+}
+
+WasmSplitOptions& WasmSplitOptions::add(const std::string& longName,
+                                        const std::string& shortName,
+                                        const std::string& description,
+                                        Arguments arguments,
+                                        const Action& action) {
+  // Add an option valid in all modes.
+  for (unsigned i = 0; i < NumModes; ++i) {
+    validOptions[i].insert(longName);
+  }
+  return add(longName, shortName, description, {}, arguments, action);
+}
+
 bool WasmSplitOptions::validate() {
   bool valid = true;
   auto fail = [&](auto msg) {
@@ -231,46 +325,27 @@ bool WasmSplitOptions::validate() {
   if (!input.size()) {
     fail("no input file");
   }
-  if (instrument) {
-    using Opt = std::pair<const std::string&, const std::string>;
-    for (auto& opt : {Opt{profileFile, "--profile"},
-                      Opt{primaryOutput, "primary output"},
-                      Opt{secondaryOutput, "secondary output"},
-                      Opt{importNamespace, "--import-namespace"},
-                      Opt{placeholderNamespace, "--placeholder-namespace"},
-                      Opt{exportPrefix, "--export-prefix"}}) {
-      if (opt.first.size()) {
-        fail(opt.second + " cannot be used with --instrument");
-      }
-    }
-    if (keepFuncs.size()) {
-      fail("--keep-funcs cannot be used with --instrument");
-    }
-    if (splitFuncs.size()) {
-      fail("--split-funcs cannot be used with --instrument");
-    }
-    if (symbolMap) {
-      fail("--symbolmap cannot be used with --instrument");
-    }
-  } else {
-    if (output.size()) {
-      fail(
-        "must provide separate primary and secondary output with -o1 and -o2");
-    }
-    if (profileExport != DEFAULT_PROFILE_EXPORT) {
-      fail("--profile-export must be used with --instrument");
+
+  // Validate that all used options are allowed in the current mode.
+  for (std::string& opt : usedOptions) {
+    if (!validOptions[static_cast<unsigned>(mode)].count(opt)) {
+      std::stringstream msg;
+      msg << "Option " << opt << " cannot be used in " << mode << " mode.";
+      fail(msg.str());
     }
   }
 
-  std::vector<Name> impossible;
-  std::set_intersection(keepFuncs.begin(),
-                        keepFuncs.end(),
-                        splitFuncs.begin(),
-                        splitFuncs.end(),
-                        std::inserter(impossible, impossible.end()));
-  for (auto& func : impossible) {
-    fail(std::string("Cannot both keep and split out function ") +
-         func.c_str());
+  if (mode == Mode::Split) {
+    std::vector<Name> impossible;
+    std::set_intersection(keepFuncs.begin(),
+                          keepFuncs.end(),
+                          splitFuncs.begin(),
+                          splitFuncs.end(),
+                          std::inserter(impossible, impossible.end()));
+    for (auto& func : impossible) {
+      fail(std::string("Cannot both keep and split out function ") +
+           func.c_str());
+    }
   }
 
   return valid;
@@ -719,9 +794,12 @@ int main(int argc, const char* argv[]) {
     Fatal() << "error validating input";
   }
 
-  if (options.instrument) {
-    instrumentModule(wasm, options);
-  } else {
-    splitModule(wasm, options);
+  switch (options.mode) {
+    case WasmSplitOptions::Mode::Split:
+      splitModule(wasm, options);
+      break;
+    case WasmSplitOptions::Mode::Instrument:
+      instrumentModule(wasm, options);
+      break;
   }
 }
diff --git a/test/lit/lit.cfg.py b/test/lit/lit.cfg.py
index df3a21079..60ba1ced6 100644
--- a/test/lit/lit.cfg.py
+++ b/test/lit/lit.cfg.py
@@ -3,7 +3,7 @@ import lit.formats
 config.name = "Binaryen lit tests"
 config.test_format = lit.formats.ShTest(True)
 
-config.suffixes = ['.wat', '.wast']
+config.suffixes = ['.wat', '.wast', '.test']
 
 config.test_source_root = os.path.dirname(__file__)
 config.test_exec_root = os.path.join(config.binaryen_build_root, 'test')
diff --git a/test/lit/wasm-split/help.test b/test/lit/wasm-split/help.test
new file mode 100644
index 000000000..c473b28a1
--- /dev/null
+++ b/test/lit/wasm-split/help.test
@@ -0,0 +1,68 @@
+;; RUN: wasm-split --help | filecheck %s
+
+CHECK:      wasm-split INFILE
+CHECK-NEXT:
+CHECK-NEXT: Split a module into a primary module and a secondary module, or instrument a
+CHECK-NEXT: module to gather a profile that can inform future splitting, or manage such
+CHECK-NEXT: profiles. Options that are only accepted in particular modes are marked with the
+CHECK-NEXT: accepted "[<modes>]" in their descriptions.
+CHECK-NEXT:
+CHECK-NEXT: Options:
+
+;; Skip standard tool options
+
+CHECK:        --split                              Split an input module into two output
+CHECK-NEXT:                                        modules. The default mode.
+CHECK-NEXT:   --instrument                         Instrument an input module to allow it to
+CHECK-NEXT:                                        generate a profile that can be used to
+CHECK-NEXT:                                        guide splitting.
+CHECK-NEXT:   --profile                            [split] The profile to use to guide
+CHECK-NEXT:                                        splitting.
+CHECK-NEXT:   --keep-funcs                         [split] Comma-separated list of functions
+CHECK-NEXT:                                        to keep in the primary module, regardless
+CHECK-NEXT:                                        of any profile.
+CHECK-NEXT:   --split-funcs                        [split] Comma-separated list of functions
+CHECK-NEXT:                                        to split into the secondary module,
+CHECK-NEXT:                                        regardless of any profile. If there is no
+CHECK-NEXT:                                        profile, then this defaults to all
+CHECK-NEXT:                                        functions defined in the module.
+CHECK-NEXT:   --primary-output,-o1                 [split] Output file for the primary
+CHECK-NEXT:                                        module.
+CHECK-NEXT:   --secondary-output,-o2               [split] Output file for the secondary
+CHECK-NEXT:                                        module.
+CHECK-NEXT:   --symbolmap                          [split] Write a symbol map file for each
+CHECK-NEXT:                                        of the output modules.
+CHECK-NEXT:   --import-namespace                   [split] The namespace from which to
+CHECK-NEXT:                                        import objects from the primary module
+CHECK-NEXT:                                        into the secondary module.
+CHECK-NEXT:   --placeholder-namespace              [split] The namespace from which to
+CHECK-NEXT:                                        import placeholder functions into the
+CHECK-NEXT:                                        primary module.
+CHECK-NEXT:   --export-prefix                      [split] An identifying prefix to prepend
+CHECK-NEXT:                                        to new export names created by module
+CHECK-NEXT:                                        splitting.
+CHECK-NEXT:   --output,-o                          [instrument] Output file.
+CHECK-NEXT:   --profile-export                     [instrument] The export name of the
+CHECK-NEXT:                                        function the embedder calls to write the
+CHECK-NEXT:                                        profile into memory. Defaults to
+CHECK-NEXT:                                        `__write_profile`.
+CHECK-NEXT:   --emit-module-names                  [split, instrument] Emit module names,
+CHECK-NEXT:                                        even if not emitting the rest of the
+CHECK-NEXT:                                        names section. Can help differentiate the
+CHECK-NEXT:                                        modules in stack traces. This option will
+CHECK-NEXT:                                        be removed once simpler ways of naming
+CHECK-NEXT:                                        modules are widely available. See
+CHECK-NEXT:                                        https://bugs.chromium.org/p/v8/issues/detail?id=11808.
+CHECK-NEXT:   --initial-table                      [split, instrument] A hack to ensure the
+CHECK-NEXT:                                        split and instrumented modules have the
+CHECK-NEXT:                                        same table size when using Emscripten's
+CHECK-NEXT:                                        SPLIT_MODULE mode with dynamic linking.
+CHECK-NEXT:                                        TODO: Figure out a more elegant solution
+CHECK-NEXT:                                        for that use case and remove this.
+CHECK-NEXT:   --verbose,-v                         Verbose output mode. Prints the functions
+CHECK-NEXT:                                        that will be kept and split out when
+CHECK-NEXT:                                        splitting a module.
+CHECK-NEXT:   --emit-text,-S                       Emit text instead of binary for the
+CHECK-NEXT:                                        output file or files.
+CHECK-NEXT:   --debuginfo,-g                       Emit names section in wasm binary (or
+CHECK-NEXT:                                        full debuginfo in wast)
diff --git a/test/lit/wasm-split/invalid-options.wast b/test/lit/wasm-split/invalid-options.wast
index 32899b87b..89a68e6aa 100644
--- a/test/lit/wasm-split/invalid-options.wast
+++ b/test/lit/wasm-split/invalid-options.wast
@@ -39,32 +39,32 @@
 
 ;; Split mode requires -o1 and -o2 rather than -o
 ;; RUN: not wasm-split %s -o %t 2>&1 \
-;; RUN:   | filecheck %s --check-prefix NO-INSTRUMENT-OUT
+;; RUN:   | filecheck %s --check-prefix SPLIT-OUT
 
 ;; --instrument is required to use --profile-export
 ;; RUN: not wasm-split %s --profile-export=foo 2>&1 \
-;; RUN:   | filecheck %s --check-prefix NO-INSTRUMENT-PROFILE-EXPORT
+;; RUN:   | filecheck %s --check-prefix SPLIT-PROFILE-EXPORT
 
-;; INSTRUMENT-PROFILE: error: --profile cannot be used with --instrument
+;; INSTRUMENT-PROFILE: error: Option --profile cannot be used in instrument mode.
 
-;; INSTRUMENT-OUT1: error: primary output cannot be used with --instrument
+;; INSTRUMENT-OUT1: error: Option --primary-output cannot be used in instrument mode.
 
-;; INSTRUMENT-OUT2: error: secondary output cannot be used with --instrument
+;; INSTRUMENT-OUT2: error: Option --secondary-output cannot be used in instrument mode.
 
-;; INSTRUMENT-SYMBOLMAP: error: --symbolmap cannot be used with --instrument
+;; INSTRUMENT-SYMBOLMAP: error: Option --symbolmap cannot be used in instrument mode.
 
-;; INSTRUMENT-IMPORT-NS: error: --import-namespace cannot be used with --instrument
+;; INSTRUMENT-IMPORT-NS: error: Option --import-namespace cannot be used in instrument mode.
 
-;; INSTRUMENT-PLACEHOLDER-NS: error: --placeholder-namespace cannot be used with --instrument
+;; INSTRUMENT-PLACEHOLDER-NS: error: Option --placeholder-namespace cannot be used in instrument mode.
 
-;; INSTRUMENT-EXPORT-PREFIX: error: --export-prefix cannot be used with --instrument
+;; INSTRUMENT-EXPORT-PREFIX: error: Option --export-prefix cannot be used in instrument mode.
 
-;; INSTRUMENT-KEEP-FUNCS: error: --keep-funcs cannot be used with --instrument
+;; INSTRUMENT-KEEP-FUNCS: error: Option --keep-funcs cannot be used in instrument mode.
 
-;; INSTRUMENT-SPLIT-FUNCS: error: --split-funcs cannot be used with --instrument
+;; INSTRUMENT-SPLIT-FUNCS: error: Option --split-funcs cannot be used in instrument mode.
 
-;; NO-INSTRUMENT-OUT: error: must provide separate primary and secondary output with -o1 and -o2
+;; SPLIT-OUT: error: Option --output cannot be used in split mode.
 
-;; NO-INSTRUMENT-PROFILE-EXPORT: error: --profile-export must be used with --instrument
+;; SPLIT-PROFILE-EXPORT: error: Option --profile-export cannot be used in split mode.
 
 (module)