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.

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

Disable lto and explicit profile #385

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ijc
Copy link

@ijc ijc commented Sep 18, 2024

Fixes #384.

I included both the disable LTO bit and the capability to support custom profiles. I'm happy to drop either one.

I'm concerned about how custom profiles will interact with these uses of build.release and build.dev:

cargo-fuzz/src/project.rs

Lines 233 to 235 in a608970

if !build.release || build.debug_assertions || build.careful_mode {
rustflags.push_str(" -Cdebug-assertions");
}

and

cargo-fuzz/src/project.rs

Lines 241 to 251 in a608970

// If release mode is enabled then we force 1 CGU to be used in rustc.
// This will result in slower compilations but it looks like the sancov
// passes otherwise add `notEligibleToImport` annotations to functions
// in LLVM IR, meaning that *nothing* can get imported with ThinLTO.
// This means that in release mode, where ThinLTO is critical for
// performance, we're taking a huge hit relative to actual release mode.
// Local tests have once showed this to be a ~3x faster runtime where
// otherwise functions like `Vec::as_ptr` aren't inlined.
if !build.dev {
rustflags.push_str(" -C codegen-units=1");
}

I'm unclear how to determine the right condition for a custom profile -- might be a reason to drop that bit.

@fitzgen
Copy link
Member

fitzgen commented Sep 18, 2024

I'm concerned about how custom profiles will interact with ...

I haven't looked at the actual PR yet, but I think we can make it so that we have a build.profile that is ultimately a String1 and we can match on that value and set these flags only for the profiles and special cases we are aware of. For custom profiles, the user can always specify their own custom flags in the profile definition, and that might be the best we can offer here.

Footnotes

  1. Or an enum Profile { Dev, Release, Custom(String) } or something.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM with the nitpick below addressed.

src/project.rs Outdated
Comment on lines 152 to 153
} else {
cmd.args(["--config", "profile.dev.lto=false"]);
Copy link
Member

Choose a reason for hiding this comment

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

Can you do this in the above .arg(..).arg(..) method chain, since it is added in both dev and non-dev, and then just have an if without an else for adding the !build.dev extra args? That would cut down on duplication a little.

Copy link
Author

Choose a reason for hiding this comment

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

I think this is a comment on the first commit, at this commit the two config options are slightly different since they contain the profile name profile.release.lto=false vs profile.dev.lto=false which I think makes it hard to directly refactor as you suggest.

But this actually changes how you suggest in later commits as part of making the profile support more generic and we end up with:

        cmd.args([
            &format!("--profile={profile}"),
            "--config",
            &format!("profile.{profile}.debug=true"),
            "--config",
            &format!("profile.{profile}.lto=false"),
        ]);

@ijc
Copy link
Author

ijc commented Sep 20, 2024

@fitzgen I'm not sure what is wrong with CI.

I ran the cargo +nightly-2024-09-18 test locally (since that is the version CI picked up) and it passes.

@fitzgen
Copy link
Member

fitzgen commented Sep 20, 2024

I've retriggered CI, just in case it was some weird ephemeral issue (haven't really had those issues as far as I can remember)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot fuzz if profile.release.lto = true
2 participants