Skip to content

wasm-reduce unintentionally adds a feature #2813

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

Closed
kripken opened this issue Apr 28, 2020 · 2 comments
Closed

wasm-reduce unintentionally adds a feature #2813

kripken opened this issue Apr 28, 2020 · 2 comments

Comments

@kripken
Copy link
Member

kripken commented Apr 28, 2020

This annoying thing happens:

  • The reducer finds a bug. It uses no features while doing so on that testcase, plain MVP.
  • wasm-reduce adds -all to all its commands. It does that so that it can succeed to load all wasm files.
  • When it writes out the wasm after doing -all, it writes out the DataCount section, because bulk-memory has been enabled, and we always emit that section when that feature is enabled.
  • When we try to run wasm2c on that testcase it now fails, as wasm2c only supports MVP features.

IOW, wasm-reduce uses -all to work around feature errors, but enabling features - even when just reading and writing the wasm - has a side effect of turning the output wasm into actually using features 😢

@tlively I hope you'll have a good idea here...

@kripken
Copy link
Member Author

kripken commented Apr 28, 2020

Ah reading the code I see this promising comment:

        // TODO(tlively): -all should be replaced with an option to use the
        // existing feature set, once implemented.

So maybe this is expected and we have a plan?

@tlively
Copy link
Member

tlively commented Apr 28, 2020

Hmm, we have an option --detect-features that uses the target features section, but in general that section may not be present in the input. The simplest thing to do might be to add feature flags to wasm-reduce? I guess we could also add a pass or flag that detects used features and shrinks the enabled feature set to be minimal, but I've so far resisted doing that in favor of being more explicit and declarative.

kripken added a commit that referenced this issue Jan 15, 2021
This goes back to the downsides of #2813, but that seems unavoidable
as without this, testcases without the features section but that use features
did not work.

This PR at least makes it easy to customize the flags send to the commands.

See also #3393 (comment)
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

2 participants