-
Notifications
You must be signed in to change notification settings - Fork 745
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
Validate types are not stale, and fix many issues there #1011
Conversation
… we can't just replace the block with its contents naively as the parent type may then need to be updated
072daa0
to
948cba3
Compare
… in replaceCurrent, which is far less bug prone
This looks stable now. Sadly it is 6% slower than master (tested on poppler). The core issue is that full and proper handling of unreachable means that any time you remove a branch or alter a type to unreachable, you must recompute types for the entire potentially-affected tree underneath it. Perhaps we should reconsider our options here, but this PR should probably land since it fixes a large amount of fuzz bugs + adds a lot more testing to prevent more. |
…e value at the end
…on logic in both vacuum and dce
This is getting out of hand. May need to rethink the entire approach here - it is going to be very hard to actually fix all these issues. |
Hm. Sounds like we need to re-rethink what My gut feeling is that the complexity stems from:
|
afl-fuzz hinted in this direction, but this is much bigger than a fuzz bug fix. I think the full story is this:
finalize()
on the node changes its type. It can do that in only one legal way, turning a concrete value into unreachable, e.g. finalizing(block i32 (unreachable))
will remove the extra i32 annotation, and set the more natural unreachable type. But aside from that, if finalize would have changed the type, we have a bug, generally that the type is 'stale', it hasn't been recomputed. So I added that to validation.A sad thing in all of this is that we need to do more work. We have passes that may turn a node's type to unreachable, and logically that means we need to update the types of all its parents. That we didn't notice this before is due to a combination of not having the new extra validation, and that optimizations tend to work around it, in particular dce. I don't think we can avoid this, unless we get rid of the unreachable type.