Repository: bytecodealliance/wasmtime
Pull request #8544 (2024-05-03 18:47:33)

Fix combining `cache_call_indirects` and memory64
This commit fixes a fuzz bug that popped up where the `cache_call_indirects` feature wasn't reading memory64-based offsets correctly. This is due to (not great) API design in `wasmparser` (introduced by me) where wasmparser by default won't read 64-bit offsets unless explicitly allowed to. This is to handle how spec tests assert that overlong 32-bit encodings are invalid as the memory64 proposal isn't merged into the spec yet.

The fix here is to call `allow_memarg64` with whether memory64 is enabled or not and then that'll enable reading these overlong and/or larger offsets correctly.

<!--
Please make sure you include the following information:

- If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

- Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->



Patch:
diff --git a/crates/cli-flags/src/lib.rs b/crates/cli-flags/src/lib.rs
index 320b76e6c..985993e4c 100644
--- a/crates/cli-flags/src/lib.rs
+++ b/crates/cli-flags/src/lib.rs
@@ -101,6 +101,17 @@ wasmtime_option_group! {
         /// The maximum number of WebAssembly stacks which can be created with
         /// the pooling allocator.
         pub pooling_total_stacks: Option<u32>,
+
+        /// Whether to enable call-indirect caching.
+        pub cache_call_indirects: Option<bool>,
+
+        /// The maximum call-indirect cache slot count.
+        ///
+        /// One slot is allocated per indirect callsite; if the module
+        /// has more indirect callsites than this limit, then the
+        /// first callsites in linear order in the code section, up to
+        /// the limit, will receive a cache slot.
+        pub max_call_indirect_cache_slots: Option<usize>,
     }
 
     enum Optimize {
@@ -244,15 +255,6 @@ wasmtime_option_group! {
         pub function_references: Option<bool>,
         /// Configure support for the GC proposal.
         pub gc: Option<bool>,
-        /// Whether to enable call-indirect caching.
-        pub cache_call_indirects: Option<bool>,
-        /// The maximum call-indirect cache slot count.
-        ///
-        /// One slot is allocated per indirect callsite; if the module
-        /// has more indirect callsites than this limit, then the
-        /// first callsites in linear order in the code section, up to
-        /// the limit, will receive a cache slot.
-        pub max_call_indirect_cache_slots: Option<usize>,
     }
 
     enum Wasm {
@@ -553,10 +555,10 @@ impl CommonOptions {
         if let Some(enable) = self.opts.memory_init_cow {
             config.memory_init_cow(enable);
         }
-        if let Some(enable) = self.wasm.cache_call_indirects {
+        if let Some(enable) = self.opts.cache_call_indirects {
             config.cache_call_indirects(enable);
         }
-        if let Some(max) = self.wasm.max_call_indirect_cache_slots {
+        if let Some(max) = self.opts.max_call_indirect_cache_slots {
             config.max_call_indirect_cache_slots(max);
         }
 
diff --git a/crates/environ/src/compile/module_environ.rs b/crates/environ/src/compile/module_environ.rs
index d8f8368f4..3bf194489 100644
--- a/crates/environ/src/compile/module_environ.rs
+++ b/crates/environ/src/compile/module_environ.rs
@@ -16,7 +16,6 @@ use std::collections::HashMap;
 use std::mem;
 use std::path::PathBuf;
 use std::sync::Arc;
-use wasmparser::OperatorsReader;
 use wasmparser::{
     types::Types, CustomSectionReader, DataKind, ElementItems, ElementKind, Encoding, ExternalKind,
     FuncToValidate, FunctionBody, NameSectionReader, Naming, Operator, Parser, Payload, TypeRef,
@@ -543,6 +542,7 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> {
 
             Payload::CodeSectionEntry(mut body) => {
                 let validator = self.validator.code_section_entry(&body)?;
+                body.allow_memarg64(self.validator.features().contains(WasmFeatures::MEMORY64));
                 let func_index =
                     self.result.code_index + self.result.module.num_imported_funcs as u32;
                 let func_index = FuncIndex::from_u32(func_index);
@@ -567,7 +567,7 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> {
                             params: sig.params().into(),
                         });
                 }
-                self.prescan_code_section(body.get_operators_reader()?)?;
+                self.prescan_code_section(&body)?;
                 body.allow_memarg64(self.validator.features().contains(WasmFeatures::MEMORY64));
                 self.result.function_body_inputs.push(FunctionBodyData {
                     validator,
@@ -721,9 +721,9 @@ and for re-adding support for interface types you can see this issue:
     ///   index in this count for each function, so we can generate
     ///   its code (with accesses to its own `call_indirect` callsite
     ///   caches) in parallel.
-    fn prescan_code_section(&mut self, reader: OperatorsReader<'data>) -> Result<()> {
+    fn prescan_code_section(&mut self, body: &FunctionBody<'_>) -> Result<()> {
         if self.tunables.cache_call_indirects {
-            for op in reader {
+            for op in body.get_operators_reader()? {
                 let op = op?;
                 match op {
                     // Check whether a table may be mutated by any
diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs
index e063c2650..c7cf11958 100644
--- a/crates/wasmtime/src/config.rs
+++ b/crates/wasmtime/src/config.rs
@@ -1708,6 +1708,24 @@ impl Config {
         self
     }
 
+    pub(crate) fn conditionally_enable_defaults(&mut self) {
+        // If tail calls were not explicitly enabled/disabled (i.e. tail_callable is None), enable
+        // them if we are targeting a backend that supports them. Currently the Cranelift
+        // compilation strategy is the only one that supports tail calls, but not targeting s390x.
+        if self.tunables.tail_callable.is_none() {
+            #[cfg(feature = "cranelift")]
+            let default_tail_calls = self.compiler_config.strategy == Strategy::Cranelift
+                && self.compiler_config.target.as_ref().map_or_else(
+                    || target_lexicon::Triple::host().architecture,
+                    |triple| triple.architecture,
+                ) != Architecture::S390x;
+            #[cfg(not(feature = "cranelift"))]
+            let default_tail_calls = false;
+
+            self.wasm_tail_call(default_tail_calls);
+        }
+    }
+
     pub(crate) fn validate(&self) -> Result<Tunables> {
         if self.features.contains(WasmFeatures::REFERENCE_TYPES)
             && !self.features.contains(WasmFeatures::BULK_MEMORY)
diff --git a/crates/wasmtime/src/engine.rs b/crates/wasmtime/src/engine.rs
index 540aa35c5..9b0453cf9 100644
--- a/crates/wasmtime/src/engine.rs
+++ b/crates/wasmtime/src/engine.rs
@@ -101,7 +101,12 @@ impl Engine {
             crate::runtime::vm::debug_builtins::ensure_exported();
         }
 
-        let config = config.clone();
+        let config = {
+            let mut config = config.clone();
+            config.conditionally_enable_defaults();
+            config
+        };
+
         let tunables = config.validate()?;
 
         #[cfg(any(feature = "cranelift", feature = "winch"))]


Test patch:
diff --git a/tests/all/defaults.rs b/tests/all/defaults.rs
new file mode 100644
index 000000000..0e869e567
--- /dev/null
+++ b/tests/all/defaults.rs
@@ -0,0 +1,62 @@
+use wasmtime::*;
+
+#[test]
+fn test_tail_call_default() -> Result<()> {
+    for (expected, cfg) in [
+        (
+            true,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("x86_64")?,
+        ),
+        (
+            true,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("aarch64")?,
+        ),
+        (
+            true,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("riscv64")?,
+        ),
+        (
+            false,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .target("s390x")?,
+        ),
+        (
+            false,
+            Config::new().strategy(Strategy::Winch).target("x86_64")?,
+        ),
+        (
+            false,
+            Config::new().strategy(Strategy::Winch).target("aarch64")?,
+        ),
+        (
+            false,
+            Config::new()
+                .strategy(Strategy::Cranelift)
+                .wasm_tail_call(false)
+                .target("x86_64")?,
+        ),
+    ] {
+        let engine = Engine::new(cfg)?;
+
+        let wat = r#"
+            (module $from_name_section
+                (func (export "run") (return_call 0))
+            )
+        "#;
+
+        let result = engine.precompile_module(wat.as_bytes()).map(|_| ());
+
+        eprintln!("for config {cfg:?}, got: {result:?}");
+
+        assert_eq!(expected, result.is_ok());
+    }
+
+    Ok(())
+}
diff --git a/tests/all/main.rs b/tests/all/main.rs
index f778edfde..62aaf46ce 100644
--- a/tests/all/main.rs
+++ b/tests/all/main.rs
@@ -7,6 +7,7 @@ mod code_too_large;
 mod component_model;
 mod coredump;
 mod debug;
+mod defaults;
 mod epoch_interruption;
 mod externals;
 mod fuel;
diff --git a/tests/all/module.rs b/tests/all/module.rs
index 05a74df8f..153be7b34 100644
--- a/tests/all/module.rs
+++ b/tests/all/module.rs
@@ -250,3 +250,22 @@ fn compile_a_component() -> Result<()> {
     );
     Ok(())
 }
+
+#[test]
+fn call_indirect_caching_and_memory64() -> Result<()> {
+    let mut config = Config::new();
+    config.wasm_memory64(true);
+    config.cache_call_indirects(true);
+    let engine = Engine::new(&config)?;
+    Module::new(
+        &engine,
+        "(module
+            (memory i64 1)
+            (func (param i64) (result i32)
+                local.get 0
+                i32.load offset=0x100000000
+            )
+        )",
+    )?;
+    Ok(())
+}
diff --git a/tests/disas/indirect-call-caching-exclude-0-index.wat b/tests/disas/indirect-call-caching-exclude-0-index.wat
index 6744cf7b8..93b31f59b 100644
--- a/tests/disas/indirect-call-caching-exclude-0-index.wat
+++ b/tests/disas/indirect-call-caching-exclude-0-index.wat
@@ -1,5 +1,5 @@
 ;;! target = "x86_64"
-;;! flags = [ "-Wcache-call-indirects=y" ]
+;;! flags = [ "-Ocache-call-indirects=y" ]
 
 ;; This test checks that we do *not* get the indirect-call caching optimization
 ;; when it must not be used: in this case, because there is a non-null element
diff --git a/tests/disas/indirect-call-caching-exclude-table-export.wat b/tests/disas/indirect-call-caching-exclude-table-export.wat
index 30ca6f0d5..e9434c14c 100644
--- a/tests/disas/indirect-call-caching-exclude-table-export.wat
+++ b/tests/disas/indirect-call-caching-exclude-table-export.wat
@@ -1,5 +1,5 @@
 ;;! target = "x86_64"
-;;! flags = [ "-Wcache-call-indirects=y" ]
+;;! flags = [ "-Ocache-call-indirects=y" ]
 
 ;; This test checks that we do *not* get the indirect-call caching optimization
 ;; when it must not be used: in this case, because the table is exported so
diff --git a/tests/disas/indirect-call-caching-exclude-table-writes.wat b/tests/disas/indirect-call-caching-exclude-table-writes.wat
index d167a16ee..a05a28888 100644
--- a/tests/disas/indirect-call-caching-exclude-table-writes.wat
+++ b/tests/disas/indirect-call-caching-exclude-table-writes.wat
@@ -1,5 +1,5 @@
 ;;! target = "x86_64"
-;;! flags = [ "-Wcache-call-indirects=y" ]
+;;! flags = [ "-Ocache-call-indirects=y" ]
 
 ;; This test checks that we do *not* get the indirect-call caching optimization
 ;; when it must not be used: in this case, because the table is updated with a
diff --git a/tests/disas/indirect-call-caching-slot-limit-1.wat b/tests/disas/indirect-call-caching-slot-limit-1.wat
index 937c0f9fe..afd08e507 100644
--- a/tests/disas/indirect-call-caching-slot-limit-1.wat
+++ b/tests/disas/indirect-call-caching-slot-limit-1.wat
@@ -1,5 +1,5 @@
 ;;! target = "x86_64"
-;;! flags = [ "-Wcache-call-indirects=y", "-Wmax-call-indirect-cache-slots=2" ]
+;;! flags = [ "-Ocache-call-indirects=y", "-Omax-call-indirect-cache-slots=2" ]
 
 ;; This test checks that we properly bound the number of call-indirect
 ;; cache slots. The first case (here) is when the limit falls in the
diff --git a/tests/disas/indirect-call-caching-slot-limit-2.wat b/tests/disas/indirect-call-caching-slot-limit-2.wat
index a8b89451c..4b62ff0f9 100644
--- a/tests/disas/indirect-call-caching-slot-limit-2.wat
+++ b/tests/disas/indirect-call-caching-slot-limit-2.wat
@@ -1,5 +1,5 @@
 ;;! target = "x86_64"
-;;! flags = [ "-Wcache-call-indirects=y", "-Wmax-call-indirect-cache-slots=2" ]
+;;! flags = [ "-Ocache-call-indirects=y", "-Omax-call-indirect-cache-slots=2" ]
 
 ;; This test checks that we properly bound the number of call-indirect
 ;; cache slots. The second case (here) is when the limit falls
diff --git a/tests/disas/indirect-call-caching.wat b/tests/disas/indirect-call-caching.wat
index 5ca21a148..0e19aa79a 100644
--- a/tests/disas/indirect-call-caching.wat
+++ b/tests/disas/indirect-call-caching.wat
@@ -1,5 +1,5 @@
 ;;! target = "x86_64"
-;;! flags = [ "-Wcache-call-indirects=y" ]
+;;! flags = [ "-Ocache-call-indirects=y" ]
 
 ;; This test checks that we get the indirect-call caching optimization
 ;; where it should be applicable (immutable table, null 0-index).