-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fuzzgen: Refactor name and signature generation #5764
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.
This looks promising!
I think I'd prefer configuring a probability of selecting each libcall, rather than configuring a number of calls. I'd rather have each one appear either zero or one times, instead of potentially many times. Or perhaps we should always pass in the full list of libcalls; what test coverage do we gain by randomly removing libcalls from the list?
It might be nice to only add function declarations to builder.func
if they're actually used, but that's a separate matter.
I spent a while trying to figure out if this PR makes it any easier to move the list of ALLOWED_LIBCALLS to cranelift-fuzzgen so we don't have to keep it in sync across different modules with the list of supported interpreter implementations. I didn't come up with a good answer, but it's something I'd like to see, if you happen to have any ideas.
This is what ended up happening anyway in #5765, I'm going to backport that. I don't think we lose anything by doing it that way and its a bit cleaner.
Now that you mention it, that sounds like a great idea! And it solves one of the issues in #5765 which is that, the function "headers" become quite verbose if we pass in too many call targets which is annoying because they currently don't get reduced so even the smallest function possible still has 6 lines of signatures + 6 lines of function references! I'm going to give this a go. Would you prefer that as a separate PR or should I bundle it into this one?
The only thing I can think of would be to make
I think this could work, but I'm not sure if we can const eval that? |
Regarding cleaning up the function header, I'm happy whether that's in this PR or separate. I'd take this PR even if you don't get around to that. So pick whichever is more convenient for you. When I look at the associated constant idea, I wonder: why do we export types like If we declared that struct in the cranelift-fuzzgen target instead, we just need to provide implementations of I think the same plan works for I'd prefer if both Again, though, none of that needs to happen in this PR. |
c5e4ba8
to
b5054c7
Compare
I've changed this to always pass all libcalls. I like that idea of having each target defining its own Arbitrary type. I'll make that change as a separate PR! Same thing with the function header cleanup, I'll do that in a separate PR. |
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.
Looks good, thank you!
👋 Hey,
This PR includes a few refactors to fuzzgen in order to make it easier to generate testcases with multiple functions.
The first part (first 2 commits) are sort of centralizing generation of some cranelift data structures into a trait for arbitrary.
It also moves name, signature and function references generation out of
FunctionGenerator
and intoFuzzGen
. This will allow us to control the call flow graph and avoid recursion and infinite loops when we do generate multiple functions.This PR by itself gets us generating calls to
LibCall
s on fuzzgen since we no longer just outright block funcref generation.Ran this in the fuzzer for a while and nothing came up.