cmake: disable LTO for some configure checks#1850
Conversation
Some of zlib-ng's configure tests define a function expecting it to be compiled but don't call that function, or don't use its return value. This is risky with LTO where the whole thing may be optimised out, which has happened before: * zlib-ng#1616 * zlib-ng#1622 * https://gitlab.kitware.com/cmake/cmake/-/issues/26103 Closes: zlib-ng#1841
WalkthroughThe changes in the Changes
Assessment against linked issues
The changes directly address the core concern raised in issue #1841 by introducing a mechanism to disable Link Time Optimization during specific compiler configuration checks. This prevents the compiler from potentially optimizing out test functions that are not explicitly used, which could lead to incorrect feature detection. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I diffed the generated CMake cache files on amd64 + x86 and saw no change as expected (given this is a defensive change). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CMakeLists.txt (1)
304-304: Consider documenting the LTO disable pattern.The pattern of disabling LTO for configure checks is now used in multiple places. Consider adding a comment in the CMake code explaining why LTO needs to be disabled for these checks, or create a helper function to encapsulate this pattern.
Example helper function:
function(check_compiler_feature_no_lto) cmake_parse_arguments(ARG "" "FEATURE_NAME;TEST_SOURCE" "" ${ARGN}) set(CMAKE_REQUIRED_FLAGS ${ZNOLTOFLAG}) check_c_source_compiles( "${ARG_TEST_SOURCE}" ${ARG_FEATURE_NAME} ) set(CMAKE_REQUIRED_FLAGS) endfunction()Also applies to: 494-507, 512-525, 530-543
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMakeLists.txt(5 hunks)
🔇 Additional comments (3)
CMakeLists.txt (3)
304-304: LGTM! Properly disables LTO for MSVC AVX detection.The addition of
${ZNOLTOFLAG}prevents Link Time Optimization from interfering with the AVX feature detection check.
Line range hint
494-507: LGTM! Properly disables LTO for __builtin_assume_aligned detection.The addition of
${ZNOLTOFLAG}prevents the test function from being optimized away during configure checks. The flag is correctly scoped and cleared after the check.
Line range hint
512-525: LGTM! Properly disables LTO for __builtin_ctz and __builtin_ctzll detection.The addition of
${ZNOLTOFLAG}prevents the test functions from being optimized away during configure checks. The flags are correctly scoped and cleared after each check.Also applies to: 530-543
| # Workaround for MSVC. By default MSVC does not define the __SSE*__ macros. | ||
| # Fix it if AVX is enabled. | ||
| set(CMAKE_REQUIRED_FLAGS "${NATIVEFLAG}") | ||
| set(CMAKE_REQUIRED_FLAGS "${NATIVEFLAG} ${ZNOLTOFLAG}") |
There was a problem hiding this comment.
question (if-minor): Does this actually make any difference? AFAIK there is no support for LTO in MSVC, so the variable would be empty.
There was a problem hiding this comment.
I wondered the same -- I figured I'd be consistent given someone might copy it in future, or MSVC might gain it at some point.
I googled a bit and found https://stackoverflow.com/questions/42230141/how-to-write-lto-enabled-code which implies it has it under the name "LTCG", but honestly, I'm no MSVC expert at all. Happy to drop it if you want.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1850 +/- ##
============================================
- Coverage 82.19% 32.80% -49.40%
============================================
Files 138 69 -69
Lines 10985 5661 -5324
Branches 2695 1233 -1462
============================================
- Hits 9029 1857 -7172
- Misses 1261 3548 +2287
+ Partials 695 256 -439 ☔ View full report in Codecov by Sentry. |
Some of zlib-ng's configure tests define a function expecting it to be compiled but don't call that function, or don't use its return value. This is risky with LTO where the whole thing may be optimised out, which has happened before:
Closes: #1841
Summary by CodeRabbit