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

Arbitrary IR fuzzing #1668

Merged
merged 15 commits into from
Jan 14, 2022
Merged

Arbitrary IR fuzzing #1668

merged 15 commits into from
Jan 14, 2022

Conversation

kvark
Copy link
Member

@kvark kvark commented Jan 13, 2022

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.

@kvark kvark requested a review from jimblandy January 13, 2022 18:29
Copy link
Member

@jimblandy jimblandy left a 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.

src/valid/function.rs Show resolved Hide resolved
src/valid/interface.rs Show resolved Hide resolved
src/valid/mod.rs Show resolved Hide resolved
@kvark kvark force-pushed the arbitrary branch 3 times, most recently from 57b17bf to be09528 Compare January 14, 2022 00:05
Copy link
Member

@jimblandy jimblandy left a 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 resembles get but returns a Result, 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.

src/proc/mod.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Member Author

kvark commented Jan 14, 2022

Thank you for the well-thought review!
I believe everything is addressed now. I'm going to merge it without squashing, since the commits are individual, and the only failures here are related to MSRV of arbitrary and the formatting stuff - we can live with that when bisecting.

@kvark kvark merged commit d468e15 into gfx-rs:master Jan 14, 2022
@kvark kvark deleted the arbitrary branch January 14, 2022 17:44
@kvark
Copy link
Member Author

kvark commented Jan 14, 2022

I had an abrupt connection loss and my last commit wasn't pushed here before merging. So following up with #1669

jimblandy added a commit to jimblandy/naga that referenced this pull request Jan 10, 2023
jimblandy added a commit to jimblandy/naga that referenced this pull request Jan 11, 2023
jimblandy added a commit to jimblandy/naga that referenced this pull request Jan 15, 2023
jimblandy added a commit that referenced this pull request Jan 15, 2023
Undo some changes from #1668, now that #2090 has been merged.
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