wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

fuzzgen: Generate compiler flags

Open afonso360 opened this issue 3 years ago • 3 comments

👋 Hey,

This PR allows fuzzgen to generate compiler flags and test using those. We only allow compiler flags that shouldn't change the behaviour of the code.

I wanted to submit this now so that we could fuzz PR's such as #4953 and #5004 that require optimizations enabled.

However, when testing this it has found an issue with one of the passes (I haven't yet narrowed it down), but I don't want to start fixing it if we are going to change pretty much everything once #4953 lands. I'm opening this as a draft, and lets re test this once #4953 lands.

The actual bug manifests as following:

Fails with:

target/aarch64-unknown-linux-gnu/release/cranelift-fuzzgen: Running 1 inputs 1 time(s) each.
Running: fuzz/artifacts/cranelift-fuzzgen/crash-00e37c2858c35c573ed85d618ea5d0c75ae06fda
thread '<unnamed>' panicked at 'assertion failed: `(left != right)`
  left: `v137`,
 right: `v137`: Aliasing v137 to v134 would create a loop', cranelift/codegen/src/ir/dfg.rs:322:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

And I cannot reproduce via the CLIF file since that runs into https://github.com/bytecodealliance/wasmtime/issues/4875#issuecomment-1239314171 (which I kinda forgot about 😅 ). But I'm going to try and investigate that one in the mean time.

This also enables regalloc_checker as we discussed in #4979.

cc: @jameysharp

afonso360 avatar Oct 05 '22 13:10 afonso360

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "cranelift", "cranelift:meta", "fuzzing"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

github-actions[bot] avatar Oct 05 '22 14:10 github-actions[bot]

This PR had some performance issues:

  • The regalloc checker started consuming a bunch of time (around 45%) so we now try to minimize runs with it (10% with the current iteration).

  • I've also had to limit the amount of runs that we allow, at some point the fuzzer decided that spending 100% of the time in the interpreter was a good idea, and started generating huge inputs, that were in the order of 20k runs each. (up to 130k!)

execs/s seems way better than before, which is nice.

And for those who are interested, here's a flamegraph of where we spend time. This is about 2min at 1Khz around 190k inputs into my normal benchmark run:

flamegraph all

Edit: I should note, this is all on top of #5034

afonso360 avatar Oct 10 '22 19:10 afonso360

I've given this one round of fuzzing on AArch64 and x86, and it found the two issues with iconst, one has already been fixed in #5061 and I'm working on removing iconst.i128, once that is merged I'll give this another round of fuzzing and if everything goes right we should be able to merge this.

afonso360 avatar Oct 17 '22 21:10 afonso360

@cfallin What do you think about merging this with the use_egraphs flag commented out and enabling that on a separate PR?

It's getting a bit annoying having to rebase my other work on this PR just to fuzz it. I've fuzzed this for about an hour on both AArch64 and x86 without use_egraphs and it seems stable.

afonso360 avatar Oct 20 '22 21:10 afonso360

Oh, for sure, I didn't know we were waiting on something here; sorry! I was waiting for your go-ahead to merge. FWIW the PR is still marked as a draft; go ahead and transition it to "Ready for review" when you want me to merge.

I actually think maybe it's fine to include use_egraphs; better to get the fuzzbugs coming sooner than later, and I'm happy to take them as they come in. But I'm open to the other approach if you think otherwise.

cfallin avatar Oct 20 '22 21:10 cfallin

Oh, for sure, I didn't know we were waiting on something here; sorry!

I wasn't really, I just got a bit annoyed today at having to do a rebase for the third time, that's where that suggestion came from!

I'm okay with merging this as is and letting ossfuzz report issues as we go along.

afonso360 avatar Oct 20 '22 22:10 afonso360