Stream: git-wasmtime

Topic: wasmtime / PR #1343 Turn off binaryen in fuzzing by default

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:33):

alexcrichton opened PR #1343 from no-binaryen to master:

... but turn it back on in CI by default. The binaryen-sys crate
builds binaryen from source, which is a drag on CI for a few reasons:

• This is quite large and takes a good deal of time to build
• The debug build directory for binaryen is 4GB large

In an effort to both save time and disk space on the builders this
commit adds a binaryen feature to the wasmtime-fuzz crate. This
feature is enabled specifically when running the fuzzers on CI, but it
is disabled during the typical cargo test --all command. This means
that the test builders should save an extra 4G of space and be a bit
speedier now that they don't build a giant wad of C++.

We'll need to update the OSS-fuzz integration to enable the binaryen
feature when executing cargo fuzz build, and I'll do that once this
gets closer to landing.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:33):

alexcrichton requested fitzgen for a review on PR #1343.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:34):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:34):

alexcrichton created PR Review Comment:

@fitzgen curious on your thoughts on this. An alternative way to handle this would be to do this if binaryen is available but otherwise just pull out a Vec<u8> and assume it's wasm.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:35):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:35):

alexcrichton created PR Review Comment:

(the latter approach reducing the need for #[cfg] anywhere basically

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:36):

alexcrichton updated PR #1343 from no-binaryen to master:

... but turn it back on in CI by default. The binaryen-sys crate
builds binaryen from source, which is a drag on CI for a few reasons:

• This is quite large and takes a good deal of time to build
• The debug build directory for binaryen is 4GB large

In an effort to both save time and disk space on the builders this
commit adds a binaryen feature to the wasmtime-fuzz crate. This
feature is enabled specifically when running the fuzzers on CI, but it
is disabled during the typical cargo test --all command. This means
that the test builders should save an extra 4G of space and be a bit
speedier now that they don't build a giant wad of C++.

We'll need to update the OSS-fuzz integration to enable the binaryen
feature when executing cargo fuzz build, and I'll do that once this
gets closer to landing.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:42):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:42):

fitzgen created PR Review Comment:

Two problems with that approach:

1. It may confuse the fuzzer, since the input is interpreted differently in two different cases.
2. We rely on certain properties of the wasm-opt -ttf generated Wasm modules, such as that they have a global counter to prevent infinite loops, so it is safe for us to call their exports even though we haven't implemented an interruption API yet.

I think the #[cfg(...)]s are the way to go, unfortunately.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:46):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:46):

fitzgen created PR Review Comment:

This needs to take WasmOptTtf inputs, otherwise we have no guarantee that calling the wasm module's exports will terminate.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:46):

fitzgen created PR Review Comment:

Why don't we force enable the binaryen feature in this crate?

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:46):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:50):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:50):

alexcrichton created PR Review Comment:

Due to the way cargo calculates features it'd actually nullify the benefit of this PR, the feature would still be enabled for cargo test --all

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:52):

alexcrichton updated PR #1343 from no-binaryen to master:

... but turn it back on in CI by default. The binaryen-sys crate
builds binaryen from source, which is a drag on CI for a few reasons:

• This is quite large and takes a good deal of time to build
• The debug build directory for binaryen is 4GB large

In an effort to both save time and disk space on the builders this
commit adds a binaryen feature to the wasmtime-fuzz crate. This
feature is enabled specifically when running the fuzzers on CI, but it
is disabled during the typical cargo test --all command. This means
that the test builders should save an extra 4G of space and be a bit
speedier now that they don't build a giant wad of C++.

We'll need to update the OSS-fuzz integration to enable the binaryen
feature when executing cargo fuzz build, and I'll do that once this
gets closer to landing.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:52):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 16:52):

alexcrichton created PR Review Comment:

Sure thing, updated!

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:42):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 17:42):

fitzgen created PR Review Comment:

Can we --exclude wasmtime-fuzz so that this crate isn't part of the cargo test --all graph?

Wasmtime GitHub notifications bot (Mar 17 2020 at 18:58):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 18:58):

alexcrichton created PR Review Comment:

That does work, yeah, but I'd prefer to not require --exclude to get things working as expected. It's already sort of a bummer that you have to remember to at least --exclude lightbeam and I think it'd be better if we didn't require --exclude at all to get a reasonable set of testing locally by default.

Wasmtime GitHub notifications bot (Mar 17 2020 at 19:02):

alexcrichton updated PR #1343 from no-binaryen to master:

... but turn it back on in CI by default. The binaryen-sys crate
builds binaryen from source, which is a drag on CI for a few reasons:

• This is quite large and takes a good deal of time to build
• The debug build directory for binaryen is 4GB large

In an effort to both save time and disk space on the builders this
commit adds a binaryen feature to the wasmtime-fuzz crate. This
feature is enabled specifically when running the fuzzers on CI, but it
is disabled during the typical cargo test --all command. This means
that the test builders should save an extra 4G of space and be a bit
speedier now that they don't build a giant wad of C++.

We'll need to update the OSS-fuzz integration to enable the binaryen
feature when executing cargo fuzz build, and I'll do that once this
gets closer to landing.

Wasmtime GitHub notifications bot (Mar 17 2020 at 20:03):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Mar 17 2020 at 20:04):

fitzgen merged PR #1343.

Last updated: Dec 23 2024 at 12:05 UTC