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 <args>") 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 17 Jan 2020 07:10:13 +0000 Gerrit-HasComments: Yes
