[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15058 ) Change subject: build: restrict clang version, prefer lld, enable thinlto .. build: restrict clang version, prefer lld, enable thinlto * Restricts clang to at least version 6.0, which gets rid of a couple older special cases. * We now loop through linkers trying to prefer lld where available, either using the version provided by the compiler with -fuse-lld or explicitly passing the path to the toolchain lld. lld should be faster than gold and GNU ld, and also has the advantage of being a known quantity within our toolchain. This fixes ASAN builds on my Ubuntu 18 machine, where the version of gold that ships with the system can't seem to link ASAN executables. * Switches the LTO build to use ThinLTO, which provides most of the performance benefits but at much lower cost. Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Reviewed-on: http://gerrit.cloudera.org:8080/15058 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M CMakeLists.txt A cmake_modules/KuduLinker.cmake 2 files changed, 191 insertions(+), 120 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15058 ) Change subject: build: restrict clang version, prefer lld, enable thinlto .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15058/4/cmake_modules/KuduLinker.cmake File cmake_modules/KuduLinker.cmake: http://gerrit.cloudera.org:8080/#/c/15058/4/cmake_modules/KuduLinker.cmake@68 PS4, Line 68: # [1] https://reviews.llvm.org/D63974 Might prefer to link to the actual bug itself rather than the CR: https://bugs.llvm.org/show_bug.cgi?id=42442. -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 21 Jan 2020 22:37:23 + Gerrit-HasComments: Yes
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15058 to look at the new patch set (#4). Change subject: build: restrict clang version, prefer lld, enable thinlto .. build: restrict clang version, prefer lld, enable thinlto * Restricts clang to at least version 6.0, which gets rid of a couple older special cases. * We now loop through linkers trying to prefer lld where available, either using the version provided by the compiler with -fuse-lld or explicitly passing the path to the toolchain lld. lld should be faster than gold and GNU ld, and also has the advantage of being a known quantity within our toolchain. This fixes ASAN builds on my Ubuntu 18 machine, where the version of gold that ships with the system can't seem to link ASAN executables. * Switches the LTO build to use ThinLTO, which provides most of the performance benefits but at much lower cost. Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 --- M CMakeLists.txt A cmake_modules/KuduLinker.cmake 2 files changed, 191 insertions(+), 120 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/15058/4 -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15058 ) Change subject: build: restrict clang version, prefer lld, enable thinlto .. Patch Set 3: (3 comments) Something's up with TSAN... http://gerrit.cloudera.org:8080/#/c/15058/3/cmake_modules/KuduLinker.cmake File cmake_modules/KuduLinker.cmake: http://gerrit.cloudera.org:8080/#/c/15058/3/cmake_modules/KuduLinker.cmake@38 PS3, Line 38: # How we handle this situation depends on other factors: : # - If gold is optional, we won't use it. : # - If gold is required and we're using dynamic linking, we'll either: : # - Raise an error in RELEASE builds (we shouldn't release such a product), or : # - Drop tcmalloc in all other builds. Forgot to mention earlier, but this needs to be updated to reflect the new behavior. http://gerrit.cloudera.org:8080/#/c/15058/3/cmake_modules/KuduLinker.cmake@61 PS3, Line 61: Whitespace. http://gerrit.cloudera.org:8080/#/c/15058/3/cmake_modules/KuduLinker.cmake@112 PS3, Line 112: # GNU ld version 2.25.1-22.base.el7 Trailing whitespace. -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Sat, 18 Jan 2020 00:08:03 + Gerrit-HasComments: Yes
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/15058 )
Change subject: build: restrict clang version, prefer lld, enable thinlto
..
Patch Set 3:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@239
PS1, Line 239:
> May want to recommend the ccache wrappers in build-support/ccache-clang ins
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@416
PS1, Line 416: # Doesn't make much sense to do LTO on a non-release build
-- it's slow
> Could you explain why in a comment?
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@419
PS1, Line 419: endif()
> You sure you want PROJECT_BINARY_DIR? We don't call project() so what actua
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake
File cmake_modules/KuduLinker.cmake:
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@20
PS1, Line 20: # Search candidate linkers in the order of decreasing speed and
functionality.
: # In particular, lld is the best choice since it's quite fast
and supports
: # ThinLTO, etc.
: foreach(candid
> Want to move this down to where it's actually needed?
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@25
PS1, Line 25: set(candidate_flag "")
> May want to add a comment explaining the rationale for the ordering (i.e. p
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@26
PS1, Line 26: else()
: set(candidate_flag "-fuse-ld=${candidate_linker}")
: endif()
: GET_LINKER_VERSION("${candidate_flag}")
: if(NOT
> Could you get away with this?
nope, since ${candidate_flag} will hang around from a previous loop iteration
(cmake has function scope, not lexical scope)
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@56
PS1, Line 56:
> How was it fixed?
added a comment. Just tested this experimentally, didn't locate the actual
patch.
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@80
PS1, Line 80: r
> remove
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@85
PS1, Line 85: # LINKER_FAMILY: lld/ld/gold
> Maybe adjust the message a bit if ${ARGN} is empty (i.e. the default linker
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@89
PS1, Line 89: if(ARGN)
: message(STATUS "Checking linker version with cflags: ${ARGN}")
: else()
> Nit: move this down to just above L93.
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@102
PS1, Line 102: message(SEND_ERROR "Could not extract GNU gold version. "
> Would be great if you could provide some sample text to help illustrate how
Done
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@118
PS1, Line 118: set(LINKER_FAMILY "ld")
> Typo here (I think you meant to omit PARENT_SCOPE?) Surprised it didn't giv
think it ended up fine bceause any non-empty string is true according to cmake
--
To view, visit http://gerrit.cloudera.org:8080/15058
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0
Gerrit-Change-Number: 15058
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 17 Jan 2020 21:46:08 +
Gerrit-HasComments: Yes
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15058 to look at the new patch set (#3). Change subject: build: restrict clang version, prefer lld, enable thinlto .. build: restrict clang version, prefer lld, enable thinlto * Restricts clang to at least version 6.0, which gets rid of a couple older special cases. * We now loop through linkers trying to prefer lld where available, either using the version provided by the compiler with -fuse-lld or explicitly passing the path to the toolchain lld. lld should be faster than gold and GNU ld, and also has the advantage of being a known quantity within our toolchain. This fixes ASAN builds on my Ubuntu 18 machine, where the version of gold that ships with the system can't seem to link ASAN executables. * Switches the LTO build to use ThinLTO, which provides most of the performance benefits but at much lower cost. Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 --- M CMakeLists.txt A cmake_modules/KuduLinker.cmake 2 files changed, 183 insertions(+), 120 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/15058/3 -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15058 to look at the new patch set (#2). Change subject: build: restrict clang version, prefer lld, enable thinlto .. build: restrict clang version, prefer lld, enable thinlto * Restricts clang to at least version 6.0, which gets rid of a couple older special cases. * We now loop through linkers trying to prefer lld where available, either using the version provided by the compiler with -fuse-lld or explicitly passing the path to the toolchain lld. lld should be faster than gold and GNU ld, and also has the advantage of being a known quantity within our toolchain. This fixes ASAN builds on my Ubuntu 18 machine, where the version of gold that ships with the system can't seem to link ASAN executables. * Switches the LTO build to use ThinLTO, which provides most of the performance benefits but at much lower cost. Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 --- M CMakeLists.txt A cmake_modules/KuduLinker.cmake 2 files changed, 183 insertions(+), 120 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/15058/2 -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15058 )
Change subject: build: restrict clang version, prefer lld, enable thinlto
..
Patch Set 1:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@239
PS1, Line 239: " CC=${THIRDPARTY_TOOLCHAIN_DIR}/bin/clang CXX=$CC++
cmake ")
May want to recommend the ccache wrappers in build-support/ccache-clang instead.
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@410
PS1, Line 410: if (NOT "${KUDU_USE_LTO}" STREQUAL "")
I think you can also get away with:
if (KUDU_USE_LTO)
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@416
PS1, Line 416: message(FATAL_ERROR "LTO only supported for release builds")
Could you explain why in a comment?
http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@419
PS1, Line 419: set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}
-Wl,--thinlto-cache-dir=${PROJECT_BINARY_DIR}/lto.cache")
You sure you want PROJECT_BINARY_DIR? We don't call project() so what actually
gets used?
Elsewhere we would use CMAKE_CURRENT_BINARY_DIR for this sort of thing.
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake
File cmake_modules/KuduLinker.cmake:
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@20
PS1, Line 20: execute_process(COMMAND which ld.gold
: OUTPUT_VARIABLE GOLD_LOCATION
: OUTPUT_STRIP_TRAILING_WHITESPACE
: ERROR_QUIET)
Want to move this down to where it's actually needed?
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@25
PS1, Line 25: foreach(candidate_linker lld
"${THIRDPARTY_TOOLCHAIN_DIR}/bin/ld.lld" gold default)
May want to add a comment explaining the rationale for the ordering (i.e.
preferences).
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@26
PS1, Line 26: if(candidate_linker STREQUAL "default")
: set(candidate_flag "")
: else()
: set(candidate_flag "-fuse-ld=${candidate_linker}")
: endif()
Could you get away with this?
if(NOT candidate_linker STREQUAL "default")
set(candidate_flag "-fuse-ld=${candidate_linker}")
endif()
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@56
PS1, Line 56: This seems to be fixed in devtoolset-6 or later.
How was it fixed?
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@80
PS1, Line 80: )
remove
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@85
PS1, Line 85: message(STATUS "Checking linker version with cflags: ${ARGN}")
Maybe adjust the message a bit if ${ARGN} is empty (i.e. the default linker)?
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@89
PS1, Line 89: # We're expecting LINKER_OUTPUT to look like one of these:
: # GNU gold (version 2.24) 1.11
: # GNU gold (GNU Binutils for Ubuntu 2.30) 1.15
Nit: move this down to just above L93.
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@102
PS1, Line 102: string(REGEX MATCH "GNU ld version (([0-9]+\\.?)+)" _
"${LINKER_OUTPUT}")
Would be great if you could provide some sample text to help illustrate how the
regex works for ld.bfd and lld.
http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@118
PS1, Line 118: set(LINKER_FOUND TRUEPARENT_SCOPE)
Typo here (I think you meant to omit PARENT_SCOPE?) Surprised it didn't give
you any problems.
--
To view, visit http://gerrit.cloudera.org:8080/15058
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0
Gerrit-Change-Number: 15058
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 17 Jan 2020 07:10:13 +
Gerrit-HasComments: Yes
[kudu-CR] build: restrict clang version, prefer lld, enable thinlto
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/15058 to review the following change. Change subject: build: restrict clang version, prefer lld, enable thinlto .. build: restrict clang version, prefer lld, enable thinlto * Restricts clang to at least version 6.0, which gets rid of a couple older special cases. * We now loop through linkers trying to prefer lld where available, either using the version provided by the compiler with -fuse-lld or explicitly passing the path to the toolchain lld. lld should be faster than gold and GNU ld, and also has the advantage of being a known quantity within our toolchain. This fixes ASAN builds on my Ubuntu 18 machine, where the version of gold that ships with the system can't seem to link ASAN executables. * Switches the LTO build to use ThinLTO, which provides most of the performance benefits but at much lower cost. Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 --- M CMakeLists.txt A cmake_modules/KuduLinker.cmake 2 files changed, 168 insertions(+), 120 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/15058/1 -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo
