-
Notifications
You must be signed in to change notification settings - Fork 747
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
Conversation
…t to from.wast. this allows wast tests with comments in them
If we can get away with disabling just one spec test instead of all of them, let's definitely do that. |
Yeah, makes sense. Looking more carefully, two now fail, |
…n, not a drop (of course...)
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. |
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). |
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. |
This LGTM to fix the various brokenness. We shouldn't call #903 resolved yet though. |
LGTM as well |
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. |
Third attempt at resolving #903. This
br
s.unreachable
opcodes, see comments in the diff. This is a simple solution, but not the most efficient, see below.This also
from-wast
outputs, instead of assuming that the input == the output, which means no comments).return.wast
, so perhaps we should just disable that one?Other issues:
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%).