Set LLVM Flag to Disable cmov (select instr) generation

[DanBlackwell]

Hi, I work on fuzzing and I've found this flag to be useful as it breaks up conditional checks, giving coverage feedback after each check in conditionals with &&s (by disabling generation of cmov instructions - aka SelectInst in LLVM API) . As you can imagine, having some checkpoints along the way helps the fuzzer break through complex checks. For example:

if x > 5 && y < 6 && z > 9 {
  // coverage feedback 1 set here
  do_stuff();
}

becomes equivalent to this:

if x > 5 {
  // coverage feedback 1 set here
  if y < 6 {
    // coverage feedback 2 set here
    if z > 9 {
      // coverage feedback 3 set here
      do_stuff();
    }
  }
}

This link shows an example in godbolt (notice how with this flag there are 3 coverage feedback entries in the generated assembly): example

Hope this helps!

[nagisa]

This seems like a reasonable thing to do, but I haven't reviewed if this flag is the best way to go about achieving the stated goal (do you have any references we could read, even if its to LLVM source code?)

[DanBlackwell]

I don't have any proof that it's the simplest way to achieve it, but essentially it's just setting a flag in the SimplifyCFGPass
documented here. It basically just forces LLVM to never make the transformation that produces the SelectInsts (it does that here). I can't imagine an easier way to do this than just disabling the pass (well, short of disabling all optimisation - but that seems overkill).

Sometimes it helps, sometimes it doesn't; but from what I've seen it never hurts the fuzzer.

EDIT: perhaps this can be hidden behind a flag until you are certain that it's good?

[fitzgen]

Is this something that we can set even when we aren't on nightly? #376 is preparing us for when sanitizers are stable in rustc. Although I suspect that if this new LLVM flag is nightly-only then the rest of them are as well.

---

I'd be fine merging this if we add a CLI flag like

--disable-branch-folding=true|false

with true being the default. Ideally this would take an Option<bool> in the clap configuration and then we can set the default to whatever we want given the rest of the config and context (e.g. we might disable it by default when running on non-nightly).

[Shnatsel]

Yes, passing LLVM flags directly works on stable. In fact, the godbolt example uses the stable compiler without ever passing the -Z flag for any sanitizer.

My only concern here is potential performance degradation, but it shouldn't result in additional loads or stores, so it shouldn't run into extreme slowdowns from sanitizers, and degrading performance by a single digit percentage in exchange for instrumentation that's this much better seems to be worth it.

Pedantic note

The godbolt example erroneously passes `-C opt-level=2` instead of `-C opt-level=3` to the compiler. 3 is the default for Cargo release builds. But changing it to 3 doesn't change the resulting assembly.

[Shnatsel]

As for doing this without messing with LLVM passes, our best bet is an equivalent sancov option. I couldn't find any documentation for sancov options, but at least their list is defined here: https://llvm.org/doxygen/SanitizerCoverage_8cpp_source.html

I do not see anything relating to cmov or SelectInst in the list.

[DanBlackwell]

As for doing this without messing with LLVM passes, our best bet is an equivalent sancov option. I couldn't find any documentation for sancov options, but at least their list is defined here: https://llvm.org/doxygen/SanitizerCoverage_8cpp_source.html

I do not see anything relating to cmov or SelectInst in the list.

I only discovered this by discussing this with one of the sanitizer maintainers actually - their initial suggestion was to modify SanitizerCoverage myself... seemed overkill, but AFL++ does that here. This just seems like a much simpler option...

[Shnatsel]

Yeah, that (among other things) makes AFL++ difficult to compile and in turn difficult to use. That's why I almost entirely gave up on it in favor of using cargo fuzz.

[DanBlackwell]

hmm, this doesn't do what I want:

    #[arg(long, default_value = "true")]
    pub disable_branch_folding: bool,

I could rename it to enable_branch_folding, but that makes it sound like we're adding a new optimisation rather than disabling a standard one. What's the best solution here?

[fitzgen]

It should be an Option<bool> without a default_value. Then, when we actually use it, we do

let disable = opts.disable_branch_folding
    .unwrap_or_else(|| compute_what_the_default_should_be());

[DanBlackwell]

Ok, got you 7990230 has it set to default to off (i.e. don't add the compiler flag), and 83cb30b defaults to on. Let me know which you prefer.

btw, the run_with_coverage test fails for me even on main; I'm running with cargo +nightly test, is there anything extra I need to do or is that test failing for everyone?

[fitzgen]

LGTM with one nitpick inline below, thanks!

[fitzgen]

Let's inline the explanation here. It will be more likely to be kept up to date that way, and user's won't have to chase hyperlinks.

[DanBlackwell]

Please see the latest commit; I've condensed it as much as I can, but it's definitely a long doc comment.

[fitzgen]

Fantastic, thanks!

[fitzgen]

Looks like there is some trailing whitespace that cargo fmt is complaining about: https://github.com/rust-fuzz/cargo-fuzz/actions/runs/10288807948/job/28475210232?pr=380#step:4:8

btw, the run_with_coverage test fails for me even on main; I'm running with cargo +nightly test, is there anything extra I need to do or is that test failing for everyone?

Testing locally now, will get back to you in a minute. It's been a while since I ran these tests, quite possible that something on nightly changed and broke them.

[DanBlackwell]

Agh, I swear I'd ran it through fmt before I committed. Just fixed that, hopefully that's all for the CI failure!

[fitzgen]

run_with_coverage passes okay for me.

I have nightly as my default in rustup, so I just did cargo test.

This is on cargo 1.82.0-nightly (b5d44db1d 2024-07-26).

[DanBlackwell]

run_with_coverage passes okay for me.

I have nightly as my default in rustup, so I just did cargo test.

This is on cargo 1.82.0-nightly (b5d44db1d 2024-07-26).

I'm on rustc 1.82.0-nightly (60d146580 2024-08-06) now and it still fails. I might add this is on an M-series macbook, so maybe an AArch64 difference rather than a bug.

EDIT: It's this I think:

    Error: Merging raw coverage files failed.
    
    Do you have LLVM coverage tools installed?
    https://doc.rust-lang.org/rustc/instrument-coverage.html#installing-llvm-coverage-tools

EDIT 2: If it works for you, that's good enough - it seems non-trivial to install llvm-tools on macOS (with brew at least)

[DanBlackwell]

btw @fitzgen, clippy has some lints when I ran it; is it worth me creating a PR to fix those?

warning: unneeded `return` statement
   --> src/project.rs:287:13
    |
287 |             return Ok(Some(PathBuf::from(target_dir)));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
    = note: `#[warn(clippy::needless_return)]` on by default
help: remove `return`
    |
287 -             return Ok(Some(PathBuf::from(target_dir)));
287 +             Ok(Some(PathBuf::from(target_dir)))
    |

warning: this expression creates a reference which is immediately dereferenced by the compiler
   --> src/project.rs:321:51
    |
321 |         if let Some(target_dir) = self.target_dir(&build)? {
    |                                                   ^^^^^^ help: change this to: `build`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
    = note: `#[warn(clippy::needless_borrow)]` on by default

warning: this expression creates a reference which is immediately dereferenced by the compiler
   --> src/project.rs:690:75
    |
690 |                 self.create_coverage_cmd(coverage, &coverage_out_raw_dir, &corpus.as_path())?;
    |                                                                           ^^^^^^^^^^^^^^^^^ help: change this to: `corpus.as_path()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

[fitzgen]

I personally find that, by default, clippy is a bit too noisy for my tastes. These particular warnings all seem fine to fix.