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

[interpreter] Fix symbolic local resolution with typeuse #1213

Merged
merged 1 commit into from
Aug 17, 2020
Merged

Conversation

rossberg
Copy link
Member

@rossberg rossberg commented Jun 18, 2020

Fixes #1212.

There is a complication here. In order to maintain the malformed/invalid distinction properly, we need to make sure that type definitions are only looked up during parsing when actually needed for computing symbolic indices. Otherwise, we'd turn some syntactically valid, fully desugared programs like the following:

(module (func (type 0) (local i32)))

into triggering a syntax error instead of a validation error (due to an undefined type index). My first naive attempt at a fix actually did that and regressed one test expectation from assert_invalid to assert_malformed, which e.g. means that it could not be converted to binary anymore.

Unfortunately, we generally don't know whether the type will be needed until we have seen the full function body and know whether it contains any symbolic local (consider let in particular).

To address that, the parser now uses a work list to defer looking up the function type (and consecutive index shifts for anonymous binders) until actually needed for computing symbolic indices. In a fully desugared program it is never triggered.

This is a bit ugly, but most implementations probably don't need to care about this distinction. But for the reference interpreter it seems relevant.

@rossberg rossberg requested a review from binji June 18, 2020 12:48
@@ -873,7 +873,7 @@

.. Stack, meta functions

.. |expand| mathdef:: \xref{exec/runtime}{syntax-frame}{\F{expand}}
.. |expand| mathdef:: \xref{exec/runtime}{exec-expand}{\F{expand}}
Copy link
Member

Choose a reason for hiding this comment

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

Is this an unrelated change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, yeah, I didn't notice I still had that in my tree. One could argue it's somewhat related to typeuse, though, since it expands type uses in block types. :)

@binji
Copy link
Member

binji commented Jun 22, 2020

Strange, the IPR bot says that you haven't joined the community group. Did your email address change?

@rossberg
Copy link
Member Author

@binji, yeah, I have had this problem for a while, ever since I merged my two GH accounts. I couldn't figure out how to fix it. It tells me I need to join the CG (with some other account?), but when I try by clicking the link it tells me that I've already joined.

@rossberg rossberg merged commit b35206a into master Aug 17, 2020
@rossberg rossberg deleted the typeuse branch August 17, 2020 07:42
gumb0 pushed a commit to wasmx/wasm-spec that referenced this pull request Sep 18, 2020
gumb0 pushed a commit to wasmx/wasm-spec that referenced this pull request Sep 21, 2020
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.

[interpreter] Local variable index is incorrect with function type use
2 participants