Use target features section in wasm-opt (#1967)

If the user does not supply features explicitly on the command line,
read and use the features in the target features section for
validation and passes. If the user does supply features explicitly,
error if they are not a superset of the features marked as used in the
target features section and the user does not explicitly handle this.

11 files changed, 198 insertions, 91 deletions

diff --git a/src/ir/module-utils.h b/src/ir/module-utils.h
index 66d3a1015..259ec9fea 100644
--- a/src/ir/module-utils.h
+++ b/src/ir/module-utils.h
@@ -17,6 +17,7 @@
 #ifndef wasm_ir_module_h
 #define wasm_ir_module_h
 
+#include "wasm-binary.h"
 #include "wasm.h"
 #include "ir/find_all.h"
 #include "ir/manipulation.h"
@@ -68,6 +69,75 @@ struct BinaryIndexes {
   }
 };
 
+// Read the features section into the out param, if it is present. If it is not
+// present, return false
+inline bool readFeaturesSection(const Module& module, FeatureSet& features) try {
+  features = FeatureSet::MVP;
+
+  const UserSection* section = nullptr;
+  for (auto &s : module.userSections) {
+    if (s.name == BinaryConsts::UserSections::TargetFeatures) {
+      section = &s;
+      break;
+    }
+  }
+
+  if (!section) {
+    return false;
+  }
+
+  // read target features section
+  size_t index = 0;
+  auto next_byte = [&]() {
+    return section->data.at(index++);
+  };
+
+  size_t num_feats = U32LEB().read(next_byte).value;
+  for (size_t i = 0; i < num_feats; ++i) {
+    uint8_t prefix = section->data.at(index++);
+    if (prefix != '=' && prefix != '+' && prefix != '-') {
+      // unrecognized prefix, silently fail
+      features = FeatureSet::MVP;
+      return false;
+    }
+
+    size_t len = U32LEB().read(next_byte).value;
+    if (index + len > section->data.size()) {
+      // ill-formed string, silently fail
+      features = FeatureSet::MVP;
+      return false;
+    }
+    std::string name(&section->data[index], len);
+    index += len;
+
+    if (prefix == '-') {
+      continue;
+    }
+
+    if (name == BinaryConsts::UserSections::AtomicsFeature) {
+      features.setAtomics();
+    } else if (name == BinaryConsts::UserSections::BulkMemoryFeature) {
+      features.setBulkMemory();
+    } else if (name == BinaryConsts::UserSections::ExceptionHandlingFeature) {
+      WASM_UNREACHABLE(); // TODO: exception handling
+    } else if (name == BinaryConsts::UserSections::TruncSatFeature) {
+      features.setTruncSat();
+    } else if (name == BinaryConsts::UserSections::SignExtFeature) {
+      features.setSignExt();
+    } else if (name == BinaryConsts::UserSections::SIMD128Feature) {
+      features.setSIMD();
+    }
+  }
+
+  return true;
+
+// silently fail to read features. Maybe this should warn?
+} catch (std::out_of_range& e) {
+  features = FeatureSet::MVP;
+  return false;
+}
+
+
 inline Function* copyFunction(Function* func, Module& out) {
   auto* ret = new Function();
   ret->name = func->name;
@@ -254,4 +324,3 @@ inline void iterDefinedFunctions(Module& wasm, T visitor) {
 } // namespace wasm
 
 #endif // wasm_ir_module_h
-
diff --git a/src/support/command-line.h b/src/support/command-line.h
index 89bf72070..de773bfa8 100644
--- a/src/support/command-line.h
+++ b/src/support/command-line.h
@@ -34,7 +34,7 @@ namespace wasm {
 
 class Options {
  public:
-  typedef std::function<void(Options *, const std::string& )> Action;
+  using Action = std::function<void(Options *, const std::string& )>;
   enum class Arguments { Zero, One, N, Optional };
 
   bool debug;
@@ -44,7 +44,7 @@ class Options {
   ~Options();
   Options &add(const std::string& longName, const std::string& shortName,
                const std::string& description, Arguments arguments,
-               const Action &action);
+               const Action& action);
   Options &add_positional(const std::string& name, Arguments arguments,
                           const Action &action);
   void parse(int argc, const char *argv[]);
diff --git a/src/tools/asm2wasm.cpp b/src/tools/asm2wasm.cpp
index 4907033e5..9ec083db2 100644
--- a/src/tools/asm2wasm.cpp
+++ b/src/tools/asm2wasm.cpp
@@ -185,7 +185,7 @@ int main(int argc, const char *argv[]) {
   if (memInit != options.extra.end()) {
     if (options.runningDefaultOptimizationPasses()) {
       PassRunner runner(&wasm);
-      runner.setFeatures(options.getFeatures());
+      runner.setFeatures(options.passOptions.features);
       runner.add("memory-packing");
       runner.run();
     }
diff --git a/src/tools/tool-options.h b/src/tools/tool-options.h
index 671085a4e..5b09b8d23 100644
--- a/src/tools/tool-options.h
+++ b/src/tools/tool-options.h
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include "ir/module-utils.h"
 #include "support/command-line.h"
 #include "pass.h"
 
@@ -30,92 +31,104 @@ struct ToolOptions : public Options {
       : Options(command, description) {
     (*this)
         .add("--mvp-features", "-mvp", "Disable all non-MVP features",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features = FeatureSet::MVP;
-             })
-        .add("--all-features", "-all", "Enable all features (default)",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features = FeatureSet::All;
-             })
-        .add("--enable-sign-ext", "", "Enable sign extension operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setSignExt();
-             })
-        .add("--disable-sign-ext", "", "Disable sign extension operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setSignExt(false);
-             })
-        .add("--enable-threads", "", "Enable atomic operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setAtomics();
-             })
-        .add("--disable-threads", "", "Disable atomic operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setAtomics(false);
-             })
-        .add("--enable-mutable-globals", "", "Enable mutable globals",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setMutableGlobals();
-             })
-        .add("--disable-mutable-globals", "", "Disable mutable globals",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setMutableGlobals(false);
-             })
-        .add("--enable-nontrapping-float-to-int", "",
-             "Enable nontrapping float-to-int operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setTruncSat();
-             })
-        .add("--disable-nontrapping-float-to-int", "",
-             "Disable nontrapping float-to-int operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setTruncSat(false);
+             Arguments::Zero,
+             [this](Options*, const std::string&) {
+               hasFeatureOptions = true;
+               passOptions.features.makeMVP();
+               enabledFeatures.makeMVP();
+               disabledFeatures.setAll();
              })
-        .add("--enable-simd", "",
-             "Enable SIMD operations and types",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setSIMD();
+        .add("--all-features", "-all", "Enable all features "
+             "(default if there is no target features section"
+             " or if any feature options are provided)",
+             Arguments::Zero,
+             [this](Options*, const std::string&) {
+               hasFeatureOptions = true;
+               passOptions.features.setAll();
+               enabledFeatures.setAll();
+               disabledFeatures.makeMVP();
              })
-        .add("--disable-simd", "",
-             "Disable SIMD operations and types",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setSIMD(false);
-             })
-        .add("--enable-bulk-memory", "",
-             "Enable bulk memory operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setBulkMemory();
-             })
-        .add("--disable-bulk-memory", "",
-             "Disable bulk memory operations",
-             Options::Arguments::Zero,
-             [this](Options *o, const std::string& arguments) {
-               passOptions.features.setBulkMemory(false);
-             })
-        .add("--no-validation", "-n", "Disables validation, assumes inputs are correct",
+        .add("--detect-features", "",
+             "Use features from the target features section"
+             "(default if there is a target features section"
+             " and no feature options are provided)",
+             Arguments::Zero,
+             [this](Options*, const std::string&) {
+               hasFeatureOptions = true;
+               detectFeatures = true;
+               enabledFeatures.makeMVP();
+               disabledFeatures.makeMVP();
+             });
+    (*this)
+        .addFeature("sign-ext", "sign extension operations",
+                    FeatureSet::SignExt)
+        .addFeature("threads", "atomic operations",
+                    FeatureSet::Atomics)
+        .addFeature("mutable-globals", "mutable globals",
+                    FeatureSet::MutableGlobals)
+        .addFeature("nontrapping-float-to-int",
+                    "nontrapping float-to-int operations",
+                    FeatureSet::TruncSat)
+        .addFeature("simd", "SIMD operations and types",
+                    FeatureSet::SIMD)
+        .addFeature("bulk-memory", "bulk memory operations",
+                    FeatureSet::BulkMemory)
+        .add("--no-validation", "-n",
+             "Disables validation, assumes inputs are correct",
              Options::Arguments::Zero,
              [this](Options* o, const std::string& argument) {
                passOptions.validate = false;
              });
-        ;
   }
 
-  FeatureSet getFeatures() const {
-    return passOptions.features;
+  ToolOptions& addFeature(const std::string& name,
+                          const std::string& description,
+                          FeatureSet::Feature feature) {
+    (*this)
+        .add(std::string("--enable-") + name, "",
+             std::string("Enable ") + description, Arguments::Zero,
+             [=](Options*, const std::string&) {
+               hasFeatureOptions = true;
+               passOptions.features.set(feature, true);
+               enabledFeatures.set(feature, true);
+               disabledFeatures.set(feature, false);
+             })
+
+        .add(std::string("--disable-") + name, "",
+             std::string("Disable ") + description, Arguments::Zero,
+             [=](Options*, const std::string&) {
+               hasFeatureOptions = true;
+               passOptions.features.set(feature, false);
+               enabledFeatures.set(feature, false);
+               disabledFeatures.set(feature, true);
+             });
+    return *this;
   }
+
+  void calculateFeatures(const Module& module) {
+    FeatureSet wasmFeatures;
+    bool wasmHasFeatures =
+        ModuleUtils::readFeaturesSection(module, wasmFeatures);
+
+    if (hasFeatureOptions) {
+      if (detectFeatures) {
+        wasmFeatures.enable(enabledFeatures);
+        wasmFeatures.disable(disabledFeatures);
+        passOptions.features = wasmFeatures;
+      } else if (!(wasmFeatures <= passOptions.features)) {
+        Fatal() << "module uses features not explicitly specified, "
+                << "use --detect-features to resolve";
+      }
+    } else if (wasmHasFeatures) {
+      passOptions.features = wasmFeatures;
+    }
+  }
+
+private:
+  bool hasFeatureOptions = false;
+  bool detectFeatures = false;
+  FeatureSet enabledFeatures = FeatureSet::MVP;
+  FeatureSet disabledFeatures = FeatureSet::MVP;
 };
 
 } // namespace wasm
diff --git a/src/tools/wasm-opt.cpp b/src/tools/wasm-opt.cpp
index ca4d87dd8..d36459a4a 100644
--- a/src/tools/wasm-opt.cpp
+++ b/src/tools/wasm-opt.cpp
@@ -162,8 +162,10 @@ int main(int argc, const char* argv[]) {
       Fatal() << "error in building module, std::bad_alloc (possibly invalid request for silly amounts of memory)";
     }
 
