Improve errors with pooling-by-default serve command (#10483)

* Improve errors with pooling-by-default `serve` command

This commit is intended to address #10482 where the defaults of the
`wasmtime serve` subcommand produced a confusing and surprising error.
Specifically the `-Wmax-table-elements` option, prior to this change, only
affected the store limiter used and didn't actually affect the pooling
allocator settings. That meant that if a module exceeded the limits of
the pooling allocator it would produce an error message that seemed like
`-Wmax-table-elements` would fix but it wouldn't actually.

Two changes in this commit are meant to address this:

* Errors from the pooling allocator have been update to mention "pooling
  allocator" within them somewhere to surface where the error is coming
  from.
* The `-Wmax-memory-size` and `-Wmax-table-elements` configuration are
  now applied to the pooling allocator automatically if the
  corresponding `-Opooling-*` option isn't passed.

That should mean that the original error should be a bit easier to debug
while the attempted solution will also work.

* Fix test expectations

* Get new test passing on 32-bit

Author: alexcrichton

crates/cli-flags/src/lib.rs

@@ -826,7 +826,9 @@ impl CommonOptions {
                     if let Some(limit) = self.opts.pooling_total_tables {
                         cfg.total_tables(limit);
                     }
-                    if let Some(limit) = self.opts.pooling_table_elements {
+                    if let Some(limit) = self.opts.pooling_table_elements
+                        .or(self.wasm.max_table_elements)
+                    {
                         cfg.table_elements(limit);
                     }
                     if let Some(limit) = self.opts.pooling_max_core_instance_size {
@@ -837,7 +839,9 @@ impl CommonOptions {
                         limit => cfg.total_stacks(limit),
                         _ => err,
                     }
-                    if let Some(max) = self.opts.pooling_max_memory_size {
+                    if let Some(max) = self.opts.pooling_max_memory_size
+                        .or(self.wasm.max_memory_size)
+                    {
                         cfg.max_memory_size(max);
                     }
                     if let Some(size) = self.opts.pooling_decommit_batch_size {

crates/wasmtime/src/runtime/vm/instance/allocator/pooling.rs

@@ -147,19 +147,24 @@ pub struct InstanceLimits {
 
 impl Default for InstanceLimits {
     fn default() -> Self {
+        let total = if cfg!(target_pointer_width = "32") {
+            100
+        } else {
+            1000
+        };
         // See doc comments for `wasmtime::PoolingAllocationConfig` for these
         // default values
         Self {
-            total_component_instances: 1000,
+            total_component_instances: total,
             component_instance_size: 1 << 20, // 1 MiB
-            total_core_instances: 1000,
+            total_core_instances: total,
             max_core_instances_per_component: u32::MAX,
             max_memories_per_component: u32::MAX,
             max_tables_per_component: u32::MAX,
-            total_memories: 1000,
-            total_tables: 1000,
+            total_memories: total,
+            total_tables: total,
             #[cfg(feature = "async")]
-            total_stacks: 1000,
+            total_stacks: total,
             core_instance_size: 1 << 20, // 1 MiB
             max_tables_per_module: 1,
             // NB: in #8504 it was seen that a C# module in debug module can
@@ -169,9 +174,9 @@ impl Default for InstanceLimits {
             #[cfg(target_pointer_width = "64")]
             max_memory_size: 1 << 32, // 4G,
             #[cfg(target_pointer_width = "32")]
-            max_memory_size: usize::MAX,
+            max_memory_size: 10 << 20, // 10 MiB
             #[cfg(feature = "gc")]
-            total_gc_heaps: 1000,
+            total_gc_heaps: total,
         }
     }
 }
@@ -497,7 +502,8 @@ unsafe impl InstanceAllocatorImpl for PoolingInstanceAllocator {
         offsets: &VMComponentOffsets<HostPtr>,
         get_module: &'a dyn Fn(StaticModuleIndex) -> &'a Module,
     ) -> Result<()> {
-        self.validate_component_instance_size(offsets)?;
+        self.validate_component_instance_size(offsets)
+            .context("component instance size does not fit in pooling allocator requirements")?;
 
         let mut num_core_instances = 0;
         let mut num_memories = 0;
@@ -533,23 +539,23 @@ unsafe impl InstanceAllocatorImpl for PoolingInstanceAllocator {
         {
             bail!(
                 "The component transitively contains {num_core_instances} core module instances, \
-                 which exceeds the configured maximum of {}",
+                 which exceeds the configured maximum of {} in the pooling allocator",
                 self.limits.max_core_instances_per_component
             );
         }
 
         if num_memories > usize::try_from(self.limits.max_memories_per_component).unwrap() {
             bail!(
                 "The component transitively contains {num_memories} Wasm linear memories, which \
-                 exceeds the configured maximum of {}",
+                 exceeds the configured maximum of {} in the pooling allocator",
                 self.limits.max_memories_per_component
             );
         }
 
         if num_tables > usize::try_from(self.limits.max_tables_per_component).unwrap() {
             bail!(
                 "The component transitively contains {num_tables} tables, which exceeds the \
-                 configured maximum of {}",
+                 configured maximum of {} in the pooling allocator",
                 self.limits.max_tables_per_component
             );
         }
@@ -558,9 +564,12 @@ unsafe impl InstanceAllocatorImpl for PoolingInstanceAllocator {
     }
 
     fn validate_module_impl(&self, module: &Module, offsets: &VMOffsets<HostPtr>) -> Result<()> {
-        self.validate_memory_plans(module)?;
-        self.validate_table_plans(module)?;
-        self.validate_core_instance_size(offsets)?;
+        self.validate_memory_plans(module)
+            .context("module memory does not fit in pooling allocator requirements")?;
+        self.validate_table_plans(module)
+            .context("module table does not fit in pooling allocator requirements")?;
+        self.validate_core_instance_size(offsets)
+            .context("module instance size does not fit in pooling allocator requirements")?;
         Ok(())
     }
 

tests/all/cli_tests.rs

@@ -2405,3 +2405,33 @@ fn compilation_logs() -> Result<()> {
     }
     Ok(())
 }
+
+#[test]
+fn big_table_in_pooling_allocator() -> Result<()> {
+    // Works by default
+    run_wasmtime(&["tests/all/cli_tests/big_table.wat"])?;
+
+    // Does not work by default in the pooling allocator, and the error message
+    // should mention something about the pooling allocator.
+    let output = run_wasmtime_for_output(
+        &["-Opooling-allocator", "tests/all/cli_tests/big_table.wat"],
+        None,
+    )?;
+    assert!(!output.status.success());
+    println!("{}", String::from_utf8_lossy(&output.stderr));
+    assert!(String::from_utf8_lossy(&output.stderr).contains("pooling allocator"));
+
+    // Does work with `-Wmax-table-elements`
+    run_wasmtime(&[
+        "-Opooling-allocator",
+        "-Wmax-table-elements=25000",
+        "tests/all/cli_tests/big_table.wat",
+    ])?;
+    // Also works with `-Opooling-table-elements`
+    run_wasmtime(&[
+        "-Opooling-allocator",
+        "-Opooling-table-elements=25000",
+        "tests/all/cli_tests/big_table.wat",
+    ])?;
+    Ok(())
+}

tests/all/cli_tests/big_table.wat

@@ -0,0 +1,3 @@
+(module
+  (table 25000 funcref)
+)

tests/all/main.rs

@@ -118,3 +118,17 @@ pub(crate) fn gc_store() -> wasmtime::Result<wasmtime::Store<()>> {
     let engine = wasmtime::Engine::new(&config)?;
     Ok(wasmtime::Store::new(&engine, ()))
 }
+
+trait ErrorExt {
+    fn assert_contains(&self, msg: &str);
+}
+
+impl ErrorExt for anyhow::Error {
+    fn assert_contains(&self, msg: &str) {
+        if self.chain().any(|e| e.to_string().contains(msg)) {
+            return;
+        }
+
+        panic!("failed to find {msg:?} within error message {self:?}")
+    }
+}

tests/all/pooling_allocator.rs

@@ -1,4 +1,4 @@
-use super::skip_pooling_allocator_tests;
+use super::{skip_pooling_allocator_tests, ErrorExt};
 use wasmtime::*;
 
 #[test]
@@ -35,18 +35,17 @@ fn memory_limit() -> Result<()> {
     // Module should fail to instantiate because it has too many memories
     match Module::new(&engine, r#"(module (memory 1) (memory 1))"#) {
         Ok(_) => panic!("module instantiation should fail"),
-        Err(e) => assert_eq!(
-            e.to_string(),
-            "defined memories count of 2 exceeds the per-instance limit of 1",
-        ),
+        Err(e) => {
+            e.assert_contains("defined memories count of 2 exceeds the per-instance limit of 1")
+        }
     }
 
     // Module should fail to instantiate because the minimum is greater than
     // the configured limit
     match Module::new(&engine, r#"(module (memory 4))"#) {
         Ok(_) => panic!("module instantiation should fail"),
-        Err(e) => assert_eq!(
-            e.to_string(),
+        Err(e) =>
+            e.assert_contains(
             "memory index 0 has a minimum byte size of 262144 which exceeds the limit of 0x30000 bytes",
         ),
     }
@@ -244,18 +243,16 @@ fn table_limit() -> Result<()> {
     // Module should fail to instantiate because it has too many tables
     match Module::new(&engine, r#"(module (table 1 funcref) (table 1 funcref))"#) {
         Ok(_) => panic!("module compilation should fail"),
-        Err(e) => assert_eq!(
-            e.to_string(),
-            "defined tables count of 2 exceeds the per-instance limit of 1",
-        ),
+        Err(e) => {
+            e.assert_contains("defined tables count of 2 exceeds the per-instance limit of 1")
+        }
     }
 
     // Module should fail to instantiate because the minimum is greater than
     // the configured limit
     match Module::new(&engine, r#"(module (table 31 funcref))"#) {
         Ok(_) => panic!("module compilation should fail"),
-        Err(e) => assert_eq!(
-            e.to_string(),
+        Err(e) => e.assert_contains(
             "table index 0 has a minimum element size of 31 which exceeds the limit of 10",
         ),
     }
@@ -634,26 +631,14 @@ fn instance_too_large() -> Result<()> {
     config.allocation_strategy(pool);
 
     let engine = Engine::new(&config)?;
-    let expected = if cfg!(feature = "wmemcheck") {
-        "\
-        instance allocation for this module requires 336 bytes which exceeds the \
-configured maximum of 16 bytes; breakdown of allocation requirement:
-
- * 76.19% - 256 bytes - instance state management
- * 21.43% - 72 bytes - static vmctx data
-"
-    } else {
-        "\
-instance allocation for this module requires 240 bytes which exceeds the \
-configured maximum of 16 bytes; breakdown of allocation requirement:
-
- * 66.67% - 160 bytes - instance state management
- * 30.00% - 72 bytes - static vmctx data
-"
-    };
     match Module::new(&engine, "(module)") {
         Ok(_) => panic!("should have failed to compile"),
-        Err(e) => assert_eq!(e.to_string(), expected),
+        Err(e) => {
+            e.assert_contains("exceeds the configured maximum of 16 bytes");
+            e.assert_contains("breakdown of allocation requirement");
+            e.assert_contains("instance state management");
+            e.assert_contains("static vmctx data");
+        }
     }
 
     let mut lots_of_globals = format!("(module");
@@ -662,26 +647,14 @@ configured maximum of 16 bytes; breakdown of allocation requirement:
     }
     lots_of_globals.push_str(")");
 
-    let expected = if cfg!(feature = "wmemcheck") {
-        "\
-instance allocation for this module requires 1936 bytes which exceeds the \
-configured maximum of 16 bytes; breakdown of allocation requirement:
-
- * 13.22% - 256 bytes - instance state management
- * 82.64% - 1600 bytes - defined globals
-"
-    } else {
-        "\
-instance allocation for this module requires 1840 bytes which exceeds the \
-configured maximum of 16 bytes; breakdown of allocation requirement:
-
- * 8.70% - 160 bytes - instance state management
- * 86.96% - 1600 bytes - defined globals
-"
-    };
     match Module::new(&engine, &lots_of_globals) {
         Ok(_) => panic!("should have failed to compile"),
-        Err(e) => assert_eq!(e.to_string(), expected),
+        Err(e) => {
+            e.assert_contains("exceeds the configured maximum of 16 bytes");
+            e.assert_contains("breakdown of allocation requirement");
+            e.assert_contains("defined globals");
+            e.assert_contains("instance state management");
+        }
     }
 
     Ok(())
@@ -879,10 +852,9 @@ fn component_instance_size_limit() -> Result<()> {
 
     match wasmtime::component::Component::new(&engine, "(component)") {
         Ok(_) => panic!("should have hit limit"),
-        Err(e) => assert_eq!(
-            e.to_string(),
-            "instance allocation for this component requires 48 bytes of `VMComponentContext` space \
-             which exceeds the configured maximum of 1 bytes"
+        Err(e) => e.assert_contains(
+            "instance allocation for this component requires 48 bytes of \
+            `VMComponentContext` space which exceeds the configured maximum of 1 bytes",
         ),
     }
 
@@ -1047,10 +1019,9 @@ fn component_core_instances_limit() -> Result<()> {
         "#,
     ) {
         Ok(_) => panic!("should have hit limit"),
-        Err(e) => assert_eq!(
-            e.to_string(),
+        Err(e) => e.assert_contains(
             "The component transitively contains 2 core module instances, which exceeds the \
-             configured maximum of 1"
+             configured maximum of 1",
         ),
     }
 
@@ -1090,10 +1061,9 @@ fn component_memories_limit() -> Result<()> {
         "#,
     ) {
         Ok(_) => panic!("should have hit limit"),
-        Err(e) => assert_eq!(
-            e.to_string(),
+        Err(e) => e.assert_contains(
             "The component transitively contains 2 Wasm linear memories, which exceeds the \
-             configured maximum of 1"
+             configured maximum of 1",
         ),
     }
 
@@ -1133,10 +1103,9 @@ fn component_tables_limit() -> Result<()> {
         "#,
     ) {
         Ok(_) => panic!("should have hit limit"),
-        Err(e) => assert_eq!(
-            e.to_string(),
+        Err(e) => e.assert_contains(
             "The component transitively contains 2 tables, which exceeds the \
-             configured maximum of 1"
+             configured maximum of 1",
         ),
     }
 
@@ -1281,13 +1250,9 @@ fn shared_memory_unsupported() -> Result<()> {
         "#,
     )
     .unwrap_err();
-    let err = err.to_string();
-    assert!(
-        err.contains(
-            "memory index 0 is shared which is not supported \
-             in the pooling allocator"
-        ),
-        "bad error: {err}"
+    err.assert_contains(
+        "memory index 0 is shared which is not supported \
+         in the pooling allocator",
     );
     Ok(())
 }