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

Use new wast parser in wasm2js #6606

Merged
merged 2 commits into from
May 29, 2024
Merged

Use new wast parser in wasm2js #6606

merged 2 commits into from
May 29, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented May 18, 2024

When generating assertions, traverse the WASTScript data structure rather than
interleaving assertion parsing with emitting.

@tlively tlively requested a review from kripken May 18, 2024 00:52
Copy link
Member Author

tlively commented May 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlively and the rest of your teammates on Graphite Graphite

@@ -18,6 +18,10 @@ function asmFunc(imports) {

}

function $0() {

}
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea, and not for lack of trying to figure it out :/

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps wasm-reduce can help here? It could compare the old and new outputs and find the smallest possible input that causes a difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I thought this was an extra empty function that was being inserted at the beginning, but it's just the names that are being changed. The addition of function $0 in this diff is balanced by the removal of old function $4. The difference in function names is just a difference between the old and new text parsers when parsing unnamed functions.

tlively added 2 commits May 21, 2024 11:44
When generating assertions, traverse the `WASTScript` data structure rather than
interleaving assertion parsing with emitting.
@tlively tlively force-pushed the wasm2js-rewrite branch from 99ac5e2 to 6ca0bfa Compare May 29, 2024 02:10
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I see, thanks.

The small changes do make reviewing a little hard but I didn't see anything obviously worrying, and we do execute those outputs in check.py, so should be ok. We also have the emscripten wasm2js tests on the bots that will give additional coverage.

@tlively tlively merged commit f622b8e into main May 29, 2024
13 checks passed
@tlively tlively deleted the wasm2js-rewrite branch May 29, 2024 17:35
@gkdn gkdn mentioned this pull request Aug 31, 2024
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