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

fuzzgen: Add scalar float support #4467

Merged
merged 3 commits into from
Jul 21, 2022

Conversation

afonso360
Copy link
Contributor

@afonso360 afonso360 commented Jul 19, 2022

👋 Hey,

This PR adds support for generating floats in fuzzgen and adds a bunch of instructions.

I think we have all the float ops, except bit ops that operate on floats.

So far this has found #4446 and #4460. We can merge this before #4460 but it will probably start crashing in oss-fuzz.

cc: @jameysharp

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Jul 19, 2022
@jameysharp jameysharp self-requested a review July 19, 2022 19:52
Copy link
Contributor

@jameysharp jameysharp 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 this looks great and I'm excited to get more fuzzing for Cranelift!

I saw one copy-paste bug and sort of expect I might have missed some, because having a separate function for each arity leaves a lot of opportunities for this kind of bug. I don't mind merging this with just the one bug fixed, but if you're willing to take a little time to see if you can refactor that, that would be cool.

I'm trying to decide whether to wait on #4460 before merging this. I don't know how long that will take to get reviewed. If you're currently only seeing bugs in the Fma opcode, how would you feel about commenting out that opcode in this patch? Then there's no question, I'm happy to merge it immediately.

cranelift/fuzzgen/src/function_generator.rs Outdated Show resolved Hide resolved
@afonso360
Copy link
Contributor Author

If you're currently only seeing bugs in the Fma opcode, how would you feel about commenting out that opcode in this patch? Then there's no question, I'm happy to merge it immediately.

Sure, sounds good. I've commented it out and left a link on it.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you!

@jameysharp
Copy link
Contributor

Fantastic! We're hitting CI hard today so this probably won't get a chance for the CI checks to run for a while, but I'm setting it to auto-merge once that passes. Thanks for your efforts on this!

I think I understand why fuzzing didn't catch the misuse of ListPool: since you created a new pool, all the values in your ValueLists had small indexes (0..3) into the pool, and it's not so surprising that those were valid indexes into the DFG's pool as well. Or something like that.

@jameysharp jameysharp enabled auto-merge (squash) July 20, 2022 20:19
@alexcrichton
Copy link
Member

Apologies for the CI breakage, but things should be fixed with a rebase.

Add support for generating floats and some float instructions.
Both IEEE754 and the Wasm spec are somewhat loose about what is allowed
to be returned from NaN producing operations. And in practice this changes
from X86 to Aarch64 and others. Even in the same host machine, the
interpreter may produce a code sequence different from cranelift that
generates different NaN's but produces legal results according to the spec.

These differences cause spurious failures in the fuzzer. To fix this
we enable the NaN Canonicalization pass that replaces any NaN's produced
with a single fixed canonical NaN value.
This deduplicates a few inserters!
auto-merge was automatically disabled July 20, 2022 22:30

Head branch was pushed to by a user without write access

@jameysharp jameysharp enabled auto-merge (squash) July 20, 2022 22:45
@jameysharp jameysharp merged commit a0a2fd1 into bytecodealliance:main Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants