wasmtime / PR #6039 Chaos mode MVP: Skip branch optimizat... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #6039 Chaos mode MVP: Skip branch optimizat...

Wasmtime GitHub notifications bot (Mar 17 2023 at 04:31):

remlse opened PR #6039 from fuzz-skip-branch-opt to main:

This is a draft of the MVP for chaos mode (#4134).

It extends the fuzz target cranelift-icache for now, by allowing it to run with the feature chaos enabled. This will pseudo-randomly toggle branch optimization in MachBuffer via the new chaos mode control plane in the crate cranelift-chaos.

Quick command for the documentation:
cargo doc -p cranelift-chaos --document-private-items --open
Running the fuzz target with chaos mode enabled:
cargo fuzz run --no-default-features --features chaos cranelift-icache
Passing a reference counted chaos engine around is not that bad, the diff is less noisy than I would've expected. I'm still planning to make an equivalent POC with private, global, mutable state in the cranelift-chaos crate to get a better idea of the trade-offs.

Note that because of this zulip topic, I didn't bump the version of arbitrary in this PR to keep those issues isolated. Once that's resolved, we think it's probably a good idea to update arbitrary while we're working with it.

I've added a couple print statements during development, and it seems the branch optimization is more often carried out than skipped. I guess this is consistent with libfuzzer's goal of generating data in a way that code coverage is maximized.

I also ran into a crash while running this fuzz target. The crash happens at cranelift-icache.rs:220:
if expect_cache_hit {
    let after_mutation_result_from_cache = icache::try_finish_recompile(&func, &serialized)
        .expect("recompilation should always work for identity");
    assert_eq!(*after_mutation_result, after_mutation_result_from_cache); // <-- this assert fails
Maybe someone has an intuition along the lines of: "Oh yes, of course that will fail when branch optimization is randomly skipped", or similar? In any case, I'll investigate to see if the panic is caused by my changes or something different.

Questions

[ ] I needed to use Arc and Mutex instead of Rc and RefCell in the control plane, because the compiler was complaining about the Send trait not being implemented. So if Cranelift runs in parallel, won't that interfere with our plans with the fuel parameter? If fuel from the chaos engine is requested in a different order every time, we won't be able to deterministically reproduce bugs and pinpoint their origin.

[ ] Did I get the "paperwork" right?
- version 0.95.0 like other cranelift crates
- license
- Cargo.toml
- ... etc. ?

[ ] There are a several ChaosEngine::todo()s in the wild. Is it ok to merge these in principle or should we find a different solution for adding the chaos engine everywhere incrementally?

Todos

[ ] Add appropriate explanations to the commit messages

[ ] Investigate crash (Base64: Av////////8AAAIAAAAAAAD5jIyMjAAKAAAAAPHx8fERDgcAAAAAAJkBAAAAAAAAKwBp/5r//wAAAAAAAAAHbS45azEAAAAACF0=)

[ ] Exend existing fuzzing documentation with an overview of chaos mode as well as how to run a target with chaos mode enabled (cargo fuzz run --features chaos $TARGET)

Wasmtime GitHub notifications bot (Mar 18 2023 at 18:01):

iximeow submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2023 at 18:01):

iximeow created PR review comment:

    /// # Panics
(hi, i was interested in the Unstructured conversation and noticed this)

Wasmtime GitHub notifications bot (Mar 18 2023 at 18:18):

iximeow submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2023 at 18:18):

iximeow created PR review comment:

from a safety perspective, the other extremely important detail of this trick is that ChaosEngineData must also never move once references to engine.data are taken. so the "this must not move"-ness of data kind of percolates through to any enclosing type until it's somewhere that won't move (which works out here because ChaosEngineData ends up owned by an Arc where it oughtn't be moved out of.

as an example that certainly won't come up here but would be Technically Possible, Arc::new(some_chaos_engine.data.try_unwrap()) would yield a ChaosEngineData whose unstructured points to somewhere else, and would (hopefully! :D) fault on use.

Wasmtime GitHub notifications bot (Mar 18 2023 at 18:19):

iximeow edited PR review comment.

Wasmtime GitHub notifications bot (Mar 18 2023 at 22:43):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2023 at 22:43):

remlse created PR review comment:

Thanks for pointing that out! Does it also apply if some type is heap allocated? I think the article I got this from used a Box to create a level of indirection. The idea being that if the Box itself is moved, the values on the heap won't. So any existing references into that heap allocation would still be valid. In this case, the Vec is supposed to serves the same purpose as the Box in the article.

That being said, I just noticed that I got the order of the fields wrong, which the article warns against. data will be dropped before unstructured, which creates a dangling pointer and UB.(?) oops :smile: I definitely prefer a safe solution as well.

Wasmtime GitHub notifications bot (Mar 18 2023 at 22:56):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2023 at 22:56):

