wasmtime / PR #6281 winch(fuzz): Initial support for diff... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6281 winch(fuzz): Initial support for diff...

Wasmtime GitHub notifications bot (Apr 24 2023 at 23:56):

saulecabrera opened PR #6281 from saulecabrera:winch-fuzzing-init to bytecodealliance:main:

This commit introduces initial support for differential fuzzing for Winch. In order to fuzz winch, this change introduces the winch cargo feature. When the winch cargo feature is enabled the differential fuzz target uses wasmi as the differential engine and wasm-smith and single-inst as the module sources.

The intention behind this change is to have a local approach for fuzzing and verifying programs generated by Winch and to have an initial implementation that will allow us to eventually enable this change by default. Currently it's not worth it to enable this change by default given all the filtering that needs to happen to ensure that the generated modules are supported by Winch.

Wasmtime GitHub notifications bot (Apr 24 2023 at 23:56):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #6281.

Wasmtime GitHub notifications bot (Apr 24 2023 at 23:56):

saulecabrera requested fitzgen for a review on PR #6281.

Wasmtime GitHub notifications bot (Apr 24 2023 at 23:56):

saulecabrera requested wasmtime-core-reviewers for a review on PR #6281.

Wasmtime GitHub notifications bot (Apr 24 2023 at 23:56):

saulecabrera requested wasmtime-default-reviewers for a review on PR #6281.

Wasmtime GitHub notifications bot (Apr 25 2023 at 00:00):

saulecabrera updated PR #6281.

Wasmtime GitHub notifications bot (Apr 25 2023 at 00:00):

saulecabrera edited PR #6281:

This commit introduces initial support for differential fuzzing for Winch. In order to fuzz winch, this change introduces the winch cargo feature. When the winch cargo feature is enabled the differential fuzz target uses wasmi as the differential engine and wasm-smith and single-inst as the module sources.

The intention behind this change is to have a local approach for fuzzing and verifying programs generated by Winch and to have an initial implementation that will allow us to eventually enable this change by default. Currently it's not worth it to enable this change by default given all the filtering that needs to happen to ensure that the generated modules are supported by Winch.

It's worth noting that the Wasm filtering code will be temporary, until Winch
reaches feature parity in terms of Wasm operators.

Wasmtime GitHub notifications bot (Apr 25 2023 at 00:07):

saulecabrera requested cfallin for a review on PR #6281.

Wasmtime GitHub notifications bot (Apr 25 2023 at 00:09):

saulecabrera updated PR #6281.

Wasmtime GitHub notifications bot (Apr 25 2023 at 00:33):

cfallin submitted PR review:

This looks generally good-- I'm reviewing it with the understanding that the WInch-specific config details are temporary (in the long run) so I won't fuss too much about configuration details :-)

One thing I might suggest: the winch feature in a fuzz-build does something subtly different than the winch feature in the rest of Wasmtime, namely it restricts options rather than adds support. It turns the general fuzz target into a Winch-specific thing. So perhaps we could name the feature in the fuzz crate something more specific, like fuzz-winch-only or winch-fuzz or somesuch? (Then the feature can require the winch feature in the rest of the engine.)

LGTM with or without that -- I'll leave it up to you -- but thought it might be clearer. Thanks!

Wasmtime GitHub notifications bot (Apr 25 2023 at 01:12):

saulecabrera updated PR #6281.

Wasmtime GitHub notifications bot (Apr 25 2023 at 04:32):

cfallin merged PR #6281.

Wasmtime GitHub notifications bot (Apr 25 2023 at 15:09):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 25 2023 at 15:09):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 25 2023 at 15:09):

alexcrichton created PR review comment:

One thought I had reading over this is that the winch support here seems pretty specific and targeted, which while ok for now may not translate to easier integration in the future. For example having specific paths for winch such as this may not age well as more support is added.

An alternative, though, would be something like:

Always have CompilerStrategy

Only generate CompilerStrategy::Winch when fuzz-winch is enabled

Don't configure allowed_instructions, but allow modules fall through to winch_supports_module below

This would result in a higher "rejection rate" or a smaller rate of coverage for winch since modules won't be tailor-made to run in winch, but I think that could be possibly smoothed over with environment variable based configuration like ALLOWED_ENGINES?

Overall I think it'd be best to integrate without #[cfg] where possible since having special one-off instructions like that tend to accumulate more-complicated logic over time.

To be clear though this is more of a stylistic concern than anything else. I think it's fine to leave this all as-is and it can be revisited when the fuzzing support is updated in the future.

Wasmtime GitHub notifications bot (Apr 25 2023 at 15:09):

alexcrichton created PR review comment:

How come winch changes the selection of default engines?

Wasmtime GitHub notifications bot (Apr 25 2023 at 16:01):

saulecabrera created PR review comment:

Since the objective of this change is mostly to have a way to fuzz locally, my line of thought here was to choose the most lightweight option for differential fuzzing, in that sense, this decision is very specific to Winch and to how I intend to use this fuzz target today. Once Winch is robust enough, such that we are able to enable fuzzing in cluster-fuzz, I think we can revert this change. The other thing that I've discussed briefly with @cfallin is that maybe for Winch, even in cluster-fuzz we could consider only fuzzing Winch against an interpreter like wasmi in order to save some fuzz time in cluster-fuzz, and also because Winch is already partly covered by Cranelift's fuzzing given its dependency on its backends.

Wasmtime GitHub notifications bot (Apr 25 2023 at 16:20):

saulecabrera created PR review comment:

Agreed with all your points @alexcrichton, thanks for bringing this up; the only thing that I'll note is that the main reason for configuring allowed_instructions as part of this change is to have a higher change of getting a module supported by Winch, which is important for me at the moment, given that I'm only intending to use this locally, I found that configuring wasm-smith to do this would be the easiest for now.

In terms of the cfg complexity, my expectation is that as more features are added to Winch the complexity here shouldn't increase, aside from having to add the supported instructions to winch_supports_module. But in general, my expectation is that once decide to enable fuzzing for Winch by default, the cfg complexity should be minimal or non-existent and my hope if that we can control any Winch specific behaviour through environment variables as you suggested.

Wasmtime GitHub notifications bot (Apr 25 2023 at 17:59):

cfallin created PR review comment:

To add a bit, @alexcrichton the intent of the design here is to give temporary support for Winch bringup; as @saulecabrera noted we hope that eventually it's "just another engine in the list". The alternative to this would be to develop a completely separate "bringup fuzz target" doing the targeted comparison that we want, which we decided would be less ideal (code duplication etc).

Wasmtime GitHub notifications bot (Apr 25 2023 at 19:59):

alexcrichton created PR review comment:

That all makes sense to me, but for fuzzing locally the env vars can be set, so I'm not sure why this change was needed? It's not wrong of course, and I agree that there's no need for a whole separate fuzz target for winch. I was expecting, though, that how to fuzz winch would be "probably set some env vars to specific values to avoid known crashes and get quicker test coverage", so that way fuzzing winch in full is "remove the env vars" or similar.

Last updated: Oct 23 2024 at 20:03 UTC