-
Notifications
You must be signed in to change notification settings - Fork 193
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
Arbitrary IR fuzzing #1668
Arbitrary IR fuzzing #1668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to fixing CI, I think we need to change the errors being used here.
57b17bf
to
be09528
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to get these changes in, but I definitely think there are two loose ends we should tie up.
- We should have a single error type for bad handles, used everywhere. See my comments in the patch for some ideas. Another idea would be to have this error type defined in
arena.rs
, returned directly by some function that resemblesget
but returns aResult
, and then have#[transparent]
variants in the error types of functions that use those arena methods. But, it's important for us to establish a clear practice for this, because otherwise everyone who contributes patches to the validator or processors is going to improvise. - It's not spelled out in any way where it's okay to unwrap
size
, and where one should pass an error. I know that the rationale right now is, "If validation should already have caught it, then unwrap," but I don't think people are going to pick up on this on their own. There should at least be comments. But it might? be better to have separate functions, designated for use only on validated modules, that panic on bad handles. I'm not sure it's much better - but either way, there needs to be a comment we can point people at that explains the policy.
Thank you for the well-thought review! |
I had an abrupt connection loss and my last commit wasn't pushed here before merging. So following up with #1669 |
Undo some changes from gfx-rs#1668, now that gfx-rs#2090 has been merged.
Undo some changes from gfx-rs#1668, now that gfx-rs#2090 has been merged.
Undo some changes from gfx-rs#1668, now that gfx-rs#2090 has been merged.
This change adds
arbitrary
feature and implements fuzzing of IR based on it, with cargo-fuzz.We aren't enabling it on CI yet because it catches a ton of low-hanging fruits - let's fix those first.