Skip to content

Fix -flto support with clang++#3377

Merged
TheMarex merged 6 commits intomasterfrom
fix-clang-lto
Dec 1, 2016
Merged

Fix -flto support with clang++#3377
TheMarex merged 6 commits intomasterfrom
fix-clang-lto

Conversation

@springmeyer
Copy link
Copy Markdown
Contributor

@springmeyer springmeyer commented Nov 29, 2016 •
edited by TheMarex
Loading

Issue

Targeting #2120.

The gcc -flto support works currently by:

  • Telling cmake about gcc-ar and gcc-ranlib (

    osrm-backend/CMakeLists.txt

    Lines 221 to 223 in ef087f9

    message(STATUS "Using gcc specific binutils for LTO.")
    set(CMAKE_AR "/usr/bin/gcc-ar")
    set(CMAKE_RANLIB "/usr/bin/gcc-ranlib")
    )
  • Gcc internally using its lto-wrapper for linking

For clang++ and -flto to work we need to:

    1. Add the -flto flag in both compile flags and link flags manually (since no equivalent lto-wrapper exists for linking with clang++)
    1. Use recent binutils for gold linker that understands clang++ lto format
    1. Provide the absolute paths to llvm-ar and llvm-ranlib

This PR builds i into the CMakeLists.txt and ii + iii into the .travis.yml.

This PR also:

  • Starts passing -flto and related flags to the pkg config file. Without this the example.cpp will not link because it is not compiled with -flto and the lto-enabled libosrm.a cannot be used.
  • Changes the warning about ENABLE_MASON possibly not working with lto on some systems to only display on linux (since OS X and windows are not impacted)

Tasklist

  • review
  • adjust for comments

@springmeyer springmeyer mentioned this pull request Nov 30, 2016
Closed
@TheMarex
Copy link
Copy Markdown
Member

TheMarex commented Dec 1, 2016

Grabbing this to cmakefy this a little bit more.

@TheMarex
No need to set ccache explicitly if its in the path
bff8d2c
@TheMarex
Make the binutil detection work the same for clang and gcc
1317c57 
This uses the cmake internal find_program that will utilize the PATH at
configuration time. This way we don't need to pass CXXFLAGS to clang.
@TheMarex
Copy link
Copy Markdown
Member

TheMarex commented Dec 1, 2016

@springmeyer I moved some of this logic to cmake to make it work consistent with the gcc binutils. Otherwise this looks good to me. 👍

@springmeyer springmeyer mentioned this pull request Dec 1, 2016
Closed
@springmeyer
Copy link
Copy Markdown
Contributor Author

springmeyer commented Dec 1, 2016

@TheMarex great, thanks for the cmake help! I think before merging this we should test node-osrm against it. My hope is that the -flto flags will be picked up by node-osrm and it should link correctly without changes. But we should make sure. I'll kick off a node-osrm build against this branch once travis is green with your changes.

@springmeyer
Copy link
Copy Markdown
Contributor Author

springmeyer commented Dec 1, 2016

confirmed that node-osrm master is working against this branch and the build flags show -flto properly coming through from osrm-backend's pkg-config file: https://travis-ci.org/Project-OSRM/node-osrm/jobs/180491933. I think this is ready to merge.

@TheMarex
Copy link
Copy Markdown
Member

TheMarex commented Dec 1, 2016

@springmeyer 🙇 thanks for you help. Merging.

@TheMarex TheMarex merged commit c99c8bc into master Dec 1, 2016
@TheMarex TheMarex deleted the fix-clang-lto branch December 1, 2016 21:02
springmeyer pushed a commit to mapbox/spatial-algorithms that referenced this pull request May 4, 2017
fix -flto linking with clang/llvm - refs Project-OSRM/osrm-backend#3377
66775b4
zavadovsky pushed a commit to GSGroup/stingraykit that referenced this pull request Sep 2, 2024
[stingray/stingraykit]CMakeLists: explicitly disable lto support for …
16234a5 
…clang

check_cxx_compiler_flag(-flto) doesn't work properly on clang and always produces a negative result, see Project-OSRM/osrm-backend#2120 for details
proposed in Project-OSRM/osrm-backend#3377 solution (use -Wl,-flto instead of -flto) seems to always produce positive result, even in toolchains without lto support, which leads to false positive checks and fails during build

there seems to be some "dirty" workaround (usins try_compile / try_run / check_c_source_compiles)
but because clang builds are low priority and don't really need lto, for now just explicitely disable lto support for them
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Compilation Ready To Merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@springmeyer @TheMarex