Fix wasm-tools component new for page-size-1 memory

[chenyan2002]

• In wasm-tools component new, check if the adapter import memory has the same page size as the core module memory's.
• In GC pass, allow parsing adapters with custom page size.
• Generalize the realloc_via_memory_grow function to work with any custom page size.

[alexcrichton]

Thanks! Personally though this doesn't feel like the right solution to me as it's changing the meaning of the original adapter module. The code here says:

        // The page size override is safe, because the adapter code doesn't
        // allocate memory nor assume alignment.

but while that's specifically true for the current iteration of the adapter wasmtime publishes this is not something guaranteed to be generally true. Originally the adapter very much did use memory.grow and depended on the 64k page size, and so this transformation would be invalid for that iteration of the adapter.

Does this work correctly if both the adapter and the main module use a 1-byte-size page size? If not then that's definitely a bug to fix, but otherwise I'd lean more on "the adapter should be compiled for 1-byte pages" as opposed to retroactively changing the adapter.

[chenyan2002]

Originally the adapter very much did use memory.grow and depended on the 64k page size, and so this transformation would be invalid for that iteration of the adapter.

Is the concern that people would still use this old version of the adapter? We can scan the adapter code, if there is no memory.grow/size instructions, then it's safe to overwrite? At the minimal, I think we should bail out if the adapter and the core module has different page sizes. Currently, component new just silently generates an inconsistent component without any error.

I'd lean more on "the adapter should be compiled for 1-byte pages" as opposed to retroactively changing the adapter.

Yeah, it's certainly more robust this way. But this means we need to publish the page-size-1 adapter. This adds an extra dimension to the "matrix" of adapters. We will have (library | command) * (page-size-1 | regular). Internally, we also have another dimension of whether to shift user memory address space. In the future, if we support more custom page sizes, then we need to publish even more variants of the adapters. This doesn't seem to scale?

If we plan to support more custom page sizes, then making sure that the adapter code doesn't assume the 64k page size is a good property to have? If we maintain this property as we currently are, then the rewriting is safe and easier to maintain.

[alexcrichton]

I'm not so concerned about using older versions of the adapter as placing more restrictions on what it can/can't do. Right now the only real restrictions are what sections it contains, which while a major restriction has still provided a great deal of flexibility in changing the implementation of the adapter over time without having to update this crate as well.

I do agree though that a better first-class error from wit-component, at minimum, would be good to add!

---

For the combinatorial complexity, yeah I can see where this would add to that. In a sense though the adapter and tooling was all developed as a short-term hack to get deleted. The native wasip2/wasip3 targets, for example, no longer depend on the adapter for Rust & C toolchains. In that sense I would personally prefer to either sunset the adapter paradigm entirely or to move development entirely to toolchains which depend on the adapter. Needing a combination of builds is, in my mind, part of the complexity of a toolchain continuing to require an adapter vs updating toolchain support to not require an adapter.

[chenyan2002]

Sounds good. Checking the page size is enough for our use case.

updating toolchain support to not require an adapter

The main use case for the adapter is when targeting wasm32-unknown-unknown, where WASI is not needed. Theoretically, we can use wasm32-wasip2 and GC the WASI part, but if we virtualize some of the WASI interface, I think wasm-component-ld get confused?

[alexcrichton]

I'm a bit wary of probing the module like this because this would handle multi-memory, should a module be using that, relatively poorly. The validation.rs step already has a MainModuleMemory payload which carries a MemoryType, and it's also got imported_memory: Option<MemoryType> for what's being imported for adapters I believe. Could those be used directly instead of duplicate-ly probing here? Ideally for example there's some location where where the main module memory is hooked into the adapter's memory, and that's where the type-check happens.

Also on that type there's a page_size method which means you don't have to read the raw fields directly

[chenyan2002]

Done