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

Re-enable lto support guarded behind feature flags #198

Merged
merged 1 commit into from
Jan 27, 2023
Merged

Re-enable lto support guarded behind feature flags #198

merged 1 commit into from
Jan 27, 2023

Conversation

NobodyXu
Copy link
Contributor

  • fat-lto for fat lto, will override thin-lto if specified
  • thin-lto for thin lto, will fallback to fat-lto is not supported

Signed-off-by: Jiahao XU [email protected]

 - fat-lto for fat lto, will override thin-lto if specified
 - thin-lto for thin lto, will fallback to fat-lto is not supported

Signed-off-by: Jiahao XU <[email protected]>
Comment on lines +154 to +160
if cfg!(feature = "fat-lto") {
config.flag_if_supported("-flto");
} else if cfg!(feature = "thin-lto") {
flag_if_supported_with_fallbacks(
&mut config,
&["-flto=thin", "-flto"],
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous bug with lto should be fixed by rust-lang/cc-rs#757

@gyscos I recommend to upgrade to new cc once it lands.

But still, I want to be safe here by putting them behind feature flags so that users can decide whether to enable fat-lto or thin-lto.

@gyscos gyscos merged commit bd75cb4 into gyscos:main Jan 27, 2023
@gyscos
Copy link
Owner

gyscos commented Jan 27, 2023

Thanks for the work!

@NobodyXu NobodyXu deleted the re-enable-lto branch January 27, 2023 02:25
@gyscos
Copy link
Owner

gyscos commented Jan 27, 2023

Hmmm at this moment running cargo test --features thin-lto from zstd-safe fails for me.
(Running it from zstd-sys does not matter because the tests don't actually link anything.)

@NobodyXu NobodyXu mentioned this pull request Jan 27, 2023
@NobodyXu
Copy link
Contributor Author

Hmmm at this moment running cargo test --features thin-lto from zstd-safe fails for me. (Running it from zstd-sys does not matter because the tests don't actually link anything.)

Hmmm both fat-lto and thin-lto works on my M1 macbook.

Well, at least we can override thin-lto with fat-lto, which should work more reliably on more targets.

@gyscos
Copy link
Owner

gyscos commented Jan 27, 2023

It also fails with --features fat-lto. (Running on a x86_64 linux machine).

Indeed, running it on a M1 macbook running macOS, the tests pass.

Might be because on the linux machine, cc is gcc, while it's clang on macOS.

@NobodyXu
Copy link
Contributor Author

@gyscos It should get better with rust-lang/cc-rs#757 once cc releases a new version.

@gyscos
Copy link
Owner

gyscos commented Jan 27, 2023

Aaah right I had an explicit override for gcc before. Now it only relies on cc detection, which still hasn't released. Sorry for the confusion.

(Was just trying to understand why it's now working worse for me than before disabling LTO.)

@NobodyXu
Copy link
Contributor Author

Now it only relies on cc detection, which still hasn't released.

My fix for cc is actually really trivial if you look at the PR: It simply checks for the exit status.

I don't know why it didn't check for that but this should improve things a bit, when compiler fails without any stderr output.

@gyscos
Copy link
Owner

gyscos commented Jan 29, 2023

(This post may sound trivial for anyone used to LTO, but I was not, so here's my journey.)

Soo I just realized this only works if the C lib is compiled by clang (or another LLVM-based compiler), otherwise the intermediate compiled file will only contain gcc's flavor of IR, which rust cannot use.

So at the moment, both fat-lto and thin-lto only successfully compile if CC is set to clang, and if the linker is given to proper option.

In addition, -Clinker-plugin-lto is probably recommended to actually run LTO, though it appears to at least compile fine if omitted.

TLDR, In my case, I had to run:

RUSTFLAGS="-Clinker-plugin-lto -Clink-arg=-fuse-ld=lld" CC=clang cargo test -vv --features thin-lto

Unsurprisingly, on platforms where clang is already the default compiler/linker, these overrides are not required.

It also means that, since this can currently only work with clang, there never was a real need for the better gcc lto-support detection from cc-rs. :^/

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.

2 participants