Stream: git-wasmtime

Topic: wasmtime / PR #5998 cranelift: Fuzz mid-end optimizations

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2023 at 15:58):

ghostway0 opened PR #5998 from issue-5967 to main:

This has been discussed on Zulip as well as in #5967.
The PR fuzzes and tests after optimization or on the host by demand in the cranelift-fuzzgen target.
I do not know which maintainer should be assigned to review this, but I'd imagine @jameysharp.
This does fail when optimized with a HeapMisaligned trap, and I didn't manage to find why

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp created PR review comment:

Hey @afonso360, I don't remember: why did we have a check for RunResult::Success before asserting that both runs produced the same result?

The earlier match int_res only continues to here if int_res is Success, so the final assert_eq! would panic in the same cases where this match panics, and tell us the values of the results at the same time.

Can we just delete this match? That would make it easier to assert that both interpreter runs produce the same trap: we'd only have to decide above whether to continue or fall through on trap.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp created PR review comment:

This shouldn't panic if it can't generate a random boolean. It should just propagate the error to the caller. Does this work?

        let should_optimize = gen.u.arbitrary()?;

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp created PR review comment:

We should come up with a better name than should_optimize. There are flags elsewhere for different optimization levels and options, which this doesn't affect. At the same time, the main thing this controls is whether to compare against native code or the interpreter, and the name doesn't reflect that.

I'm having trouble coming up with a good name though. run_native maybe? compare_against_host?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp created PR review comment:

I have a slight preference for the impl Trait syntax in cases like this:

fn run_test_inputs(testcase: &TestCase, run: impl Fn(&[DataValue]) -> RunResult) {

I'm tempted to use &dyn instead of impl, actually; I doubt there's anything to gain from compiling separate versions of run_test_inputs for the two cases. But that's a micro-optimization and probably not important.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp created PR review comment:

There's an existing comment in run_test_inputs that I think is worth copying here:

        // We rebuild the interpreter every run so that we don't accidentally carry over any state
        // between runs, such as fuel remaining.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 20:09):

jameysharp created PR review comment:

I don't think args needs to be borrowed here, right? It's already a &[DataValue] which is what run_in_interpreter expects.

            run_in_interpreter(&mut interpreter, args)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 21:15):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 21:15):

afonso360 created PR review comment:

I don't know, and yeah, we should be probably remove the match.

I checked the commit log and this dates all the way back to the beginning of fuzzgen, almost two years ago! I think it was probably just an oversight.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 21:22):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 21:22):

afonso360 created PR review comment:

I can't highlight the actual line, but can we print a comment to the resulting test case if we choose the should_optimize flag? Or even better, add a different variant of PrintableTestCase that gets us a better representation of this test case?

When the fuzzer finds an issue we try to print a .clif file that reproduces the issue, that isn't always possible, but we try to get it as close as we can so that its easier to reproduce later.

In this case we don't have any way of running the interpreter after optimizations in the test case, but adding a comment would be really helpful so that we know that this is sort of a special case.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 21:23):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 21:25):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 21:26):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 23:13):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2023 at 23:13):

jameysharp created PR review comment:

I'd definitely like to have the optimized test case in the Debug output, but we have to be a little careful. If anything in the optimizer panics, we wouldn't get any debug output at all.

To keep things simple for the moment, I think I just want your first suggestion: add a comment to the Debug output indicating whether the test case compared the interpreter against the host, or against interpreting optimized CLIF.

I think we should add a command line mode for cranelift-tools that runs the optimization pass and pretty-prints the resulting CLIF. Then we don't need to do anything further in fuzzgen's Debug formatter.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:04):

ghostway0 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:04):

ghostway0 created PR review comment:

The possibly-overkill end of the spectrum is make some kind of diff utility. But that is out of scope for this PR (... probably). Should I just add a println before running the PrintableTestCase?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 07:07):

ghostway0 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 07:15):

ghostway0 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 09:59):

ghostway0 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 09:59):

ghostway0 created PR review comment:

Both are micro-optimizations -- the template one was just my default. What do you prefer/is present?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 10:05):

ghostway0 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 10:05):

ghostway0 created PR review comment:

Yes, I didn't find the best name. compare_against_host sounds nice

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 10:05):

ghostway0 updated PR #5998 from issue-5967 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 10:07):

ghostway0 updated PR #5998 from issue-5967 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 11:09):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 11:09):

afonso360 created PR review comment:

Yeah, I think write!ing something before printing the test case sounds like a good idea.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 19:07):

ghostway0 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 19:07):

ghostway0 created PR review comment:

Thing is, PrintableTestCase prints some stuff before the test attributes

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 19:11):

ghostway0 edited PR #5998 from issue-5967 to main:

This has been discussed on Zulip as well as in #5967.
The PR fuzzes and tests after optimization or on the host by demand in the cranelift-fuzzgen target.
I do not know which maintainer should be assigned to review this, but I'd imagine @jameysharp.

Closes #5967

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 20:49):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 20:49):

afonso360 created PR review comment:

No worries, just print out a CLIF comment indicating which way we went with the compare_against_host flag. Just so we know if we need a bit of manual work to reproduce the case. As long as it's in a comment format it shouldn't really affect the reproducibility of the test case.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 10:52):

ghostway0 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 10:52):

ghostway0 created PR review comment:

I have this feeling that TestCase does multiple things when to_optimized is used. Before, it was something like an options container -- now it isn't anymore. But I don't think a restructure is preferable to this itchy problem :)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 14:03):

ghostway0 updated PR #5998 from issue-5967 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 14:03):

ghostway0 has marked PR #5998 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 14:04):

ghostway0 edited PR #5998 from issue-5967 to main:

This has been discussed on Zulip as well as in #5967.
The PR fuzzes and tests after optimization or on the host by demand in the cranelift-fuzzgen target.
Closes #5967

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 23:17):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 23:17):

jameysharp created PR review comment:

I'm happy with the impl Trait version, which in an argument works exactly the same as the <F: Trait> syntax, so it's just a style preference. Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 23:46):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 16 2023 at 06:54):

ghostway0 updated PR #5998 from issue-5967 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 16 2023 at 16:54):

jameysharp has enabled auto merge for PR #5998.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 17 2023 at 03:46):

alexcrichton merged PR #5998.

Last updated: Oct 23 2024 at 20:03 UTC