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

Specify "shared" attribute of TableType to enable upgrade of wasm encoder to 0.213 #280

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

burakemir
Copy link
Contributor

@burakemir burakemir commented Nov 8, 2024

wasm-encoder has a struct TableType which got a new attribute "shared".

The context is a proposal to share data among threads.

We set the attribute to false which means that the table is not to be shared.
This is the defensive setting and seems closest to status quo / ignoring the proposal.

In other words, this change only makes walrus compile with more recent versions and is not an attempt to do anything smart about the shared-everything-threads proposal.

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

In the wasmparser side we should add a read of this property and throw if it is found if we are not handling this state.

Alternatively we should include it in the representation.

@burakemir
Copy link
Contributor Author

Please help me understand better what you mean.

wasmparser already has validation, in the following sense: shared=true is only ever permitted if the "shared everything threads" proposal is enabled: https://github.com/burakemir/wasm-tools/blob/3e0dda0a4736848e48b699d064e2173375eb6560/crates/wasmparser/src/validator/core.rs#L771

So I am pretty confident that shared=false is a good, safe value, not only to get walrus to compile, but also if walrus-generated WASM is used in places where shared=true may be encountered.

The shared everything proposal does change a lot of things, and supporting WASM transformations in a shared everything world probably benefits from a careful look at, well, everything, and potentially some redesign.

No matter what happens with respect to supporting shared-everything, a small change like this PR seems necessary just to catch up. I have another PR that adds all the new atomic instructions (as unsupported operations, also just to get things to build again).

@burakemir burakemir requested a review from guybedford November 11, 2024 12:24
@guybedford
Copy link
Collaborator

I'm asking one of two things:

  1. Fully support the shared state in Walrus in both parsing and serializing
  2. Throw if the shared = true state is found during parsing

You are welcome to do either, but we absolutely must do one to avoid incorrectly interpreting Wasm binaries.

@guybedford
Copy link
Collaborator

I suppose shared everything is disabled in the features by default so this is fine for now, we just need to watch if that default changes.

@guybedford guybedford merged commit 1e5d3e9 into rustwasm:main Nov 12, 2024
8 checks passed
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