remlse created PR review comment:

Aaand reading a bit further, I also forgot the thing about AliasableBox so you're definitely right, moving data would also be UB.

Wasmtime GitHub notifications bot (Mar 18 2023 at 23:28):

remlse updated PR #6039 from fuzz-skip-branch-opt to main.

Wasmtime GitHub notifications bot (Mar 18 2023 at 23:29):

remlse updated PR #6039 from fuzz-skip-branch-opt to main.

Wasmtime GitHub notifications bot (Mar 18 2023 at 23:30):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2023 at 23:30):

remlse created PR review comment:

Thanks! fixed it.

Wasmtime GitHub notifications bot (Mar 18 2023 at 23:53):

iximeow submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2023 at 23:53):

iximeow created PR review comment:

yeah that's the part that makes the blog post's solution a little more robust to moves - a &AliasableBox<ZipArchive<File>> could be made to dangle, but with private internals you can ensure that wouldn't happen. anyway, hopefully threading a &mut ControlPlane through the compiler as appropriate lets you avoid the whole construction, and double-hopefully the extra arguments don't affect compile time all that much :)

Wasmtime GitHub notifications bot (Mar 19 2023 at 09:38):

remlse updated PR #6039 from fuzz-skip-branch-opt to main.

Wasmtime GitHub notifications bot (Mar 19 2023 at 11:01):

remlse edited PR #6039 from fuzz-skip-branch-opt to main:

