Skip to content

Use generator expressions for variables instead of branching ifs, which don't work on multi-config generators. #17042

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

Closed
Please-just-dont opened this issue Apr 12, 2025 · 3 comments
Labels

Comments

@Please-just-dont
Copy link

Please-just-dont commented Apr 12, 2025

I did this

MSVC and XCode are what are known as multi-config generators, and it's not possible to branch on CMAKE_BUILD_TYPE using if statements. Meaning, that if in Curl CMakeLists the condition of whether to enable LTO (interprocedural optimization) looks like this:

if (USE_LTO)

Then it can never work with MSVC or XCode, is my understanding. I guess this is a limitation of CMake.

In my code I want to enable LTO in Debug mode, so I do:
set(CURL_LTO $<$<CONFIG:DEBUG>:TRUE> CACHE BOOL "")

But then when Curl checks this variable to decide whether to enable LTO, it's too early, as generator expressions don't work in if statements.

Suggestion:

As far as I know, generator expressions work with multi-config and non multi-config generators. The relevant part in CMakeLists.txt is:

if(CURL_LTO)
  if(CMAKE_VERSION VERSION_LESS 3.9)
    message(FATAL_ERROR "LTO has been requested, but your cmake version ${CMAKE_VERSION} is to old. You need at least 3.9")
  endif()

  cmake_policy(SET CMP0069 NEW)

  include(CheckIPOSupported)
  check_ipo_supported(RESULT CURL_HAS_LTO OUTPUT _lto_error LANGUAGES C)
  if(CURL_HAS_LTO)
    message(STATUS "LTO supported and enabled")
  else()
    message(FATAL_ERROR "LTO has been requested, but the compiler does not support it\n${_lto_error}")
  endif()
endif()

The user sets CURL_LTO with a generator expression, or a simple set, both should work. And then in the lib CMakeLists.txt:

change:

  if(CURL_HAS_LTO)
    set_target_properties(${LIB_OBJECT} PROPERTIES INTERPROCEDURAL_OPTIMIZATION TRUE)
  endif()

to

set_target_properties(${LIB_OBJECT} PROPERTIES INTERPROCEDURAL_OPTIMIZATION $<CURL_LTO:TRUE>)

This should work on CURL_LTO variables set through normal 'set' and set through generator expressions, and will get rid of inconsistent settings at build time (I got warnings on MSVC that that LTCG wasn't set, or something like that.) Someone who knows CMake better can come up with something better. Just to reiterate, it's my belief that this won't change behaviour with users who have already set CURL_LTO through a simple variable (non-generator-expression) variable, I think.

I expected the following

No response

curl/libcurl version

Latest, linking from source code.

operating system

Windows, Linux.

@vszakats
Copy link
Member

The condition curl uses here is not CURL_LTO, but CURL_HAS_LTO.

In this case, wouldn't it be more straightforward to restore the pre- a1eaa12
logic for multi-target generators, and keep the current code otherwise?:

if(CURL_HAS_LTO)
  if(CMAKE_CONFIGURATION_TYPES)
    set_target_properties(${LIB_OBJECT} PROPERTIES
      INTERPROCEDURAL_OPTIMIZATION_RELEASE TRUE
      INTERPROCEDURAL_OPTIMIZATION_RELWITHDEBINFO TRUE)
  else()
    set_target_properties(${LIB_OBJECT} PROPERTIES INTERPROCEDURAL_OPTIMIZATION TRUE)
  endif()
endif()

@Please-just-dont
Copy link
Author

@vszakats OK, I've never worked on this library, I'm not familiar with anything, this is just a heads-up to let you know in case you want to do something about it.

@vszakats
Copy link
Member

No worries, thanks for reporting this. I was contemplating addressing
it after your #17034 report, as a regression from the linked commit.

@vszakats vszakats added the cmake label Apr 12, 2025
nbaws pushed a commit to nbaws/curl that referenced this issue Apr 26, 2025
To avoid having LTO enabled for Debug configurations with multi-config
generators (e.g. MSVC.)

Reported-by: PleaseJustDont
Fixes curl#17042
Ref: #curl#17034
Follow-up to a1eaa12 curl#15829
Closes curl#17043
nbaws pushed a commit to nbaws/curl that referenced this issue Apr 26, 2025
To avoid having LTO enabled for Debug configurations with multi-config
generators (e.g. MSVC.)

Reported-by: PleaseJustDont
Fixes curl#17042
Ref: #curl#17034
Follow-up to a1eaa12 curl#15829
Closes curl#17043
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants