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

Wasmtime toplevel module: re-export wasmparser. #9485

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Oct 18, 2024

In some use-cases, an embedder might also use wasmparser directly. It might be useful in these cases to keep the version the embedder uses in sync with the version that Wasmtime uses, both for exact feature-support compatibility reasons and to remove the redundancy in the dependency tree.

@cfallin cfallin requested a review from a team as a code owner October 18, 2024 18:34
@cfallin cfallin requested review from fitzgen and removed request for a team October 18, 2024 18:34
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.

I have some concerns around semver here.

This public dependency means that, in practice, we can't change our wasmparser dependency as part of any bug fixes that we want to backport, say for a security bug, because wasmparser doesn't really do patch releases. We don't usually do patch releases for wasm-tools because there are too many tools and crates and types and APIs to keep track of whether we had breaking changes or not, which are exactly the same difficulties we would run into here when backporting patches. And if we get this wrong, then we break embedders' compilations, which is a terrible downstream experience.

I also recognize that staying in sync with wasmtime's parsing is a valid use case, so I'm not sure what to do.

  • Am I overly worried about the semver issues? Making this a bigger deal than it should be?
  • Does it make sense to put this behind a cargo feature and document that we don't necessarily follow semver for this feature and its re-export?
  • Is there an alternative solution for this use case that I'm not aware of?

@cfallin
Copy link
Member Author

cfallin commented Oct 18, 2024

Excellent points. In a vacuum I agree it'd make sense to minimize what we promise -- the wasmtime API should be about Wasm execution only -- but as you note, this is motivated by a real issue on the other side, too.

I think it might be worth thinking about the bugfix case further. If the embedder has the requirement that its wasmparser dependency matches its parsing behavior with Wasmtime's, and if we need to support this requirement, then already wasmparser's behavior is relevant when we think about a wasmtime patch release. So I think this already means that we either need to have the property that backports work with the version of wasmparser matched with the old version of wasmtime, or if not, we do a patch release of wasm-tools. In practice I can't think of any backported bugfixes we've had recently at least that were tied closely to a version of wasmparser; are you aware of any?

I'm also totally fine with putting this re-export behind a feature. The question still remains whether we follow semver or whether it's a "semver-exempt" corner of the API when the feature is on; obviously the former is a nicer experience for any embedder with this requirement.

Anyway, my personal opinion is that as long as this use-case exists out there, we don't gain much in terms of global simplicity or ease of maintenance by pushing the requirement outside of our own local semver bounds, but I'd be curious what others (@alexcrichton ?) think too!

@fitzgen
Copy link
Member

fitzgen commented Oct 18, 2024

I'm not aware of any security backports that required wasmparser bumps. Again, maybe I'm being paranoid.

But for the use case of "I need to use the exact same parser as wasmtime" then I think it really does make sense to say "you really get the exact same parser that wasmtime uses, regardless of the versioning that wasmtime is otherwise using" and that semver opt-out motivates putting this behind a cargo feature, IMO. Taken altogether, that feels like a consistent approach.

In some use-cases, an embedder might also use wasmparser directly. It
might be useful in these cases to keep the version the embedder uses in
sync with the version that Wasmtime uses, both for exact feature-support
compatibility reasons and to remove the redundancy in the dependency
tree.
@cfallin
Copy link
Member Author

cfallin commented Oct 18, 2024

OK, cool, I've added an off-by-default feature that gates this re-export -- thanks!

Comment on lines +282 to +295
# Enables a re-export of wasmparser, so that an embedder of Wasmtime can use
# exactly the version of the parser library that Wasmtime does. Sometimes this
# is necessary, e.g. to guarantee that there will not be any mismatches in
# which modules are accepted due to Wasm feature configuration or support
# levels.
#
# Note that when this feature is enabled, the version of wasmparser that is
# re-exported is *not subject to semver*: we reserve the right to make patch
# releases of Wasmtime that bump the version of wasmparser used, and hence the
# version re-exported, in semver-incompatible ways. This is the tradeoff that
# the embedder needs to opt into: in order to stay exactly in sync with an
# internal detail of Wasmtime, the cost is visibility into potential internal
# version changes. This is why the re-export is guarded by a feature flag which
# is off by default.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for writing this up!

@fitzgen fitzgen added this pull request to the merge queue Oct 18, 2024
Merged via the queue into bytecodealliance:main with commit dc2c94d Oct 18, 2024
39 checks passed
@alexcrichton
Copy link
Member

Am I overly worried about the semver issues? Making this a bigger deal than it should be?

Personally I think this may be the case, I'm not too worried about this. I think this is something we'd have to be cognizant of when responding to a security issue but it wouldn't be the end of the world to issue backports for wasmparser too.

That being said I think the docs added here are fine. We can just see how it goes over time!

@cfallin cfallin deleted the reexport-wasmparser branch October 21, 2024 16:02
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.

3 participants