This is a draft of the MVP for chaos mode (#4134).

It extends the fuzz target cranelift-icache for now, by allowing it to run with the feature chaos enabled. This will pseudo-randomly toggle branch optimization in MachBuffer via the new chaos mode control plane in the crate cranelift-chaos.

Quick command for the documentation:
cargo doc -p cranelift-chaos --document-private-items --open
Running the fuzz target with chaos mode enabled:
cargo fuzz run --no-default-features --features chaos cranelift-icache
Passing a reference counted chaos engine around is not that bad, the diff is less noisy than I would've expected. I'm still planning to make an equivalent POC with private, global, mutable state in the cranelift-chaos crate to get a better idea of the trade-offs.

Note that because of this zulip topic, I didn't bump the version of arbitrary in this PR to keep those issues isolated. Once that's resolved, we think it's probably a good idea to update arbitrary while we're working with it.

I've added a couple print statements during development, and it seems the branch optimization is more often carried out than skipped. I guess this is consistent with libfuzzer's goal of generating data in a way that code coverage is maximized.

I also ran into a crash while running this fuzz target. The crash happens at cranelift-icache.rs:220:
if expect_cache_hit {
    let after_mutation_result_from_cache = icache::try_finish_recompile(&func, &serialized)
        .expect("recompilation should always work for identity");
    assert_eq!(*after_mutation_result, after_mutation_result_from_cache); // <-- this assert fails
Maybe someone has an intuition along the lines of: "Oh yes, of course that will fail when branch optimization is randomly skipped", or similar? In any case, I'll investigate to see if the panic is caused by my changes or something different.

Questions

[x] I needed to use Arc and Mutex instead of Rc and RefCell in the control plane, because the compiler was complaining about the Send trait not being implemented. So if Cranelift runs in parallel, won't that interfere with our plans with the fuel parameter? If fuel from the chaos engine is requested in a different order every time, we won't be able to deterministically reproduce bugs and pinpoint their origin.
-> answer: Arc and Mutex must not be used.

[ ] Did I get the "paperwork" right?
- version 0.95.0 like other cranelift crates
- license
- Cargo.toml
- ... etc. ?

[ ] There are a several ChaosEngine::todo()s in the wild. Is it ok to merge these in principle or should we find a different solution for adding the chaos engine everywhere incrementally?

Todos

[ ] Add appropriate explanations to the commit messages

[x] Investigate crash (Base64: Av////////8AAAIAAAAAAAD5jIyMjAAKAAAAAPHx8fERDgcAAAAAAJkBAAAAAAAAKwBp/5r//wAAAAAAAAAHbS45azEAAAAACF0=)
-> most likely due to usage of Arc and Mutex

[ ] Extend existing fuzzing documentation with an overview of chaos mode as well as how to run a target with chaos mode enabled (cargo fuzz run --features chaos $TARGET)

Wasmtime GitHub notifications bot (Mar 22 2023 at 14:11):

remlse updated PR #6039 from fuzz-skip-branch-opt to main.

Wasmtime GitHub notifications bot (Mar 23 2023 at 16:23):

remlse updated PR #6039 from fuzz-skip-branch-opt to main.

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin created PR review comment:

ctrl_plane can probably go inside the state (EmitState)?

The issue with lifetimes that this would otherwise create (&mut ControlPlane inside of the struct) can be resolved I think by std::mem::move to take ownership of the control plane temporarily in places where we emit.

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin created PR review comment:

(remove debugging printlns before merging)

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin created PR review comment:

outdated comment?

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin created PR review comment:

I think it's probably better to pass in the control-plane state with each call to compile; the CompilerContext is otherwise not that semantically meaningful (meant to enable reuse).

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin created PR review comment:

Let's remove this is_noop mechanism before merging.

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin created PR review comment:

I think this body makes sense as the Default impl (an empty ControlPlane should have no affect on Cranelift's behavior as it is today -- this also implies how to use bools, i.e. false should make no change).

Wasmtime GitHub notifications bot (Mar 31 2023 at 16:50):

cfallin created PR review comment:

I would return just bool (use .unwrap_or(false) on the pop).

Wasmtime GitHub notifications bot (Apr 03 2023 at 14:27):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 15:00):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 16:07):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 16:10):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Apr 03 2023 at 16:10):

remlse created PR review comment:

Not sure if it's OK to export MachInstEmitState.

I needed it here.

Wasmtime GitHub notifications bot (Apr 03 2023 at 16:13):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Apr 03 2023 at 16:13):

remlse created PR review comment:

I didn't combine these in a Vec<(Function, ControlPlane)>, because it makes the diff a little cleaner and in the manual arbitrary implementation it can still be controlled that the two vectors have the same size.

Wasmtime GitHub notifications bot (Apr 03 2023 at 16:19):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Apr 03 2023 at 16:19):

remlse created PR review comment:

If this is forgotten, the control plane would just silently be the default one for the rest of the compilation. I guess it should be fine, (for now) it seems like this is the only place where one has to remember to move the control plane back out of the emit state.

Wasmtime GitHub notifications bot (Apr 03 2023 at 17:31):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:08):

remlse requested cfallin for a review on PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:11):

remlse edited PR #6039:

This is a draft of the MVP for chaos mode (#4134).

Edit: The implemented fuzz target changed to cranelift-fuzzgen.

It extends the fuzz target cranelift-icache for now, by allowing it to run with the feature chaos enabled. This will pseudo-randomly toggle branch optimization in MachBuffer via the new chaos mode control plane in the crate cranelift-chaos.

Quick command for the documentation:
cargo doc -p cranelift-chaos --document-private-items --open
Running the fuzz target with chaos mode enabled:
cargo fuzz run --no-default-features --features chaos cranelift-icache
Passing a reference counted chaos engine around is not that bad, the diff is less noisy than I would've expected. I'm still planning to make an equivalent POC with private, global, mutable state in the cranelift-chaos crate to get a better idea of the trade-offs.

Note that because of this zulip topic, I didn't bump the version of arbitrary in this PR to keep those issues isolated. Once that's resolved, we think it's probably a good idea to update arbitrary while we're working with it.

I've added a couple print statements during development, and it seems the branch optimization is more often carried out than skipped. I guess this is consistent with libfuzzer's goal of generating data in a way that code coverage is maximized.

I also ran into a crash while running this fuzz target. The crash happens at cranelift-icache.rs:220:
if expect_cache_hit {
    let after_mutation_result_from_cache = icache::try_finish_recompile(&func, &serialized)
        .expect("recompilation should always work for identity");
    assert_eq!(*after_mutation_result, after_mutation_result_from_cache); // <-- this assert fails
Maybe someone has an intuition along the lines of: "Oh yes, of course that will fail when branch optimization is randomly skipped", or similar? In any case, I'll investigate to see if the panic is caused by my changes or something different.

Questions

[x] I needed to use Arc and Mutex instead of Rc and RefCell in the control plane, because the compiler was complaining about the Send trait not being implemented. So if Cranelift runs in parallel, won't that interfere with our plans with the fuel parameter? If fuel from the chaos engine is requested in a different order every time, we won't be able to deterministically reproduce bugs and pinpoint their origin.
-> answer: Arc and Mutex must not be used.

[ ] Did I get the "paperwork" right?
- version 0.95.0 like other cranelift crates
- license
- Cargo.toml
- ... etc. ?

[x] There are a several ChaosEngine::todo()s in the wild. Is it ok to merge these in principle or should we find a different solution for adding the chaos engine everywhere incrementally? -> these have been removed

Todos

[ ] Add appropriate explanations to the commit messages

[x] Investigate crash (Base64: Av////////8AAAIAAAAAAAD5jIyMjAAKAAAAAPHx8fERDgcAAAAAAJkBAAAAAAAAKwBp/5r//wAAAAAAAAAHbS45azEAAAAACF0=)
-> most likely due to usage of Arc and Mutex

[ ] Extend existing fuzzing documentation with an overview of chaos mode as well as how to run a target with chaos mode enabled (cargo fuzz run --features chaos $TARGET)

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:20):

remlse has marked PR #6039 as ready for review.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:20):

remlse requested elliottt for a review on PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:20):

remlse requested wasmtime-fuzz-reviewers for a review on PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:20):

remlse requested wasmtime-compiler-reviewers for a review on PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:20):

remlse requested pchickey for a review on PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:20):

remlse requested wasmtime-core-reviewers for a review on PR #6039.

Wasmtime GitHub notifications bot (Apr 03 2023 at 18:20):

remlse requested wasmtime-default-reviewers for a review on PR #6039.

Wasmtime GitHub notifications bot (Apr 04 2023 at 17:51):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 17:51):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 17:51):

cfallin created PR review comment:

One small tweak to the conditional-compilation strategy: I had been thinking that we could have the methods that produce decisions, like get_decision here, return a default value (false here) as a constant in the non-chaos-feature case; then the sites where we use these decisions, like in MachBuffer, don't require annotation with conditional compilation. What do you think?

Wasmtime GitHub notifications bot (Apr 04 2023 at 17:51):

cfallin created PR review comment:

A few things:

Usually a mut-accessor will be named like fn ctrl_plane_mut(&mut self) -> ...

Let's have a different one too, fn take_ctrl_plane(self), that consumes the emit-state and gives us back the control-plane state

Wasmtime GitHub notifications bot (Apr 04 2023 at 17:51):

cfallin created PR review comment:

Yeah, we may be able to find a better way here eventually, but I think this strikes a good balance for now.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:26):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:26):

remlse created PR review comment:

I'm thinking about the potential performance impact in release builds, but I guess it's safe to assume the compiler inlines a constant false and removes the resulting if false {}.

In my view, it would be a nice aspect of the control plane that there is no way to (mis-)use it in regular builds. But I guess that every control plane API needs to have some default output value anyway... and that can probably always be inlined as well? And we can annotate these default-returning functions with #[inline].

What is the downside of conditional compilation at the call sites? It seemed like an easy way to be really, really sure nothing bad happens in release builds, but on second thought, it doesn't seem necessary.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:37):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:37):

cfallin created PR review comment:

Indeed, we should be able to trust the branch-folding here.

What is the downside of conditional compilation at the call sites?

The main downside is that it spreads the implementation of a conditional decision across distributed points -- the alternative, where everything is wired to a single module where all conditional-compilation logic lies, makes it easier to make changes in the future. (Another example of this principle in action is the memfd pooling-allocator mechanism in Wasmtime: when I implemented this in #3697 last year I originally had feature-conditional code in many places, but Alex convinced me to centralize everything into two versions of one module and remove conditionals everywhere else. The result is far cleaner!)

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:42):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:42):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 18:42):

remlse created PR review comment:

fn take_ctrl_plane(self)

I think that actually caught a mistake. I was taking the control plane out of the emission state inside a loop, where later loop iterations would use the state with a now-empty control plane.

Wasmtime GitHub notifications bot (Apr 04 2023 at 19:50):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 04 2023 at 19:55):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2023 at 19:55):

remlse created PR review comment:

I'm assuming this goes for the Arbitrary implementation as well, so I removed the conditional compilation here too.

The shim control plane's Arbitrary implementation now returns the default without consuming any bytes.

Wasmtime GitHub notifications bot (Apr 04 2023 at 21:28):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 16:23):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 05 2023 at 16:41):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 16:41):

bjorn3 created PR review comment:

I don't think define_function should get this argument. If you need this fine control you should probably use define_function_bytes instead. This doesn't work with a module that serializes functions rather than immediately compiles them and it is confusing for most users of cranelift.

Wasmtime GitHub notifications bot (Apr 05 2023 at 17:11):

remlse submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 17:11):

remlse created PR review comment:

I think this is the place we actually needed that argument. So that would have to be rewritten with define_function_bytes. The module there is a JITModule and its define_function and define_function_bytes methods are not trivial, so it's not obvious to me how to do that.

Wasmtime GitHub notifications bot (Apr 05 2023 at 17:16):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 17:16):

bjorn3 created PR review comment:

cranelift-object implements define_function as ctx.compile_and_emit(self.isa(), &mut code, ctrl_plane) followed by define_function_bytes. You could do the same in TestFileCompiler.

Wasmtime GitHub notifications bot (Apr 05 2023 at 17:18):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 17:18):

cfallin created PR review comment:

@bjorn3 in general the approach we've been taking is to thread through the control-plane everywhere compilation can be invoked; conceptually it's now another input along with the CLIF. (It does have a Default implementation.) If there's a way to rename this variant to a third option, and then have a variant that uses a default control plane, we can perhaps do that. Would you be willing to do that in a followup PR?

Wasmtime GitHub notifications bot (Apr 05 2023 at 17:44):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 05 2023 at 18:06):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 05 2023 at 18:10):

bjorn3 created PR review comment:

From a usability perspective having another method would work. But when serializing rather than compiling, a ControlPlane argument doesn't really make any sense as you can't serialize ControlPlane. (I have local changes to make a serializing Module which I want to upstream. I'm using it to allow using cranelift-interpreter in cg_clif with minimal changes to cg_clif.)

Wasmtime GitHub notifications bot (Apr 05 2023 at 18:10):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 18:31):

remlse updated PR #6039.

Wasmtime GitHub notifications bot (Apr 05 2023 at 19:28):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 19:28):

cfallin created PR review comment:

I'm not sure I understand why serialization of modules implies the need to serialize a ControlPlane -- it is given as an argument, it isn't stored -- but please do create an issue or PR with a fix if you have one in mind. In the meantime I'll go ahead and merge this PR (which has been under review for a while and we have general consensus on).

Wasmtime GitHub notifications bot (Apr 05 2023 at 19:33):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 05 2023 at 19:33):

bjorn3 created PR review comment:

If the passed in ControlPlane should affect the eventual compilation of the function, it did need to be serialized. If not, there it doesn't really make much sense to pass in ControlPlane.

In the meantime I'll go ahead and merge this PR (which has been under review for a while and we have general consensus on).

:+1:

Wasmtime GitHub notifications bot (Apr 05 2023 at 20:02):

cfallin merged PR #6039.

Last updated: Nov 22 2024 at 16:03 UTC