Apply features from the commandline first (#3960)

As suggested in

https://github.com/WebAssembly/binaryen/pull/3955#issuecomment-871016647

This applies commandline features first. If the features section is present, and
disallows some of them, then we warn. Otherwise, the features can combine
(for example, a wasm may enable feature X because it has to use it, and a user
can simply add the flag for feature Y if they want the optimizer to try to use it;
both flags will then be enabled).

This is important because in some cases we need to know the features before
parsing the wasm, in the case that the wasm does not use the features section.
In particular, non-nullable GC locals have an effect during parsing. (Typed
function references also does, but we found a way to apply its effect all the time,
that is, always use the refined type, and that happened to not break the case
where the feature is disabled - but such a workaround is not possible with
non-nullable locals.)

To make this less error-prone, add a FeatureSet input as a parameter to
WasmBinaryBuilder. That is, when building a module, we must give it the
features to use while doing so.

This will unblock #3955 . That PR will also add a test for the actual usage
of a feature during loading (the test can only be added there, after that PR
unbreaks things).

14 files changed, 82 insertions, 90 deletions

diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
index d7f23e259..2260a93b1 100644
--- a/src/binaryen-c.cpp
+++ b/src/binaryen-c.cpp
@@ -3974,7 +3974,8 @@ BinaryenModuleRef BinaryenModuleRead(char* input, size_t inputSize) {
   buffer.resize(inputSize);
   std::copy_n(input, inputSize, buffer.begin());
   try {
-    WasmBinaryBuilder parser(*wasm, buffer);
+    // TODO: allow providing features in the C API
+    WasmBinaryBuilder parser(*wasm, FeatureSet::MVP, buffer);
     parser.read();
   } catch (ParseException& p) {
     p.dump(std::cerr);
diff --git a/src/passes/RoundTrip.cpp b/src/passes/RoundTrip.cpp
index 392710617..628cc5d4e 100644
--- a/src/passes/RoundTrip.cpp
+++ b/src/passes/RoundTrip.cpp
@@ -33,13 +33,14 @@ struct RoundTrip : public Pass {
   void run(PassRunner* runner, Module* module) override {
     BufferWithRandomAccess buffer;
     // Save features, which would not otherwise make it through a round trip if
-    // the target features section has been stripped.
+    // the target features section has been stripped. We also need them in order
+    // to tell the builder which features to build with.
     auto features = module->features;
     // Write, clear, and read the module
     WasmBinaryWriter(module, buffer).write();
     ModuleUtils::clearModule(*module);
     auto input = buffer.getAsChars();
-    WasmBinaryBuilder parser(*module, input);
+    WasmBinaryBuilder parser(*module, features, input);
     parser.setDWARF(runner->options.debugInfo);
     try {
       parser.read();
@@ -48,8 +49,6 @@ struct RoundTrip : public Pass {
       std::cerr << '\n';
       Fatal() << "error in parsing wasm binary";
     }
-    // Reapply features
-    module->features = features;
   }
 };
 
diff --git a/src/tools/tool-options.h b/src/tools/tool-options.h
index 350b7babd..0c40721d3 100644
--- a/src/tools/tool-options.h
+++ b/src/tools/tool-options.h
@@ -41,7 +41,6 @@ struct ToolOptions : public Options {
            "Disable all non-MVP features",
            Arguments::Zero,
            [this](Options*, const std::string&) {
-             hasFeatureOptions = true;
              enabledFeatures.setMVP();
              disabledFeatures.setAll();
            })
@@ -50,20 +49,14 @@ struct ToolOptions : public Options {
            "Enable all features",
            Arguments::Zero,
            [this](Options*, const std::string&) {
-             hasFeatureOptions = true;
              enabledFeatures.setAll();
              disabledFeatures.setMVP();
            })
       .add("--detect-features",
            "",
-           "Use features from the target features section, or MVP (default)",
+           "(deprecated - this flag does nothing)",
            Arguments::Zero,
-           [this](Options*, const std::string&) {
-             hasFeatureOptions = true;
-             detectFeatures = true;
-             enabledFeatures.setMVP();
-             disabledFeatures.setMVP();
-           })
+           [](Options*, const std::string&) {})
       .add("--quiet",
            "-q",
            "Emit less verbose output and hide trivial warnings.",
@@ -134,7 +127,6 @@ struct ToolOptions : public Options {
            std::string("Enable ") + description,
            Arguments::Zero,
            [=](Options*, const std::string&) {
-             hasFeatureOptions = true;
              enabledFeatures.set(feature, true);
              disabledFeatures.set(feature, false);
            })
@@ -144,7 +136,6 @@ struct ToolOptions : public Options {
            std::string("Disable ") + description,
            Arguments::Zero,
            [=](Options*, const std::string&) {
-             hasFeatureOptions = true;
              enabledFeatures.set(feature, false);
              disabledFeatures.set(feature, true);
            });
@@ -152,24 +143,11 @@ struct ToolOptions : public Options {
   }
 
   void applyFeatures(Module& module) const {
-    if (hasFeatureOptions) {
-      if (!detectFeatures && module.hasFeaturesSection) {
-        FeatureSet optionsFeatures = FeatureSet::MVP;
-        optionsFeatures.enable(enabledFeatures);
-        optionsFeatures.disable(disabledFeatures);
-        if (!(module.features <= optionsFeatures)) {
-          Fatal() << "features section is not a subset of specified features. "
-                  << "Use --detect-features to resolve.";
-        }
-      }
-      module.features.enable(enabledFeatures);
-      module.features.disable(disabledFeatures);
-    }
+    module.features.enable(enabledFeatures);
+    module.features.disable(disabledFeatures);
   }
 
 private:
-  bool hasFeatureOptions = false;
-  bool detectFeatures = false;
   FeatureSet enabledFeatures = FeatureSet::MVP;
   FeatureSet disabledFeatures = FeatureSet::MVP;
 };
diff --git a/src/tools/wasm-as.cpp b/src/tools/wasm-as.cpp
index be18ed355..2b854a2a0 100644
--- a/src/tools/wasm-as.cpp
+++ b/src/tools/wasm-as.cpp
@@ -99,6 +99,7 @@ int main(int argc, const char* argv[]) {
   auto input(read_file<std::string>(options.extra["infile"], Flags::Text));
 
   Module wasm;
+  options.applyFeatures(wasm);
 
   try {
     if (options.debug) {
@@ -115,8 +116,6 @@ int main(int argc, const char* argv[]) {
     Fatal() << "error in parsing input";
   }
 
-  options.applyFeatures(wasm);
-
   if (options.extra["validate"] != "none") {
     if (options.debug) {
       std::cerr << "Validating..." << std::endl;
diff --git a/src/tools/wasm-ctor-eval.cpp b/src/tools/wasm-ctor-eval.cpp
index bd370a58a..04ef10f57 100644
--- a/src/tools/wasm-ctor-eval.cpp
+++ b/src/tools/wasm-ctor-eval.cpp
@@ -549,6 +549,7 @@ int main(int argc, const char* argv[]) {
   auto input(read_file<std::string>(options.extra["infile"], Flags::Text));
 
   Module wasm;
+  options.applyFeatures(wasm);
 
   {
     if (options.debug) {
@@ -563,8 +564,6 @@ int main(int argc, const char* argv[]) {
     }
   }
 
-  options.applyFeatures(wasm);
-
   if (!WasmValidator().validate(wasm)) {
     std::cout << wasm << '\n';
     Fatal() << "error in validating input";
diff --git a/src/tools/wasm-dis.cpp b/src/tools/wasm-dis.cpp
index fc07b771d..cca87514a 100644
--- a/src/tools/wasm-dis.cpp
+++ b/src/tools/wasm-dis.cpp
@@ -60,6 +60,7 @@ int main(int argc, const char* argv[]) {
     std::cerr << "parsing binary..." << std::endl;
   }
   Module wasm;
+  options.applyFeatures(wasm);
   try {
     ModuleReader().readBinary(options.extra["infile"], wasm, sourceMapFilename);
   } catch (ParseException& p) {
@@ -72,8 +73,6 @@ int main(int argc, const char* argv[]) {
     Fatal() << "error in parsing wasm source mapping";
   }
 
-  options.applyFeatures(wasm);
-
   // TODO: Validation. However, validating would mean that users are forced to
   //       run with  wasm-dis -all  or such, to enable the features (unless the
   //       features section is present, but that's rare in general). It would be
diff --git a/src/tools/wasm-emscripten-finalize.cpp b/src/tools/wasm-emscripten-finalize.cpp
index c69ec9bb5..4caa62b08 100644
--- a/src/tools/wasm-emscripten-finalize.cpp
+++ b/src/tools/wasm-emscripten-finalize.cpp
@@ -209,6 +209,7 @@ int main(int argc, const char* argv[]) {
   auto writeOutput = outfile.size() > 0 || !emitBinary;
 
   Module wasm;
+  options.applyFeatures(wasm);
   ModuleReader reader;
   // If we are not writing output then we definitely don't need to read debug
   // info, as it does not affect the metadata we will emit. (However, if we
@@ -239,8 +240,6 @@ int main(int argc, const char* argv[]) {
     Fatal() << "error in parsing wasm source map";
   }
 
-  options.applyFeatures(wasm);
-
   BYN_TRACE_WITH_TYPE("emscripten-dump", "Module before:\n");
   BYN_DEBUG_WITH_TYPE("emscripten-dump", std::cerr << &wasm);
 
diff --git a/src/tools/wasm-metadce.cpp b/src/tools/wasm-metadce.cpp
index 29ac01f90..026b6ad32 100644
--- a/src/tools/wasm-metadce.cpp
+++ b/src/tools/wasm-metadce.cpp
@@ -504,6 +504,7 @@ int main(int argc, const char* argv[]) {
   auto input(read_file<std::string>(options.extra["infile"], Flags::Text));
 
   Module wasm;
+  options.applyFeatures(wasm);
 
   {
     if (options.debug) {
@@ -519,8 +520,6 @@ int main(int argc, const char* argv[]) {
     }
   }
 
-  options.applyFeatures(wasm);
-
   if (options.passOptions.validate) {
     if (!WasmValidator().validate(wasm)) {
       std::cout << wasm << '\n';
diff --git a/src/tools/wasm-opt.cpp b/src/tools/wasm-opt.cpp
index 45d394162..5aa0c374a 100644
--- a/src/tools/wasm-opt.cpp
+++ b/src/tools/wasm-opt.cpp
@@ -217,6 +217,7 @@ int main(int argc, const char* argv[]) {
   options.parse(argc, argv);
 
   Module wasm;
+  options.applyFeatures(wasm);
 
   BYN_TRACE("reading...\n");
 
@@ -259,8 +260,6 @@ int main(int argc, const char* argv[]) {
                  "request for silly amounts of memory)";
     }
 
-    options.applyFeatures(wasm);
-
     if (options.passOptions.validate) {
       if (!WasmValidator().validate(wasm)) {
         exitOnInvalidWasm("error validating input");
@@ -268,7 +267,6 @@ int main(int argc, const char* argv[]) {
     }
   }
   if (translateToFuzz) {
-    options.applyFeatures(wasm);
     TranslateToFuzzReader reader(wasm, options.extra["infile"]);
     if (fuzzPasses) {
       reader.pickPasses(options);
diff --git a/src/tools/wasm-split.cpp b/src/tools/wasm-split.cpp
index 261c62a68..6d4e8ed5c 100644
--- a/src/tools/wasm-split.cpp
+++ b/src/tools/wasm-split.cpp
@@ -396,6 +396,7 @@ void WasmSplitOptions::parse(int argc, const char* argv[]) {
 }
 
 void parseInput(Module& wasm, const WasmSplitOptions& options) {
+  options.applyFeatures(wasm);
   ModuleReader reader;
   reader.setProfile(options.profile);
   try {
@@ -408,7 +409,6 @@ void parseInput(Module& wasm, const WasmSplitOptions& options) {
     Fatal() << "error building module, std::bad_alloc (possibly invalid "
                "request for silly amounts of memory)";
   }
-  options.applyFeatures(wasm);
 
   if (options.passOptions.validate && !WasmValidator().validate(wasm)) {
     Fatal() << "error validating input";
diff --git a/src/wasm-binary.h b/src/wasm-binary.h
index 51c2b3e52..6938214d5 100644
--- a/src/wasm-binary.h
+++ b/src/wasm-binary.h
@@ -1338,9 +1338,9 @@ class WasmBinaryBuilder {
   std::vector<HeapType> types;
 
 public:
-  WasmBinaryBuilder(Module& wasm, const std::vector<char>& input)
-    : wasm(wasm), allocator(wasm.allocator), input(input), sourceMap(nullptr),
-      nextDebugLocation(0, {0, 0, 0}), debugLocation() {}
+  WasmBinaryBuilder(Module& wasm,
+                    FeatureSet features,
+                    const std::vector<char>& input);
 
   void setDebugInfo(bool value) { debugInfo = value; }
   void setDWARF(bool value) { DWARF = value; }
diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
index 31fe1bc6e..581dbd6a2 100644
--- a/src/wasm/wasm-binary.cpp
+++ b/src/wasm/wasm-binary.cpp
@@ -1319,6 +1319,14 @@ void WasmBinaryWriter::writeField(const Field& field) {
 
 // reader
 
+WasmBinaryBuilder::WasmBinaryBuilder(Module& wasm,
+                                     FeatureSet features,
+                                     const std::vector<char>& input)
+  : wasm(wasm), allocator(wasm.allocator), input(input), sourceMap(nullptr),
+    nextDebugLocation(0, {0, 0, 0}), debugLocation() {
+  wasm.features = features;
+}
+
 bool WasmBinaryBuilder::hasDWARFSections() {
   assert(pos == 0);
   getInt32(); // magic
@@ -3186,22 +3194,21 @@ void WasmBinaryBuilder::readNames(size_t payloadLen) {
 
 void WasmBinaryBuilder::readFeatures(size_t payloadLen) {
   wasm.hasFeaturesSection = true;
-  wasm.features = FeatureSet::MVP;
 
   auto sectionPos = pos;
   size_t numFeatures = getU32LEB();
   for (size_t i = 0; i < numFeatures; ++i) {
     uint8_t prefix = getInt8();
-    if (prefix != BinaryConsts::FeatureUsed) {
-      if (prefix == BinaryConsts::FeatureRequired) {
-        std::cerr
-          << "warning: required features in feature section are ignored";
-      } else if (prefix == BinaryConsts::FeatureDisallowed) {
-        std::cerr
-          << "warning: disallowed features in feature section are ignored";
-      } else {
-        throwError("Unrecognized feature policy prefix");
-      }
+
+    bool disallowed = prefix == BinaryConsts::FeatureDisallowed;
+    bool required = prefix == BinaryConsts::FeatureRequired;
+    bool used = prefix == BinaryConsts::FeatureUsed;
+
+    if (!disallowed && !required && !used) {
+      throwError("Unrecognized feature policy prefix");
+    }
+    if (required) {
+      std::cerr << "warning: required features in feature section are ignored";
     }
 
     Name name = getInlineString();
@@ -3209,35 +3216,46 @@ void WasmBinaryBuilder::readFeatures(size_t payloadLen) {
       throwError("ill-formed string extends beyond section");
     }
 
-    if (prefix != BinaryConsts::FeatureDisallowed) {
-      if (name == BinaryConsts::UserSections::AtomicsFeature) {
-        wasm.features.setAtomics();
-      } else if (name == BinaryConsts::UserSections::BulkMemoryFeature) {
-        wasm.features.setBulkMemory();
-      } else if (name == BinaryConsts::UserSections::ExceptionHandlingFeature) {
-        wasm.features.setExceptionHandling();
-      } else if (name == BinaryConsts::UserSections::MutableGlobalsFeature) {
-        wasm.features.setMutableGlobals();
-      } else if (name == BinaryConsts::UserSections::TruncSatFeature) {
-        wasm.features.setTruncSat();
-      } else if (name == BinaryConsts::UserSections::SignExtFeature) {
-        wasm.features.setSignExt();
-      } else if (name == BinaryConsts::UserSections::SIMD128Feature) {
-        wasm.features.setSIMD();
-      } else if (name == BinaryConsts::UserSections::TailCallFeature) {
-        wasm.features.setTailCall();
-      } else if (name == BinaryConsts::UserSections::ReferenceTypesFeature) {
-        wasm.features.setReferenceTypes();
-      } else if (name == BinaryConsts::UserSections::MultivalueFeature) {
-        wasm.features.setMultivalue();
-      } else if (name == BinaryConsts::UserSections::GCFeature) {
-        wasm.features.setGC();
-      } else if (name == BinaryConsts::UserSections::Memory64Feature) {
-        wasm.features.setMemory64();
-      } else if (name ==
-                 BinaryConsts::UserSections::TypedFunctionReferencesFeature) {
-        wasm.features.setTypedFunctionReferences();
-      }
+    FeatureSet feature;
+    if (name == BinaryConsts::UserSections::AtomicsFeature) {
+      feature = FeatureSet::Atomics;
+    } else if (name == BinaryConsts::UserSections::BulkMemoryFeature) {
+      feature = FeatureSet::BulkMemory;
+    } else if (name == BinaryConsts::UserSections::ExceptionHandlingFeature) {
+      feature = FeatureSet::ExceptionHandling;
+    } else if (name == BinaryConsts::UserSections::MutableGlobalsFeature) {
+      feature = FeatureSet::MutableGlobals;
+    } else if (name == BinaryConsts::UserSections::TruncSatFeature) {
+      feature = FeatureSet::TruncSat;
+    } else if (name == BinaryConsts::UserSections::SignExtFeature) {
+      feature = FeatureSet::SignExt;
+    } else if (name == BinaryConsts::UserSections::SIMD128Feature) {
+      feature = FeatureSet::SIMD;
+    } else if (name == BinaryConsts::UserSections::TailCallFeature) {
+      feature = FeatureSet::TailCall;
+    } else if (name == BinaryConsts::UserSections::ReferenceTypesFeature) {
+      feature = FeatureSet::ReferenceTypes;
+    } else if (name == BinaryConsts::UserSections::MultivalueFeature) {
+      feature = FeatureSet::Multivalue;
+    } else if (name == BinaryConsts::UserSections::GCFeature) {
+      feature = FeatureSet::GC;
+    } else if (name == BinaryConsts::UserSections::Memory64Feature) {
+      feature = FeatureSet::Memory64;
+    } else if (name ==
+               BinaryConsts::UserSections::TypedFunctionReferencesFeature) {
+      feature = FeatureSet::TypedFunctionReferences;
+    } else {
+      // Silently ignore unknown features (this may be and old binaryen running
+      // on a new wasm).
+    }
+
+    if (disallowed && wasm.features.has(feature)) {
+      std::cerr
+        << "warning: feature " << feature.toString()
+        << " was enabled by the user, but disallowed in the features section.";
+    }
+    if (required || used) {
+      wasm.features.enable(feature);
     }
   }
   if (pos != sectionPos + payloadLen) {
diff --git a/src/wasm/wasm-io.cpp b/src/wasm/wasm-io.cpp
index 795bf40a5..b8d67f4d5 100644
--- a/src/wasm/wasm-io.cpp
+++ b/src/wasm/wasm-io.cpp
@@ -49,7 +49,9 @@ void ModuleReader::readBinaryData(std::vector<char>& input,
                                   Module& wasm,
                                   std::string sourceMapFilename) {
   std::unique_ptr<std::ifstream> sourceMapStream;
-  WasmBinaryBuilder parser(wasm, input);
+  // Assume that the wasm has had its initial features applied, and use those
+  // while parsing.
+  WasmBinaryBuilder parser(wasm, wasm.features, input);
   parser.setDebugInfo(debugInfo);
   parser.setDWARF(DWARF);
   parser.setSkipFunctionBodies(skipFunctionBodies);
diff --git a/src/wasm/wasm-s-parser.cpp b/src/wasm/wasm-s-parser.cpp
index 25be13f58..019049cb1 100644
--- a/src/wasm/wasm-s-parser.cpp
+++ b/src/wasm/wasm-s-parser.cpp
@@ -362,7 +362,8 @@ SExpressionWasmBuilder::SExpressionWasmBuilder(Module& wasm,
         stringToBinary(str, size, data);
       }
     }
-    WasmBinaryBuilder binaryBuilder(wasm, data);
+    // TODO: support applying features here
+    WasmBinaryBuilder binaryBuilder(wasm, FeatureSet::MVP, data);
     binaryBuilder.read();
     return;
   }