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

add a RUSTLINKFLAGS environment variable #4349

Open
froydnj opened this issue Aug 1, 2017 · 12 comments
Open

add a RUSTLINKFLAGS environment variable #4349

froydnj opened this issue Aug 1, 2017 · 12 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-environment-variables Area: environment variables A-profiles Area: profiles A-rustflags Area: rustflags C-bug Category: bug S-triage Status: This issue is waiting on initial triage.

Comments

@froydnj
Copy link
Contributor

froydnj commented Aug 1, 2017

The motivating example here is when you want to have development and release profiles with lto = false, because LTO is so expensive. Why not put lto = true in your release profile? Because the release profile might not be used for releases, per se; it might be used for people doing local development, and they don't want to bear the burden of compiling with LTO all the time. They want to compile with optimization on so at least their performance measurements are not completely out of whack.

But you do want to compile with LTO at certain times: when you're doing a formal release of your software, for instance. It would be convenient to turn LTO on via -C lto in RUSTFLAGS...but -C lto isn't valid for all places where RUSTFLAGS would be passed.

Hence the request for a RUSTLINKFLAGS or similar that would specify flags to be used in the final link of a binary artifact.

This could be made somewhat superfluous by #2007, where we could specify another profile for "release releases", but it might still be handy to have for flags that can't be injected via Cargo.toml.

/cc @luser @glandium

@alexcrichton
Copy link
Member

This is a pressing enough problem that I don't think it's worth waiting for custom profiles (as those may take awhile to get underway).

Do you have a gist of the errors you were seeing with RUSTFLAGS=-Clto? I think the "best" solution here may be to just fix the compiler to stop complaining about -Clto on things like dylibs/cdylibs, seeing how -Clto is just an "optimization preference" it may make sense to just bypass it in those situations.

Failing that though I may prefer to take a more targeted route for now where we just allow LTO (e.g. CARGO_BUILD_LTO=1) rather than arbitrary flags at the "link" stage. That'd solve the problem for now and still leave room for the final solution (custom profiles) I believe. Were you thinking of solving other problems with RUSTLINKFLAGS though? If so seems like not the worst feature to add!

@glandium
Copy link
Contributor

glandium commented Aug 2, 2017

A random thought I just had is that an argument to cargo that allows to override what is set in Cargo.toml would go a long way (similarly to e.g. hg or git allowing to set/override their config from the command line). Something like:

cargo build -c profile.release.lto=false

@alexcrichton
Copy link
Member

Hm yeah an excellent point! It's worth noting though that cargo's system configuration is pretty different than Cargo.toml configuration, but I think we should make an exception for profiles which really should be build time configuration

@froydnj
Copy link
Contributor Author

froydnj commented Aug 2, 2017

Do you have a gist of the errors you were seeing with RUSTFLAGS=-Clto? I think the "best" solution here may be to just fix the compiler to stop complaining about -Clto on things like dylibs/cdylibs, seeing how -Clto is just an "optimization preference" it may make sense to just bypass it in those situations.

The error when lto = false in Cargo.toml and RUSTFLAGS="-C lto" is:

error: lto can only be run for executables, cdylibs and static library outputs

which I think fits in with your suggestion of dropping the warning?

Were you thinking of solving other problems with RUSTLINKFLAGS though?

I can't think of any problems at the moment! I like the CARGO_BUILD_LTO=1 idea and/or @glandium's command-line configuration. @glandium's command-line configuration stuff seems less likely to cause problems at a later date and more generally flexible/useful anyway. (I have some reservations about exposing that much build configuration to the command-line, but I suppose it's not wildly different from what you already have with --features.) We'd want that flag to be a global flag to cargo, though, and not cargo build-specific, yes?

@luser
Copy link
Contributor

luser commented Aug 2, 2017

If we're going to fix this in a simple way, making rustc not warn about -C lto for rlib compiles seems like the simplest fix. For a better long-term solution I think custom profiles is the best way to go, personally. Being able to override individual profile bits on the commandline is a neat idea as well, though, and I could see either of them being a reasonable solution.

@glandium
Copy link
Contributor

glandium commented Aug 2, 2017

The problem with custom profiles is that they're quadratic. You may need variants for debug and non-debug. Opt vs. no-opt. LTO vs no-LTO, abort panics vs. unwind, etc.

@alexcrichton
Copy link
Member

@froydnj

which I think fits in with your suggestion of dropping the warning?

Ah yes, indeed! In retrospect I think at most a warning should be emitted in that case and in reality we should just ignore it in the compiler, doing a 'best effort' thing when possible and otherwise ignoring it for things like rlibs.

I can't think of any problems at the moment! I like the CARGO_BUILD_LTO=1 idea and/or @glandium's command-line configuration. @glandium's command-line configuration stuff seems less likely to cause problems at a later date and more generally flexible/useful anyway.

Indeed! I'll comment on the PR wrt this strategy.

@carols10cents carols10cents added A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug A-profiles Area: profiles labels Aug 27, 2017
@cgwalters
Copy link

Cross-linking https://bugzilla.mozilla.org/show_bug.cgi?id=1386371 here

cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Nov 5, 2018
For us, this is primarily right now a size issue.  See:
https://internals.rust-lang.org/t/rust-staticlibs-and-optimizing-for-size/5746

For more information, there are these two issues:
rust-lang/cargo#4349
https://bugzilla.mozilla.org/show_bug.cgi?id=1386371

The basic issue here is that a build with LTO off (and a trivial
change to add a `println!` takes 14s here, and with it on takes 38s.
However, with LTO off the stripped size of `librpmostree_rust.a` is
`6M`, with LTO on it's `1.1M`.

I named this `--enable-lto` as I'd like to investigate doing this
for the C code too.
rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this issue Nov 5, 2018
For us, this is primarily right now a size issue.  See:
https://internals.rust-lang.org/t/rust-staticlibs-and-optimizing-for-size/5746

For more information, there are these two issues:
rust-lang/cargo#4349
https://bugzilla.mozilla.org/show_bug.cgi?id=1386371

The basic issue here is that a build with LTO off (and a trivial
change to add a `println!` takes 14s here, and with it on takes 38s.
However, with LTO off the stripped size of `librpmostree_rust.a` is
`6M`, with LTO on it's `1.1M`.

I named this `--enable-lto` as I'd like to investigate doing this
for the C code too.

Closes: #1664
Approved by: jlebon
@ehuss ehuss added the A-environment-variables Area: environment variables label Sep 21, 2019
@epage
Copy link
Contributor

epage commented Oct 17, 2023

If I'm understanding correctly, there is a somewhat related RFC at rust-lang/rfcs#3310

@epage epage added the A-rustflags Area: rustflags label Oct 17, 2023
@epage
Copy link
Contributor

epage commented Dec 6, 2024

Also, if we expose more rustflags in Cargo-native ways (#12739), then Cargo can then decide to only fingerprint and pass the flag to final artifacts.

@epage
Copy link
Contributor

epage commented Dec 6, 2024

btw LTO is not a great case for this as we now do change dependencies build based on lto as discussed in #3244.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-environment-variables Area: environment variables A-profiles Area: profiles A-rustflags Area: rustflags C-bug Category: bug S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

8 participants