Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fuzz: cleanup control plane
Co-authored-by: Falk Zwimpfer <[email protected]>
Co-authored-by: Moritz Waser <[email protected]>
  • Loading branch information
3 people committed Apr 3, 2023
commit 14ba45d8ecb955ec6e1d0ba54626d53a9ab6fc6d
5 changes: 1 addition & 4 deletions cranelift/codegen/src/machinst/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -776,10 +776,7 @@ impl<I: VCodeInst> MachBuffer<I> {

fn optimize_branches(&mut self, ctrl_plane: &mut ControlPlane) {
#[cfg(feature = "chaos")]
if let Some(true) = ctrl_plane.get_decision() {
println!("");
println!("");
println!("branch optimizations skipped");
if ctrl_plane.get_decision() {
return;
}

Expand Down
54 changes: 12 additions & 42 deletions cranelift/control/src/chaos.rs
Original file line number Diff line number Diff line change
@@ -1,58 +1,28 @@
use std::{backtrace, fmt::Debug};

use arbitrary::Arbitrary;

/// The control plane of chaos mode.
/// Please see the [crate-level documentation](crate).
///
/// **Clone liberally!** The chaos engine is reference counted.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, Default)]
pub struct ControlPlane {
data: Vec<bool>,
is_noop: bool,
}

impl Arbitrary<'_> for ControlPlane {
fn arbitrary<'a>(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {
Ok(Self::new(u.arbitrary()?, false))
}
}

impl ControlPlane {
fn new(data: Vec<bool>, is_noop: bool) -> Self {
Self {
data: data,
is_noop,
}
}

/// TODO chaos: should be explained
pub fn no_chaos() -> Self {
Self::new(Vec::new(), false)
}

pub fn todo() -> Self {
Self::new(Vec::new(), false)
}
}

impl Default for ControlPlane {
fn default() -> Self {
Self::new(Vec::new(), true)
Ok(Self {
data: u.arbitrary()?,
})
}
}

impl ControlPlane {
pub fn get_decision(&mut self) -> Option<bool> {
if self.is_noop {
//println!(
// "try to get a decision from a noop chaos engine at {} ",
// backtrace::Backtrace::force_capture()
//);
//None
panic!("trying to get a decision from a noop chaos engine");
} else {
self.data.pop()
}
/// Returns a pseudo-random boolean if the control plane was constructed
/// with `arbitrary`.
///
/// The default value `false` will always be returned if the
/// pseudo-random data is exhausted or the control plane was constructed
/// with `default`.
pub fn get_decision(&mut self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.data.pop().unwrap_or_default()
}
}
12 changes: 4 additions & 8 deletions cranelift/control/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,13 @@
//! e.g. manipulate heuristic optimizations, clobber undefined register bits
//! etc.
//!
//! There are three ways to acquire a [ControlPlane]:
//! There are two ways to acquire a [ControlPlane]:
//! - [arbitrary] for the real deal
//! - [noop] for the zero-sized type when `chaos` is disabled
//! - [todo] for stubbing out code paths during development
//!
//! The reason both [noop] and [todo] exist is so that [todo] can easily
//! be searched for and removed later.
//! - [default] for an "empty" control plane which always returns default
//! values
//!
//! [arbitrary]: ControlPlane#method.arbitrary
//! [noop]: ControlPlane::noop
//! [todo]: ControlPlane::todo
//! [default]: ControlPlane#method.default

#[cfg(not(any(feature = "chaos", doc)))]
mod zero_sized;
Expand Down
19 changes: 2 additions & 17 deletions cranelift/control/src/zero_sized.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,5 @@
#[derive(Debug, Clone)]
#[derive(Debug, Clone, Default)]
pub struct ControlPlane {
/// prevent direct instantiation (use `noop` or `todo` instead)
/// prevent direct instantiation (use `default` instead)
_private: (),
}

impl ControlPlane {
pub fn no_chaos() -> Self {
Self { _private: () }
}

// get_decision function is not implemented here because it should only be called within
// conditional compiled code blocks. In that case the other implementation would be used anyway.
}

impl Default for ControlPlane {
fn default() -> Self {
Self::no_chaos()
}
}