+    options.calculateFeatures(wasm);
+
     if (options.passOptions.validate) {
-      if (!WasmValidator().validate(wasm, options.getFeatures())) {
+      if (!WasmValidator().validate(wasm, options.passOptions.features)) {
         WasmPrinter::printModule(&wasm);
         Fatal() << "error in validating input";
       }
@@ -174,12 +176,12 @@ int main(int argc, const char* argv[]) {
     if (fuzzPasses) {
       reader.pickPasses(options);
     }
-    reader.setFeatures(options.getFeatures());
+    reader.setFeatures(options.passOptions.features);
     reader.setAllowNaNs(fuzzNaNs);
     reader.setAllowMemory(fuzzMemory);
     reader.build();
     if (options.passOptions.validate) {
-      if (!WasmValidator().validate(wasm, options.getFeatures())) {
+      if (!WasmValidator().validate(wasm, options.passOptions.features)) {
         WasmPrinter::printModule(&wasm);
         std::cerr << "translate-to-fuzz must always generate a valid module";
         abort();
@@ -239,7 +241,7 @@ int main(int argc, const char* argv[]) {
     WasmBinaryBuilder parser(other, input, false);
     parser.read();
     if (options.passOptions.validate) {
-      bool valid = WasmValidator().validate(other, options.getFeatures());
+      bool valid = WasmValidator().validate(other, options.passOptions.features);
       if (!valid) {
         WasmPrinter::printModule(&other);
       }
@@ -253,7 +255,8 @@ int main(int argc, const char* argv[]) {
     auto runPasses = [&]() {
       options.runPasses(*curr);
       if (options.passOptions.validate) {
-        bool valid = WasmValidator().validate(*curr, options.getFeatures());
+        bool valid =
+            WasmValidator().validate(*curr, options.passOptions.features);
         if (!valid) {
           WasmPrinter::printModule(&*curr);
         }
diff --git a/src/tools/wasm-shell.cpp b/src/tools/wasm-shell.cpp
index dcb1b2725..0d7ff61de 100644
--- a/src/tools/wasm-shell.cpp
+++ b/src/tools/wasm-shell.cpp
@@ -292,7 +292,7 @@ int main(int argc, const char* argv[]) {
         builders[moduleName].swap(builder);
         modules[moduleName].swap(module);
         i++;
-        bool valid = WasmValidator().validate(*modules[moduleName], options.getFeatures());
+        bool valid = WasmValidator().validate(*modules[moduleName], options.passOptions.features);
         if (!valid) {
           WasmPrinter::printModule(modules[moduleName].get());
         }
diff --git a/src/tools/wasm2js.cpp b/src/tools/wasm2js.cpp
index 277a4e2c9..6a55c4483 100644
--- a/src/tools/wasm2js.cpp
+++ b/src/tools/wasm2js.cpp
@@ -81,6 +81,7 @@ int main(int argc, const char *argv[]) {
       ModuleReader reader;
       reader.setDebug(options.debug);
       reader.read(input, wasm, "");
+      options.calculateFeatures(wasm);
     } else {
       auto input(
           read_file<std::vector<char>>(options.extra["infile"], Flags::Text, options.debug ? Flags::Debug : Flags::Release));
@@ -99,7 +100,7 @@ int main(int argc, const char *argv[]) {
   }
 
   if (options.passOptions.validate) {
-    if (!WasmValidator().validate(wasm, options.getFeatures())) {
+    if (!WasmValidator().validate(wasm, options.passOptions.features)) {
       WasmPrinter::printModule(&wasm);
       Fatal() << "error in validating input";
     }
diff --git a/src/wasm-binary.h b/src/wasm-binary.h
index 8b449192a..7fe543cf6 100644
--- a/src/wasm-binary.h
+++ b/src/wasm-binary.h
@@ -29,8 +29,8 @@
 #include "wasm-traversal.h"
 #include "asmjs/shared-constants.h"
 #include "asm_v_wasm.h"
-#include "wasm-builder.h"
 #include "parsing.h"
+#include "wasm-builder.h"
 #include "wasm-validator.h"
 #include "ir/import-utils.h"
 
@@ -96,7 +96,7 @@ struct LEB {
     return offset;
   }
 
-  void read(std::function<MiniT()> get) {
+  LEB<T, MiniT>& read(std::function<MiniT()> get) {
     value = 0;
     T shift = 0;
     MiniT byte;
@@ -134,6 +134,7 @@ struct LEB {
         }
       }
     }
+    return *this;
   }
 };
 
@@ -347,10 +348,19 @@ extern const char* Linking;
 extern const char* Producers;
 extern const char* TargetFeatures;
 
+extern const char* AtomicsFeature;
+extern const char* BulkMemoryFeature;
+extern const char* ExceptionHandlingFeature;
+extern const char* TruncSatFeature;
+extern const char* SignExtFeature;
+extern const char* SIMD128Feature;
+
 enum Subsection {
   NameFunction = 1,
   NameLocal = 2,
 };
+
+
 }
 
 enum ASTNodes {
diff --git a/src/wasm-features.h b/src/wasm-features.h
index 4dd806e28..49f49db09 100644
--- a/src/wasm-features.h
+++ b/src/wasm-features.h
@@ -54,6 +54,11 @@ struct FeatureSet {
   void setSignExt(bool v = true) { set(SignExt, v); }
   void setAll(bool v = true) { features = v ? All : MVP; }
 
+  void enable(const FeatureSet& other) { features |= other.features; }
+  void disable(const FeatureSet& other) {
+    features = features & ~other.features & All;
+  }
+
   bool operator<=(const FeatureSet& other) {
     return !(features & ~other.features);
   }
diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
index 643f69dde..385ee0ccb 100644
--- a/src/wasm/wasm-binary.cpp
+++ b/src/wasm/wasm-binary.cpp
@@ -17,10 +17,10 @@
 #include <algorithm>
 #include <fstream>
 
-#include "support/bits.h"
 #include "wasm-binary.h"
 #include "wasm-stack.h"
 #include "ir/module-utils.h"
+#include "support/bits.h"
 
 namespace wasm {
 
diff --git a/src/wasm/wasm.cpp b/src/wasm/wasm.cpp
index f7081e49c..e14f6455f 100644
--- a/src/wasm/wasm.cpp
+++ b/src/wasm/wasm.cpp
@@ -33,6 +33,12 @@ const char* Dylink = "dylink";
 const char* Linking = "linking";
 const char* Producers = "producers";
 const char* TargetFeatures = "target_features";
+const char* AtomicsFeature = "atomics";
+const char* BulkMemoryFeature = "bulk-memory";
+const char* ExceptionHandlingFeature = "exception-handling";
+const char* TruncSatFeature = "nontrapping-fptoint";
+const char* SignExtFeature = "sign-ext";
+const char* SIMD128Feature = "simd128";
 }
 }