Stream: git-wasmtime

Topic: wasmtime / PR #4862 cranelift-fuzzgen: Consume all traili...

Wasmtime GitHub notifications bot (Sep 03 2022 at 01:48):

jameysharp requested fitzgen for a review on PR #4862.

Wasmtime GitHub notifications bot (Sep 03 2022 at 01:48):

jameysharp opened PR #4862 from test-inputs-length to main:

But don't keep going once we've consumed it all.

I got tired of having a bunch of copies of the same all-zeros test inputs at the end. This version always produces exactly one all-zeros test input at the end, which I feel okay about. I could remove that by explicitly checking for self.u.is_empty() but, eh, testing an all-zeros input could be useful.

This uses less of the fuzz input too since it doesn't need to come up with a count of how many test inputs to generate.

This changes the input format for this fuzz target, so I figure I'll bundle it together with #4824 and merge both sometime next week.

cc: @afonso360

Wasmtime GitHub notifications bot (Sep 05 2022 at 08:26):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 06 2022 at 18:10):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 06 2022 at 18:10):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 06 2022 at 18:10):

fitzgen created PR review comment:

Might be simpler as

loop {
    inputs.push(todo!("generate one input"));

    if u.is_empty() {
        break;
    }
}

which avoids the semi-subtle use of usize::MAX here.

Wasmtime GitHub notifications bot (Sep 06 2022 at 19:08):

jameysharp created PR review comment:

That's how I wrote it first, actually! I could go back to loop plus a conditional break at the end if you want. But just testing u.is_empty doesn't work if generating an input doesn't consume any of u: it loops forever then. The usual way that would happen is if the generated function doesn't take any parameters, but I haven't ruled out the possibility that a parameter could only have one legal value and therefore consume no input. So instead I check that the loop makes some progress on every iteration, and stop after the first iteration where it doesn't make progress.

I agree that using usize::MAX here is subtle. We'd get nearly the same result from initializing last_len to self.u.len() + 1, if that would make you feel better?

Wasmtime GitHub notifications bot (Sep 06 2022 at 19:08):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 06 2022 at 23:20):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 06 2022 at 23:20):

fitzgen created PR review comment:

I guess we could also have a maximum length on inputs as well.

Otherwise I guess I would prefer making last_len an option and doing something like

while last_len.map_or(true, |last_len| self.u.len() < last_len) {
    ...
}

Wasmtime GitHub notifications bot (Sep 06 2022 at 23:44):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 06 2022 at 23:44):

jameysharp created PR review comment:

I'm definitely not a fan of that alternative :laughing: but I can still move the test to the end of the loop, if that works for you.

Wasmtime GitHub notifications bot (Sep 07 2022 at 01:37):

jameysharp updated PR #4862 from test-inputs-length to main.

Wasmtime GitHub notifications bot (Sep 07 2022 at 15:46):

jameysharp merged PR #4862.

Last updated: Dec 23 2024 at 13:07 UTC