Wasmi `v0.32.0-beta.13` seems to have broken linking

[tomaka]

I'm trying to update from beta 12 to beta 13 here: https://github.com/smol-dot/smoldot/pull/1817

However, after this PR, wasmi returns linking errors when instantiating something. Example error: Instantiation("cannot find definition for import env::ext_offchain_index_set_version_1 with type Func(FuncType { params: [I64, I64], results: [] })")

The way smoldot works is:

• It gathers the list of all imported functions using module.imports().
• For each function, it calls wasmi::Func::new and adds the function the linker.
• Then it calls linker.instantiate.

I have checked the function signatures and module names and they all look correct. In particular, we indeed add to the linker the functions that wasmi fails to instantiate.

My random guess as what might be causing trouble is the fact that I'm using the generic function constructor: https://github.com/smol-dot/smoldot/blob/61cf6a956907f384e8ed3225a4145ff47b8bb26b/lib/src/executor/vm/interpreter.rs#L132-L135

[Robbepop]

@tomaka thanks for reporting the bug, I am going to investigate it with high priority.

My random guess as what might be causing trouble is the fact that I'm using the generic function constructor:

Is there a specific reason as to why you are not using Linker::func_new or even better Linker::func_wrap?

Does using Linker::func_new or Linker::func_wrap solve your issues?

[tomaka]

you are not using Linker::func_new

I have no idea if one way to do things is better than the other 🤷

or even better Linker::func_wrap?

I need to be able to accept unresolved imports. If a function name is not recognized by smoldot, the VM should still be able run, and errors should happen only if the function is actually called. To achieve that, I can't know the function signature at compilation time. And I use the same code for both recognized and unrecognized functions by convenience.

Does using Linker::func_new or Linker::func_wrap solve your issues?

func_new doesn't solve it, no.

[Robbepop]

Okay, very weird question but does enabling the new no-hash-maps crate feature of Wasmi solve your issues?

[tomaka]

does enabling the new no-hash-maps crate feature of Wasmi solve your issues?

It doesn't compile:

error[E0432]: unresolved imports `std::cmp`, `std::mem`, `std::ops`
 --> /Users/tomaka/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasmi_collections-0.1.0/src/string_interner/detail.rs:4:5
  |
4 |     cmp::Ordering,
  |     ^^^ could not find `cmp` in `std`
5 |     collections::{btree_map::Entry, BTreeMap},
6 |     mem,
  |     ^^^ no `mem` in the root
7 |     ops::Deref,
  |     ^^^ could not find `ops` in `std`
  |
  = help: consider importing one of these items instead:
          core::mem
          std::mem

For more information about this error, try `rustc --explain E0432`.
error: could not compile `wasmi_collections` (lib) due to 1 previous error

[tomaka]

If I activate both std and no-hash-maps, the issue is fixed.

[Robbepop]

https://github.com/smol-dot/smoldot/pull/1817 udpates from beta.10 directly to beta.13. Can you tell me which was the last working beta.N?

I built smoldot locally and can confirm that Wasmi v0.32.0-beta.12 still worked and the problems started with Wasmi v0.32.0-beta.13.

[Robbepop]

@tomaka

I took one of the failing smoldot tests and re-created a regression test in the Wasmi repository, however, I somehow cannot reproduce it there. Can you please take a look and maybe tell me what smoldot is doing differently?

#[test]
fn smoldot_regression() {
    use crate::{Engine, Func, Linker, Memory, Module, Store};
    let wasm = wat::parse_str(
        r#"
        (module
            (import "host" "hello" (func $host_hello (param i32) (result i32)))
            (import "env" "memory" (memory $mem 0 4096))
            (func (export "hello") (result i32)
                (call $host_hello (i32.const 3))
                (i32.const 2)
                i32.add
            )
        )"#,
    )
    .unwrap();
    let engine = Engine::default();
    let mut store = Store::new(&engine, ());
    let module = Module::new(&engine, &wasm[..]).unwrap();
    let mut linker = <Linker<()>>::new(&engine);
    for import_type in module.imports() {
        let name = import_type.import_name();
        match import_type.ty() {
            ExternType::Memory(memory_type) => {
                let memory = Memory::new(&mut store, memory_type.clone()).unwrap();
                linker.define(name.module(), name.name(), memory).unwrap();
            }
            ExternType::Func(func_type) => {
                let func = Func::new(
                    &mut store,
                    func_type.clone(),
                    |_caller, _params, _results| unimplemented!(),
                );
                linker.define(name.module(), name.name(), func).unwrap();
            }
            ExternType::Global(_) => unimplemented!(),
            ExternType::Table(_) => unimplemented!(),
        }
    }
    // In smoldot this call fails ...
    linker.instantiate(&mut store, &module).unwrap();
}

[Robbepop]

I debugged it some more in smoldot:

This call fails: https://github.com/smol-dot/smoldot/blob/61cf6a956907f384e8ed3225a4145ff47b8bb26b/lib/src/executor/vm/interpreter.rs#L178

Because wasmi::Linker::get("host", "hello") at this point returns None when not enabling wasmi/no-hash-maps crate feature. I have not yet figured out why. My base assumption is that this is connected to the wasmi_collections::StringInterner which returns consecutive indices for Symbol in no-hash-maps mode and non-consecutive Symbol indices otherwise. Unfortunately I am unable to reproduce this at Wasmi locally ...

[Robbepop]

I figured out that I cannot reproduce this directly inside the Wasmi crate test suite since the Wasmi test suite does not compile under no_std but the error in smoldot only happens in Wasmi's no_std mode without the no-hash-maps feature enable. Thus enabling either wasmi/std, wasmi/no-hash-maps or both will fix the issue on the smoldot side.

@tomaka smoldot does not propagate its std feature for its wasmi dependency. Is this an oversight or wanted behavior?

[tomaka]

smoldot does not propagate its std feature

Smoldot is entirely no-std. Smoldot's std feature is completely disconnected from the VM.

enabling either wasmi/std, wasmi/no-hash-maps

I'm happy to enable no-hash-maps if it compiles.

[Robbepop]

@tomaka I opened this PR to fix no_std+no-hash-maps builds which should help you until the underlying issue is properly fixed.

If you can confirm me that the PR fixes your issue for now I will create another beta release today and work on the proper fix in the meantime.

[Robbepop]

This minimized Wasmi test case fails only if both std and no-hash-maps crate features are disabled:

#[test]
fn min_smoldot_regression() {
    let mut interner = crate::collections::StringInterner::new();
    let sym = interner.get_or_intern("hello");
    assert_eq!(interner.get("hello"), Some(sym));
}

Reason being that in the aforementioned mode make_hash calls in StringInterner::{get, get_or_intern} return different hashes for the same string which is obviously incorrect. As of now I have no clue why. On std mode everything seems to work. So my best guess is that it has something to do with how wasmi_collections use ahash in no_std mode.

[Robbepop]

@tomaka I released Wasmi version 0.32.0-beta.14 with the proper fix included.