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

add option for reference-types #124

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Gentle
Copy link

@Gentle Gentle commented Jan 19, 2025

only enables the wasmtime feature, which allows initializing Modules that were compiled with newer llvm versions but don't actually use reference-types

only enables the wasmtime feature, which allows initializing Modules
that were compiled with newer llvm versions but don't actually use
reference_types

Disabled by default
@Gentle
Copy link
Author

Gentle commented Jan 19, 2025

I tried with my own code and could successfully initialize a Module that errors without the option.

Since there is nothing actually implemented, I set the default to false and keep the feature disabled for fuzzers, I hope that is correct

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you extend the wasm_validate function to disallow the following reference-types instructions:

  • ref.null
  • ref.is_null
  • the typed variant of select
  • ref.func
  • table.get
  • table.set
  • table.size
  • table.grow
  • table.fill
  • table.init
  • table.copy

?

And also please add the following tests:

  • a table-modifying instruction is used when reference-types are enabled, but the wasm is still rejected
  • reference-types are enabled and an instruction encoding that requires reference-types is used for an instruction which doesn't actually mutate tables or manipulate references, and therefore wizer succeeds (for example, a call_indirect using a non-zero table index)

Thanks!

src/lib.rs Outdated Show resolved Hide resolved
Gentle and others added 2 commits January 24, 2025 20:44
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
@Gentle Gentle force-pushed the enable_reference_types branch from b1921f0 to 5fca98b Compare January 24, 2025 21:39
@Gentle
Copy link
Author

Gentle commented Jan 25, 2025

I hope I added everything

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Two minor things and then we can merge this.

src/lib.rs Outdated Show resolved Hide resolved
tests/tests.rs Outdated Show resolved Hide resolved
@Gentle
Copy link
Author

Gentle commented Jan 28, 2025

I hope I did it right, this seems to use the new call_indirect and the second table

@Gentle
Copy link
Author

Gentle commented Jan 28, 2025

as expected the test now goes red if I disable reference-types, so it uses the right call now :)

Edit: actually that statement is not neccessarily true, with reference types disabled it already rejects the existence of a second table, but still, the fact that call_indirect finds the second table means it should be the new call_indirect

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants