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

Improve zstd-sys/build.rs: Fallback to -Os or -O2 if -Oz is not supported #172

Merged
merged 1 commit into from
Nov 24, 2022
Merged

Improve zstd-sys/build.rs: Fallback to -Os or -O2 if -Oz is not supported #172

merged 1 commit into from
Nov 24, 2022

Conversation

NobodyXu
Copy link
Contributor

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

@NobodyXu NobodyXu changed the title Improve zstd-sys/build.rs: Add fallback to thin lto and -Oz Improve zstd-sys/build.rs: Add fallback to thin lto and -Oz, and enable LTO only for clang-like compiler Nov 1, 2022
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Nov 1, 2022

@gyscos Can you review this PR please?

@NobodyXu
Copy link
Contributor Author

@gyscos Pinging

@gyscos
Copy link
Owner

gyscos commented Nov 24, 2022

Unfortunately I'm not sure is_flag_supported does its job: it seems that for some versions of gcc, it will accept a flag, even though this flag leads to a non-working compilation.

I'm still trying to figure out how to safely enable any form of lto on GCC that will work on both recent gcc version, older gcc version used in cross (gcc 9), and the gcc used in the windows-gnu target...

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Nov 24, 2022

Unfortunately I'm not sure is_flag_supported does its job: it seems that for some versions of gcc, it will accept a flag, even though this flag leads to a non-working compilation.

I'm still trying to figure out how to safely enable any form of lto on GCC that will work on both recent gcc version, older gcc version used in cross (gcc 9), and the gcc used in the windows-gnu target...

Perhaps we should just compile a simple hello-world with lto to see if that works?
If it can be compiled, then it's likely it would work.

I can try implementing that in a new PR and meanwhile I will reduce the scope of this PR

@gyscos
Copy link
Owner

gyscos commented Nov 24, 2022

I think a simpler option to start would be letting the end user override this "guesswork" and explicitly enable LTO or other options.

Some magic to detect supported features would also be valuable, but it's not clear if it should go here or in cc itself.

@NobodyXu NobodyXu changed the title Improve zstd-sys/build.rs: Add fallback to thin lto and -Oz, and enable LTO only for clang-like compiler Improve zstd-sys/build.rs: Add fallback to -Os or -O2 if -Oz is not supported Nov 24, 2022
@NobodyXu NobodyXu changed the title Improve zstd-sys/build.rs: Add fallback to -Os or -O2 if -Oz is not supported Improve zstd-sys/build.rs: Fallback to -Os or -O2 if -Oz is not supported Nov 24, 2022
@gyscos gyscos merged commit e52020e into gyscos:master Nov 24, 2022
@NobodyXu NobodyXu deleted the improve-build-rs branch November 24, 2022 13:54
@gyscos
Copy link
Owner

gyscos commented Nov 24, 2022

Actually, do we need to turn on O2 manually? What's the advantage vs letting cc pick the optimization level?

I realize it's only when none of Oz or Os works (not sure when that happens), but that still looks like a different decision.

@NobodyXu
Copy link
Contributor Author

Actually, do we need to turn on O2 manually? What's the advantage vs letting cc pick the optimization level?

It's only turned on if feature "thin" is enabled.
Using -O2 probably generates smaller code than -O3 (or even -O1 sometimes).

@NobodyXu
Copy link
Contributor Author

I realize it's only when none of Oz or Os works (not sure when that happens),

I remember that gcc doesn't support -Oz and only -Os?

@NobodyXu
Copy link
Contributor Author

Unfortunately I'm not sure is_flag_supported does its job: it seems that for some versions of gcc, it will accept a flag, even though this flag leads to a non-working compilation.

I'm still trying to figure out how to safely enable any form of lto on GCC that will work on both recent gcc version, older gcc version used in cross (gcc 9), and the gcc used in the windows-gnu target...

I checked Build::is_flag_supported and it actually attempts to compile the file with the flag provided.

I think the problem lies in this line, where it does not check the exit status of the command and assumes that if the stderr is empty, everything works as expected.

@NobodyXu
Copy link
Contributor Author

@gyscos I've opend rust-lang/cc-rs#757 which might help fix the LTO issue in zstd.

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