Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replace the --release flag with a generic --profile flag #163

Merged
merged 8 commits into from
Sep 5, 2023
Merged

feat: replace the --release flag with a generic --profile flag #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e677c04
feat: replace the --release flag with a generic --profile flag
Ekleog Jul 14, 2023
b85421b
fix: fix tests
Ekleog Jul 14, 2023
d9df5dd
fix: inherit fuzz profile from debug rather than release
Ekleog-NEAR Aug 2, 2023
ddf745b
fix: make error message search-able
Ekleog-NEAR Aug 2, 2023
9f14dc0
fix typo
Ekleog-NEAR Aug 11, 2023
7df3de7
add fuzz profile to examples
Ekleog-NEAR Aug 14, 2023
7ff5f4c
add fuzz profile to bolero workspace
Ekleog-NEAR Aug 14, 2023
2617b95
use the fuzz profile for running
Ekleog-NEAR Sep 5, 2023
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
6 changes: 6 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ members = [
"cargo-bolero",
]

[profile.fuzz]
inherits = "dev"
opt-level = 3
incremental = false
codegen-units = 1

[profile.release]
lto = true
codegen-units = 1
Expand Down
15 changes: 0 additions & 15 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,13 @@ libfuzzer honggfuzz:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER)
@cargo run \
--features $@ \
reduce \
fuzz_bytes \
--manifest-path examples/basic/Cargo.toml \
--engine $@ \
--release \
--sanitizer $(SANITIZER)
@cargo run \
--features $@ \
Expand All @@ -68,15 +66,13 @@ libfuzzer honggfuzz:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release true \
--sanitizer $(SANITIZER)
@cargo run \
--features $@ \
reduce \
fuzz_generator \
--manifest-path examples/basic/Cargo.toml \
--engine $@ \
--release \
--sanitizer $(SANITIZER)
@cargo run \
--features $@ \
Expand All @@ -85,15 +81,13 @@ libfuzzer honggfuzz:
--manifest-path examples/basic/Cargo.toml \
--runs 1000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER)
@cargo run \
--features $@ \
reduce \
fuzz_operations \
--manifest-path examples/basic/Cargo.toml \
--engine $@ \
--release \
--sanitizer $(SANITIZER)
@SHOULD_PANIC=1 cargo run \
--features $@ \
Expand All @@ -102,7 +96,6 @@ libfuzzer honggfuzz:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER) \
|| true # TODO make this consistent
@SHOULD_PANIC=1 cargo run \
Expand All @@ -111,7 +104,6 @@ libfuzzer honggfuzz:
tests::add_test \
--manifest-path examples/basic/Cargo.toml \
--engine $@ \
--release \
--sanitizer $(SANITIZER) \
|| true # TODO make this consistent
@SHOULD_PANIC=1 cargo run \
Expand All @@ -121,7 +113,6 @@ libfuzzer honggfuzz:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER) \
|| true # TODO make this consistent
@SHOULD_PANIC=1 cargo run \
Expand All @@ -130,7 +121,6 @@ libfuzzer honggfuzz:
tests::other_test \
--manifest-path examples/basic/Cargo.toml \
--engine $@ \
--release \
--sanitizer $(SANITIZER) \
|| true # TODO make this consistent
@SHOULD_PANIC=1 cargo run \
Expand All @@ -140,7 +130,6 @@ libfuzzer honggfuzz:
--manifest-path examples/basic/Cargo.toml \
--runs 1000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER) \
|| true # TODO make this consistent

Expand All @@ -152,7 +141,6 @@ afl:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER)
@cargo run \
--features $@ \
Expand All @@ -161,7 +149,6 @@ afl:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER)
@rm -rf examples/basic/src/__fuzz__
@SHOULD_PANIC=1 cargo run \
Expand All @@ -171,7 +158,6 @@ afl:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER) \
&& exit 1 || true
@rm -rf examples/basic/src/__fuzz__
Expand All @@ -182,7 +168,6 @@ afl:
--manifest-path examples/basic/Cargo.toml \
--runs 100000 \
--engine $@ \
--release \
--sanitizer $(SANITIZER) \
&& exit 1 || true

Expand Down
14 changes: 14 additions & 0 deletions book/src/library-installation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ bolero = "0.8"
```
to `Cargo.toml`.

Then, create the `fuzz` profile: (Note that LTO is not well-supported for the fuzzing profile)
```toml
[profile.fuzz]
inherits = "dev"
opt-level = 3
incremental = false
codegen-units = 1
```

If you forget adding the profile, then you will get the following error:
```
error: profile `fuzz` is not defined
```

## Structured Test Generation

If your crate wishes to implement structured test generation on public data structures, `bolero-generator` can be added to the main dependencies:
Expand Down
27 changes: 6 additions & 21 deletions cargo-bolero/src/project.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ pub struct Project {
#[structopt(long)]
all_features: bool,

/// Build artifacts in release mode, with optimizations [default: true]
#[structopt(long)]
release: Option<Option<bool>>,
/// Build artifacts in release mode, with optimizations [default: "fuzz"]
///
/// Note that if you do not have `codegen-units = 1` in the profile, there are known compilation bugs
#[structopt(long, default_value = "fuzz")]
Copy link
Owner

@camshaft camshaft Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we default to a profile that is there by default? My worry is this will make the UI worse and just complain about a missing profile with no actions on how to fix it for bolero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my thinking was, it’s better to fail loudly than to fail silently.

If we default to release, then we open ourselves up to the LTO issue you mentioned above, and there won’t be debug-assertions. If we default to debug, then the fuzzer will be way too slow for reasonable running.

Right now, the error message for people who didn’t read through the book would be:

error: profile `fuzz` is not defined

I think it’s reasonable that the user would search the internet with this error message, and fall on the library-installation.md page (to which I just added this error message, to make sure it’s easy to find).

We could also add a log message at the beginning of each cargo-bolero invocation, reminding to read the book if it fails with this error message, but that’d feel overkill to me.

WDYT?

Copy link
Owner

Choose a reason for hiding this comment

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

Ideally the user wouldn't have to search to figure out what's wrong. Like, rustc's error messages usually provide all of the information you need to fix it inline. That being said, I do think profiles are a better way to go, especially considering the issues with LTO.

Can you open an issue to track possible solutions to improve the UX for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just did it! See #168 :)

profile: String,

/// Do not activate the `default` feature
#[structopt(long)]
Expand Down Expand Up @@ -88,10 +90,7 @@ impl Project {
let mut cmd = self.cargo();

cmd.arg(call).arg("--target").arg(&self.target);

if self.release() {
cmd.arg("--release");
}
cmd.arg("--profile").arg(&self.profile);

if self.no_default_features {
cmd.arg("--no-default-features");
Expand Down Expand Up @@ -187,12 +186,6 @@ impl Project {
} else {
None
})
// https://github.com/rust-lang/rust/issues/47071
.chain(if self.release() {
Some("-Ccodegen-units=1")
} else {
None
})
.map(String::from)
.chain(self.sanitizer_flags())
.chain(std::env::var(inherits).ok())
Expand All @@ -212,12 +205,4 @@ impl Project {
self.sanitizers()
.map(|sanitizer| format!("-Zsanitizer={}", sanitizer))
}

fn release(&self) -> bool {
match self.release {
None => true,
Some(None) => true,
Some(Some(v)) => v,
}
}
}
6 changes: 6 additions & 0 deletions examples/basic/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,9 @@ harness = false

[profile.bench]
debug = true

[profile.fuzz]
inherits = "dev"
opt-level = 3
incremental = false
codegen-units = 1
6 changes: 6 additions & 0 deletions examples/workspace/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,9 @@ members = ["crate_a", "crate_b"]

[profile.bench]
debug = true

[profile.fuzz]
inherits = "dev"
opt-level = 3
incremental = false
codegen-units = 1