Component comments #1
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add 'outer' case to <core:alias> to match Binary.md
Resolves #75
Fix size_flags for the zero-flags case
Add some missing trap cases
Switch (back) to multi-return, remove unit, s/expected/result/
Define actual "Canonical" ABI
I noticed a couple spots in Explainer.md where canonical-ABI functions return multiple values. This fixes those spots to return a pointer to those values instead, properly following the canonical ABI.
Respect `MAX_FLAT_RESULTS` in Explainer.md
Resolves #84
…import Fix example which used one-level imports in a core module type
Resolves #90
Resolves #89
Resolve #25
Add explicit result arity to start definitions
Fix inline aliases used in 'memory' examples
Add JS API notes about variants
Tweak variant/result mangling to produce valid wit
* Add support for implementation imports * Rename <export> to <projection> to avoid clash * Change , separator to space for consistency * Add semver range note * Fix semver range ordering text * Add naked integrity hash import name * Fill in missing OCI Registry reference
* Point users to docs instead of this spec * Link to docs from explainer
* add use-path grammar * re-use interface rule instead of use-path * Rename interface production to use-path
This commit commit is an implementation of #142 where semicolons are now
required as delimiters between items in the WIT text format. All items
in the WIT format are now delimited with either curly braces (`{}`) or
semicolons except for the `package` statement where it subjectively felt
a bit weird to require a semicolon. I've updated the various examples in
`WIT.md` as an example of the new syntax.
My plan on implementing this would be along the lines of:
* Implement the semicolon syntax in `wit-parser`
* Add a parser mode which requires semicolons. This means that the same
`wit-parser` crate can either or either not require semicolons.
* Update all tests in the `wasm-tools` repository to require semicolons.
* Publish `wit-parser` and `wasm-tools`, integrating the
semicolon-supporting-mode into all existing tools.
* Wait for Wasmtime to get published with this support. At this point
everything in the ecosystem should have a point where semicolons are
optionally supported.
* Remove the parser mode which doesn't require semicolons, meaning
semicolons are now required.
* Push this update through the tooling, fixing any issues that arise.
The hope is to create a period of time where both syntax forms are
accepted. This provides a transitionary means from one syntax to the
other while proposals are updated. This transitionary period is finite
in length, however.
Closes #142
* Clarify what's in Preview 2 Resolves #229 * Split out package and interface id cases in the grammar * Require the 'refines' immediate to be 0
This is a follow-up to #249 and discussion in today's component model meeting to require semicolons after the `package` statement to ensure it's consistent with all other "single line things" in the WIT format.
Resolves #247
* Add subsection describing Index Spaces Resolves #261 * Distinguish the core index spaces in WebAssembly 1.0 * Remove angle brackets in production names for consistency
* Change encoding of WIT definitions into Component Model types * Fix explanation of world encoding * Export interfaces and worlds as instances/components, not types * Update dangling use of 'instance type' to 'instance' missed by last commit
…pes (#266) Resolves #265
… string (#263) * Move structured import/export name information into the string Resolves #253 * Replace the junk byte with a fixed 0x00 byte
rossberg
commented
Nov 29, 2023
| cx = CallContext(opts, inst) | ||
| trap_if(not inst.may_enter) | ||
|
|
||
| assert(inst.may_leave) |
There was a problem hiding this comment.
Is this really an assert? Can't it be triggered by invalid user code, e.g., a realloc doing something wrong?
There was a problem hiding this comment.
I believe it's not possible for user code to trigger this based on this reasoning:
- our base case is that
may_leavestarts as true - we set
may_leaveto false only when lowering values (params of a lift, results of a lower)- during lowering, arbitrary core wasm may indeed be invoked (through
realloc) but:- we're in core wasm, so we can't directly call
canon_lift - we can directly call
canon_lower, but then we'll immediately hit thetrap_if(not may_leave)before we can successfully get out of core wasm (so that we can attempt to callcanon_lift)
- we're in core wasm, so we can't directly call
- during lowering, arbitrary core wasm may indeed be invoked (through
There was a problem hiding this comment.
That sounds plausible. It's been a while, so I don't remember what scenario I had in mind, but the fact that core Wasm cannot directly call canon_lift is key.
| cx = CallContext(opts, inst) | ||
| trap_if(not inst.may_leave) | ||
|
|
||
| assert(inst.may_enter) |
There was a problem hiding this comment.
The same sort of argument as I gave above applies here, but in reverse:
may_enterstarts true- we set
may_enterto false during an import call that leaves the component, during which time:- we can't be reentered by the other component, since doing so will necessarily need to go through
canon_lift, which will immediately trap at thetrap_if(not may_enter) - we can run core wasm in the caller's component, but only while lowering the import results, which is bracketed by clearing
may_leave, which will cause us to trap if we attempt to callcanon_lowerfromrealloc.
- we can't be reentered by the other component, since doing so will necessarily need to go through
Comment on lines
+1571
to
+1574
| Because `may_enter` is not cleared on the exceptional exit path taken by | ||
| `trap()`, if there is a trap during Core WebAssembly execution of lifting or | ||
| lowering, the component is left permanently un-enterable, ensuring the | ||
| lockdown-after-trap [component invariant]. |
There was a problem hiding this comment.
That definitely suggests that the above check on may_enter is not an assertion but can fail due to ill-behaved user code.
There was a problem hiding this comment.
I don't think so, but let me know if you still see a case not covered by the above arguments.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.