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

Fix emitting of unreachable block/if/loop #911

Merged
merged 23 commits into from
Feb 16, 2017
Merged

Fix emitting of unreachable block/if/loop #911

merged 23 commits into from
Feb 16, 2017

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 16, 2017

Third attempt at resolving #903. This

  • Defines unreachable blocks as suggested by @dschuff: having an unreachable element, and no brs.
  • Emits unreachable block/if/loop by adding extra unreachable opcodes, see comments in the diff. This is a simple solution, but not the most efficient, see below.

This also

  • Fixes a bunch of minor stuff uncovered by these changes, e.g., a missing relooper finalize, and we used to think that an if without an else was unreachable if the body was unreachable.
  • Makes it easier to add wast tests with comments (by adding from-wast outputs, instead of assuming that the input == the output, which means no comments).
  • Disables the spec tests. They fail on return.wast, so perhaps we should just disable that one?

Other issues:

  • Running the full test suite + fuzzing now, still not 100% sure this is correct.
  • We likely cannot read all our wasm outputs in binaryen now - we will need to support fully stacky input, which we want to anyhow, but this temporarily may cause issues for us (not regular users).
  • We emit extra bytes, at least 2 per unreachable block. We also sometimes emit unreachable unreachable, e.g. if unreachable things nest, etc. We should work towards optimizing all those out, as suggested in Proposal for new Binaryen IR type system #903, we can hopefully infer types etc. Meanwhile, the extra size is almost nothing, e.g. on libcxx it was 374,682 bytes and grows by 176 (so far less than 1%).

@kripken kripken mentioned this pull request Feb 16, 2017
@dschuff
Copy link
Member

dschuff commented Feb 16, 2017

If we can get away with disabling just one spec test instead of all of them, let's definitely do that.

@kripken
Copy link
Member Author

kripken commented Feb 16, 2017

Yeah, makes sense. Looking more carefully, two now fail, return and unreachable, I disabled only them.

@kripken
Copy link
Member Author

kripken commented Feb 16, 2017

So actually it turns out those two spec tests were testing something quite important ;) I had a bug in emitting of ifs with unreachable conditions. Fixed, and re-enabled them, which means all spec tests are back.

@kripken
Copy link
Member Author

kripken commented Feb 16, 2017

I'm now fairly confident this works: 3/4 of the test suite checked so far, all passing; fuzzing for several hours found nothing; and building unity generates a validating wasm (although I can't run it to confirm correctness, hopefully @juj can).

@kripken
Copy link
Member Author

kripken commented Feb 16, 2017

Test suite passes. Fuzzing found 2 issues, one of which which is fixed by that last commit (and was also a bug in sm). The other is a validation error I am still investigating.

@dschuff
Copy link
Member

dschuff commented Feb 16, 2017

This LGTM to fix the various brokenness. We shouldn't call #903 resolved yet though.

@jgravelle-google
Copy link
Contributor

LGTM as well

@kripken
Copy link
Member Author

kripken commented Feb 16, 2017

Pushed a fix for the remaining fuzz testcases from last night.

It's a tiny/obvious fix so I assume lgtms are still in effect, will merge once green.

@kripken kripken merged commit 5e29a61 into master Feb 16, 2017
@kripken kripken deleted the fix-O0-v8-c branch February 16, 2017 22:27
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