Stream: git-wasmtime

Topic: wasmtime / issue #6254 fuzz: randomize block lowering order

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 20:07):

jameysharp commented on issue #6254:

Would you mind pulling the lb_to_bindex change out to a separate PR? I'd like to review and benchmark that on its own.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 20 2023 at 20:27):

remlse commented on issue #6254:

@jameysharp sure thing: #6255

view this post on Zulip Wasmtime GitHub notifications bot (Apr 27 2023 at 08:18):

remlse commented on issue #6254:

Hi @fitzgen , I'm still getting errors from cargo vet. I merged the main branch where the wildcard audit was added. Do I need to do something else to make the audit work? I think we'd also need a wildcard audit for derive_arbitrary, right?

Run cargo vet --locked
Vetting Failed!

2 unvetted dependencies:
  arbitrary:1.3.0 missing ["safe-to-deploy"]
  derive_arbitrary:1.3.0 missing ["safe-to-deploy"]

view this post on Zulip Wasmtime GitHub notifications bot (Apr 27 2023 at 17:36):

fitzgen commented on issue #6254:

I think we'd also need a wildcard audit for derive_arbitrary, right?

Added in https://github.com/bytecodealliance/wasmtime/pull/6294

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2023 at 09:54):

remlse commented on issue #6254:

Alright, I think this should be ready to review @cfallin

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2023 at 20:48):

cfallin commented on issue #6254:

@remlse would you mind looking into the CI failure in the merge queue?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2023 at 08:30):

remlse commented on issue #6254:

Will do. I'm having trouble reproducing locally, but I'll get there.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 30 2023 at 17:14):

remlse commented on issue #6254:

Well, this was quite the rabbit whole.

I can reproduce now, compiling the fuzz target component_api causes some kind of infinite loop / memory leak during compilation. RAM consumption of rustc slowly crawls up to 100% (30GB), at which point my PC freezes. The trigger seems to be the update to (derive_)arbitrary 1.3.

With git bisect, I narrowed it down to the diff of this commit range in the arbitrary repo: 7ebc6c8...15c340e. (Given the rest of this story, I don't think the problem is in the arbitrary crate. There's nothing crazy going on in this diff.)

The build script of wasmtime-fuzz generates some code, which is included in the component_api target with

include!(concat!(env!("OUT_DIR"), "/static_component_api.rs"));

Compiling these 15 MB of generated code no longer terminates with higher versions of arbitrary. I reduced the number of generated test cases from 100 to 1 to make it easier to inspect the generated code. That actually fixed the problem, though, building the fuzz targets now succeeded.

So I started bisecting the number of test cases. But I soon noticed that not even compiling 2 test cases works. At least with 2 test cases, my memory was enough to get me to a compiler crash.

error: the compiler unexpectedly panicked. this is a bug.

Lovely. I'll attach the full output for future reference: rustc_panic.txt. And I made a branch with (hopefully) all the necessary changes to reproduce here. The command I used is

WASMTIME_FUZZ_SEED=1234 cargo fuzz build --no-default-features --dev -s none

I'll have to call it a day. I guess the next debugging step for me would be to try to understand the code generated in the build script - and how it could confuse the compiler.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 08:18):

remlse edited a comment on issue #6254:

Well, this was quite the rabbit hole.

I can reproduce now, compiling the fuzz target component_api causes some kind of infinite loop / memory leak during compilation. RAM consumption of rustc slowly crawls up to 100% (30GB), at which point my PC freezes. The trigger seems to be the update to (derive_)arbitrary 1.3.

With git bisect, I narrowed it down to the diff of this commit range in the arbitrary repo: 7ebc6c8...15c340e. (Given the rest of this story, I don't think the problem is in the arbitrary crate. There's nothing crazy going on in this diff.)

The build script of wasmtime-fuzz generates some code, which is included in the component_api target with

include!(concat!(env!("OUT_DIR"), "/static_component_api.rs"));

Compiling these 15 MB of generated code no longer terminates with higher versions of arbitrary. I reduced the number of generated test cases from 100 to 1 to make it easier to inspect the generated code. That actually fixed the problem, though, building the fuzz targets now succeeded.

So I started bisecting the number of test cases. But I soon noticed that not even compiling 2 test cases works. At least with 2 test cases, my memory was enough to get me to a compiler crash.

error: the compiler unexpectedly panicked. this is a bug.

Lovely. I'll attach the full output for future reference: rustc_panic.txt. And I made a branch with (hopefully) all the necessary changes to reproduce here. The command I used is

WASMTIME_FUZZ_SEED=1234 cargo fuzz build --no-default-features --dev -s none

I'll have to call it a day. I guess the next debugging step for me would be to try to understand the code generated in the build script - and how it could confuse the compiler.

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

alexcrichton commented on issue #6254:

Oh dear if that script is building a 15M source file that's way too big, let me see if the limits should be trimmed on that.

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

alexcrichton commented on issue #6254:

Ok I've pushed up https://github.com/bytecodealliance/wasmtime/pull/6315 which, when testing locally, gets the size of the generated test file to be a more reasonable 200K instead of 15M

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 16:27):

alexcrichton commented on issue #6254:

Ok let's try again and fingers crossed on this one. Thanks again @remlse for taking the time to investigate the failure!

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 16:27):

alexcrichton commented on issue #6254:

Oh actually I think this'll need to be rebased, but it should be able to merge afterwards.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 17:33):

remlse commented on issue #6254:

@alexcrichton Thank you for the fix! :heart:

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 17:57):

cfallin commented on issue #6254:

@remlse we've just seen a possible issue on this while discussing in a meeting; more from @elliottt in a bit...

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 18:48):

elliottt commented on issue #6254:

I think we're reliant on seeing uses before defs for the management of the value_lowered_uses field of the Lower struct. If we put the blocks in a random order, we may see the definition first, and as a result decide that the instruction isn't needed in the final program: https://github.com/bytecodealliance/wasmtime/blob/3b7274fa94c39b9e3984fa207994b60ec827a31b/cranelift/codegen/src/machinst/lower.rs#L708-L709
In the snippet above, is_any_inst_result_needed queries the value_lowered_uses vector for any result value with a number of uses that's greater than zero, and the entry in that vector is only incremented when processing uses of that value.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 18:59):

remlse commented on issue #6254:

I see, then we can't merge this. Is there some other order we could randomize to salvage some of the work? :joy:

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 19:34):

cfallin commented on issue #6254:

Yes, sorry @remlse, I had forgotten about this ordering invariant. (I have no excuse, I wrote this code ~3 years ago!)

Offline Trevor suggested a nice approach to still reorder to an allowed degree, namely: pick some permutation that still respects defs-before-uses. In particular, this loop I think can iterate successors for a block in any order and still generate valid RPO.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 08:33):

remlse commented on issue #6254:

Out of curiosity, have you fuzzed with this (and verified that no issues arise)?

I did a short sanity check yesterday, and a longer (30min) run today without panics. Is there something else I should do to verify there aren't any issues?

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 16:20):

cfallin commented on issue #6254:

Nope, that sounds great -- let's merge, thanks!

Last updated: Nov 22 2024 at 16:03 UTC