Repository: bytecodealliance/wasm-tools
Pull request #1668 (2024-07-12 14:45:39)

Refactor handling of `--features` on the CLI
This commit uses the `bitflags::Flags` trait to refactor the parsing of the `--features` CLI flag to avoid needing to manually list out wasm features. This is something I at least always personally forget during reviews and such so it should make the CLI support more robust.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 2d39c9f5..7edaef94 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1699,6 +1699,7 @@ dependencies = [
  "addr2line",
  "anyhow",
  "arbitrary",
+ "bitflags",
  "clap",
  "clap_complete",
  "cpp_demangle",
diff --git a/Cargo.toml b/Cargo.toml
index 1ac79b10..5a84f926 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -115,6 +115,7 @@ termcolor = { workspace = true }
 # Dependencies of `validate`
 wasmparser = { workspace = true, optional = true, features = ['validate'] }
 rayon = { workspace = true, optional = true }
+bitflags = { workspace = true, optional = true }
 
 # Dependencies of `print`
 wasmprinter = { workspace = true }
@@ -201,7 +202,7 @@ default = [
 ]
 
 # Each subcommand is gated behind a feature and lists the dependencies it needs
-validate = ['dep:wasmparser', 'rayon', 'dep:addr2line', 'dep:gimli']
+validate = ['dep:wasmparser', 'rayon', 'dep:addr2line', 'dep:gimli', 'dep:bitflags']
 print = []
 parse = []
 smith = ['wasm-smith', 'arbitrary', 'dep:serde', 'dep:serde_derive', 'dep:serde_json']
diff --git a/src/addr2line.rs b/src/addr2line.rs
index f81986ee..1be023c5 100644
--- a/src/addr2line.rs
+++ b/src/addr2line.rs
@@ -107,7 +107,7 @@ impl<'a> Addr2lineModules<'a> {
                 Some(start) => addr
                     .checked_sub(start)
                     .context("address is before the beginning of the text section")?,
-                None => bail!("no code section found in module"),
+                None => return Ok(None),
             }
         };
 
diff --git a/src/bin/wasm-tools/validate.rs b/src/bin/wasm-tools/validate.rs
index fe402b13..b94596b5 100644
--- a/src/bin/wasm-tools/validate.rs
+++ b/src/bin/wasm-tools/validate.rs
@@ -1,5 +1,6 @@
 use addr2line::LookupResult;
 use anyhow::{anyhow, Context, Result};
+use bitflags::Flags;
 use rayon::prelude::*;
 use std::fmt::Write;
 use std::mem;
