Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow reference types as long as not used by the initialization function? #98

Open
amesgen opened this issue Feb 25, 2024 · 8 comments
Open

Comments

@amesgen
Copy link

amesgen commented Feb 25, 2024

I have a WASM module that uses reference types, but the initialization function (some pure precomputation) doesn't use them. Currently, wizer categorically fails with

Error: reference types support is not enabled (at offset 0x31ef)

However, when I apply this patch

diff --git a/src/lib.rs b/src/lib.rs
index f2ceb94..7f5adc9 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -623,7 +623,7 @@ impl Wizer {
         config.wasm_simd(self.wasm_simd.unwrap_or(DEFAULT_WASM_SIMD));
 
         // Proposals that we should add support for.
-        config.wasm_reference_types(false);
+        config.wasm_reference_types(true);
         config.wasm_threads(false);
 
         Ok(config)
@@ -642,7 +642,7 @@ impl Wizer {
             multi_value: self.wasm_multi_value.unwrap_or(DEFAULT_WASM_MULTI_VALUE),
 
             // Proposals that we should add support for.
-            reference_types: false,
+            reference_types: true,
             simd: self.wasm_simd.unwrap_or(DEFAULT_WASM_SIMD),
             threads: false,
             tail_call: false,

everything works fine, both initialization and later usage of the WASM module (including reference types).

Would it make sense to allow reference types a priori, and instead error when instructions related to reference types are encountered during initialization? I could take a stab at a PR if this makes sense.

@fitzgen
Copy link
Member

fitzgen commented Feb 26, 2024

We do this sort of thing for bulk memory already. We do an extra validation pass over the Wasm and fail if we statically see any table.set instruction or anything like that. We could extend this for reference types as well.

Adding support for doing these checks dynamically is a little more involved. I think we could do this by inserting calls to a new dynamic_fail_wizer_init imported function, or something like that, before each forbidden instruction during the instrumentation phase. That function would always trap, making initialization fail if it was dynamically executed.

@tschneidereit
Copy link
Member

Could we perhaps check if there are any live references at the end of wizening? It seems like everything else wouldn't really matter, right?

@fitzgen
Copy link
Member

fitzgen commented Feb 26, 2024

If we limit this to extenrerfs, I think that works, so long as we also record the initialized sizes of externref tables (which would all be full of null elements if we enforce the proposed property). In fact, I don't think we need to even track the live externrefs since there won't be any: wasm can't create an externref, only the host can, and we simply won't.

So basically, I think allowing support for extenrefs is fine because there will only ever be null externrefs. And we can avoid the table-size issue for now by keeping the existing bulk-memory validation that forbids table.grow (or turn it into a dynamic check as described earlier (or just add support for growing externref tables with null references)).

But funcrefs present more problems:

  • We can't track their liveness like we could for externrefs since we don't have hooks and they are also live for the entire store's lifetime.
  • Given a live funcref we don't have a way to inspect which function it is

See #29 for more ideas about full table support.

TerrorJack added a commit to haskell-wasm/wizer that referenced this issue Mar 4, 2024
TerrorJack added a commit to haskell-wasm/wizer that referenced this issue Mar 14, 2024
TerrorJack added a commit to haskell-wasm/wizer that referenced this issue Apr 16, 2024
TerrorJack added a commit to haskell-wasm/wizer that referenced this issue Aug 21, 2024
TerrorJack added a commit to haskell-wasm/wizer that referenced this issue Sep 21, 2024
@blaine-arcjet
Copy link

blaine-arcjet commented Nov 21, 2024

Rust 1.82 seems to have enabled reference types by default, so Wizer fails on Wasm files produced by a Rust 1.82 compiler.

@tschneidereit
Copy link
Member

Ah, that's unfortunate :( I'm not sure any of the maintainers have the bandwidth to take this on currently, I'm afraid. If you're able to look into it yourself, we'd definitely be very happy to support you in doing so. Otherwise, I don't think we can give a realistic timeline for when this will be fixed.

@cfallin
Copy link
Member

cfallin commented Nov 21, 2024

I briefly talked to @fitzgen about this a few weeks ago; if I recall correctly we agreed that it was fine to enable this alongside the existing checks that there are no table-mutating instructions, because given the lack of such instructions, no refs can be live other than the initial table elements (which wizer preserves); and this should be sufficient for new LLVM usage. I had a quick hack to enable this but unfortunately have wiped and returned the laptop that was on; but IIRC I had updated around here and similarly below that to make the feature-flag a wizer command-line option (and then one would want to add a test with a handwritten module in wat).

@Gentle
Copy link

Gentle commented Jan 19, 2025

#124 I hope this is useful :)

@Gentle
Copy link

Gentle commented Jan 20, 2025

I am not sure why the tests on windows failed or what else is missing but I'd like to help get this merged any way I can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants