Revert "Add --cfg fuzzing_repro when reproducing a crash"

[alexcrichton]

Updated Description

This PR now reverts #344 to avoid build cache thrashing when switching between reproducing a fuzz test case and when fuzzing in general.

Original Description

Changing RUSTFLAGS causes a recompile of the entire project so for projects that are expensive to build this option being enabled by default means that workflows which switch back-and-forth between fuzzing and running individual tests generate a full rebuild every time. This adds a --no-cfg-fuzzing-repro option on the cargo fuzz run CLI to disable this --cfg argument from being passed.

[fitzgen]

I've also been annoyed by recompilations ever since #344 landed.

I think requiring a recompile of the whole fuzz target is perhaps not the right default. I'd be open to changing cfg(fuzzing_repro) from the default to an option. But I'd like to hear what @Manishearth and @mgeisler think about that.

[fitzgen]

(Basically, I didn't fully think through the recompilation repercussions when reviewing #344, so I think it is worth revisiting that choice now that we are aware of them.)

[mgeisler]

I think requiring a recompile of the whole fuzz target is perhaps not the right default. I'd be open to changing cfg(fuzzing_repro) from the default to an option. But I'd like to hear what @Manishearth and @mgeisler think about that.

I see what you mean: turning the flag off by default would be less invasive.

However, @alexcrichton, don't you have the same problem with the #[cfg(fuzzing)] option when flipping between cargo test and cargo fuzz?

[alexcrichton]

Ah that's not an issue for me since I personally rarely test when using cargo fuzz. If I do then I do cargo test and cargo +nightly fuzz and there's no overlap as two different compilers are used.

When fuzzing though i do very frequently switch between "run the fuzzer for a bit" and then "rerun each failing test case to debug"

[Manishearth]

I agree, I was a little worried about this when the original thing landed and I think it's better to go back to the original default.

[mgeisler]

Ah that's not an issue for me since I personally rarely test when using cargo fuzz. If I do then I do cargo test and cargo +nightly fuzz and there's no overlap as two different compilers are used.

Ah, smart 😄 Does this not suggest another work-around for you: use cargo +stable test for your testing, cargo +nightly fuzz for the general fuzzing and cargo +beta fuzz run fuzz_target artifact for the run that reproduces a failure.

Of course, this is just a silly way of getting Cargo to cache the build output across all three kinds of invocations. From a quick skim of Build cache, it seems using a separate Cargo profile for the reproducing run would be useful for you?

I agree, I was a little worried about this when the original thing landed and I think it's better to go back to the original default.

Taking this out again is of course an option — but it suggests that the cfg(fuzzing) should also go, no? It has the same compilation overhead, from what I understand. But perhaps it's more acceptable because the cost is paid at different times?

I do see a good reason for keeping fuzzing as a command line flag (instead of letting people set it by hand in RUSTFLAGS): it establishes a standard where libraries can use fuzzing to fix random seeds and so on. The fuzzing_repro flag is more for ad-hoc exploration so it doesn't matter so much.

[alexcrichton]

Personally this PR is not going to be a great solution for me. I'll probably always forget to pass --no-cfg-fuzzing-repro and by the time I remember it it's too late. I would personally prefer to not pass cfg(fuzzing_repro) by default. In fuzzing I've done I've used log::debug! or log::log_enabled! to print/display expensive things, I've never wanted to use #[cfg(fuzzing_repro)] as an entirely separate compilation. That's all to say, I'm also not a fan of this PR and I would prefer to go back to the prior default.

That being said, I'm happy to defer whwat y'all feel is best. I don't want to presume one way's the best over another. I'll figure out how to make this manageable in my workflow no matter what the end solution is. As-is today works find if I set a different target directory.

and cargo +beta fuzz run fuzz_target artifact for the run that reproduces a failure.

Alas I don't believe this works because fuzzing only works on nightly, not on beta.

it seems using a separate Cargo profile for the reproducing run would be useful for you?

Indeed! It's what I'm doing now. Personally I'd rather not have to do this either.

it suggests that the cfg(fuzzing) should also go, no?

I've personally used this before and found it useful, but I also use it very rarely as I typically don't want the code I'm testing/running to be that different from what I'm fuzzing.

[mgeisler]

Personally this PR is not going to be a great solution for me. I'll probably always forget to pass --no-cfg-fuzzing-repro and by the time I remember it it's too late. I would personally prefer to not pass cfg(fuzzing_repro) by default.

Just to be clear, I'm fine with removing the flag again since it has poor interaction with a typical workflow.

In fuzzing I've done I've used log::debug! or log::log_enabled! to print/display expensive things, I've never wanted to use #[cfg(fuzzing_repro)] as an entirely separate compilation.

Makes sense! My experience has been the opposite, as in I typically don't have logging available in my crates — but this is a good approach which I could adapt.

[Manishearth]

Yeah I think we should move forward with changing the default back to what it was and adding an opt in flag for this mode.

[alexcrichton]

Ok I've updated this PR to being a revert of #344 instead of adding a new CLI flag. (under the assumption that this is still possible with RUSTFLAGS).

as in I typically don't have logging available in my crates

Oh I do also want to be clear I'm not necessarily advocating that this is the "one true way" to do so. I've also use things like env::var("....") in the past to help control fuzzing runtime which I've found works pretty well too, for example FUZZ_PRINT_HUGE_DEBUG_REPR cargo +nightly fuzz run foo ./test-case or something like that.

[fitzgen]

Thanks for the discussion everyone.

[mgeisler]

(under the assumption that this is still possible with RUSTFLAGS).

Sounds great, I'll be trying out using a cargo profile for this next time I run into this — I don't think I knew that I could make my own profiles back when I made the PR 😄

as in I typically don't have logging available in my crates

Oh I do also want to be clear I'm not necessarily advocating that this is the "one true way" to do so. I've also use things like env::var("....") in the past to help control fuzzing runtime

Thanks, makes a lot of sense! It's good that you mentioned logging — I think I should look into using it more for my own projects... since it is a very good tool 🙂

[emaxx-google]

Hello, should the Rust Fuzz Book be updated to remove the mention of the config flag? https://rust-fuzz.github.io/book/cargo-fuzz/guide.html

[fitzgen]

@emaxx-google good catch! Would you be up for sending a PR to https://github.com/rust-fuzz/book ?

[emaxx-google]

@emaxx-google good catch! Would you be up for sending a PR to https://github.com/rust-fuzz/book ?

Sure. I'm just unsure if the discussions settled on any specific alternative that'd supersede this deleted functionality, or if anyone is working on one.

[fitzgen]

No one is working on anything, AFAIK.