@@ -171,38 +172,9 @@ impl Opts {
 fn parse_features(arg: &str) -> Result<WasmFeatures> {
     let mut ret = WasmFeatures::default();
 
-    const FEATURES: &[(&str, WasmFeatures)] = &[
-        ("reference-types", WasmFeatures::REFERENCE_TYPES),
-        ("function-references", WasmFeatures::FUNCTION_REFERENCES),
-        ("simd", WasmFeatures::SIMD),
-        ("threads", WasmFeatures::THREADS),
-        (
-            "shared-everything-threads",
-            WasmFeatures::SHARED_EVERYTHING_THREADS,
-        ),
-        ("bulk-memory", WasmFeatures::BULK_MEMORY),
-        ("multi-value", WasmFeatures::MULTI_VALUE),
-        ("tail-call", WasmFeatures::TAIL_CALL),
-        ("component-model", WasmFeatures::COMPONENT_MODEL),
-        (
-            "component-model-values",
-            WasmFeatures::COMPONENT_MODEL_VALUES,
-        ),
-        ("multi-memory", WasmFeatures::MULTI_MEMORY),
-        ("exception-handling", WasmFeatures::EXCEPTIONS),
-        ("memory64", WasmFeatures::MEMORY64),
-        ("extended-const", WasmFeatures::EXTENDED_CONST),
-        ("floats", WasmFeatures::FLOATS),
-        (
-            "saturating-float-to-int",
-            WasmFeatures::SATURATING_FLOAT_TO_INT,
-        ),
-        ("sign-extension", WasmFeatures::SIGN_EXTENSION),
-        ("mutable-global", WasmFeatures::MUTABLE_GLOBAL),
-        ("relaxed-simd", WasmFeatures::RELAXED_SIMD),
-        ("gc", WasmFeatures::GC),
-        ("legacy-exceptions", WasmFeatures::LEGACY_EXCEPTIONS),
-    ];
+    fn flag_name(flag: &bitflags::Flag<WasmFeatures>) -> String {
+        flag.name().to_lowercase().replace('_', "-")
+    }
 
     for part in arg.split(',').map(|s| s.trim()).filter(|s| !s.is_empty()) {
         let (enable, part) = if let Some(part) = part.strip_prefix("-") {
@@ -212,28 +184,27 @@ fn parse_features(arg: &str) -> Result<WasmFeatures> {
         };
         match part {
             "all" => {
-                for (name, feature) in FEATURES {
-                    // don't count this under "all" for now.
-                    if *name == "deterministic" {
-                        continue;
-                    }
-                    ret.set(*feature, enable);
+                for flag in WasmFeatures::FLAGS.iter() {
+                    ret.set(*flag.value(), enable);
                 }
             }
 
             name => {
-                let (_, feature) = FEATURES.iter().find(|(n, _)| *n == name).ok_or_else(|| {
-                    anyhow!(
-                        "unknown feature `{}`\nValid features: {}",
-                        name,
-                        FEATURES
-                            .iter()
-                            .map(|(name, _)| *name)
-                            .collect::<Vec<_>>()
-                            .join(", "),
-                    )
-                })?;
-                ret.set(*feature, enable);
+                let flag = WasmFeatures::FLAGS
+                    .iter()
+                    .find(|f| flag_name(f) == name)
+                    .ok_or_else(|| {
+                        anyhow!(
+                            "unknown feature `{}`\nValid features: {}",
+                            name,
+                            WasmFeatures::FLAGS
+                                .iter()
+                                .map(flag_name)
+                                .collect::<Vec<_>>()
+                                .join(", "),
+                        )
+                    })?;
+                ret.set(*flag.value(), enable);
             }
         }
     }


Test patch:
diff --git a/tests/cli/validate-features.wat b/tests/cli/validate-features.wat
new file mode 100644
index 00000000..3aad684f
--- /dev/null
+++ b/tests/cli/validate-features.wat
@@ -0,0 +1,5 @@
+;; FAIL: validate --features=-all %
+
+(module
+  (import "x" "y" (global v128))
+)
diff --git a/tests/cli/validate-features.wat.stderr b/tests/cli/validate-features.wat.stderr
new file mode 100644
index 00000000..45ead379
--- /dev/null
+++ b/tests/cli/validate-features.wat.stderr
@@ -0,0 +1,1 @@
+error: SIMD support is not enabled (at offset 0xb)
diff --git a/tests/cli/validate-features2.wat b/tests/cli/validate-features2.wat
new file mode 100644
index 00000000..4bf1c135
--- /dev/null
+++ b/tests/cli/validate-features2.wat
@@ -0,0 +1,6 @@
+;; RUN: validate --features=-all,simd %
+
+(module
+  (import "x" "y" (global v128))
+)
+
diff --git a/tests/cli/validate-unknown-features.wat b/tests/cli/validate-unknown-features.wat
new file mode 100644
index 00000000..daebd5c6
--- /dev/null
+++ b/tests/cli/validate-unknown-features.wat
@@ -0,0 +1,4 @@
+;; FAIL: validate --features=unknown %
+
+(module)
+
diff --git a/tests/cli/validate-unknown-features.wat.stderr b/tests/cli/validate-unknown-features.wat.stderr
new file mode 100644
index 00000000..3a72311f
--- /dev/null
+++ b/tests/cli/validate-unknown-features.wat.stderr
@@ -0,0 +1,4 @@
+error: invalid value 'unknown' for '--features <FEATURES>': unknown feature `unknown`
+Valid features: mutable-global, saturating-float-to-int, sign-extension, reference-types, multi-value, bulk-memory, simd, relaxed-simd, threads, shared-everything-threads, tail-call, floats, multi-memory, exceptions, memory64, extended-const, component-model, function-references, memory-control, gc, custom-page-sizes, component-model-values, component-model-nested-names, component-model-more-flags, legacy-exceptions
+
+For more information, try '--help'.