[PATCH] D16999: [CMake] Improve the clang order-file generation workflow

[Justin Bogner via cfe-commits]

Chris Bieneman <beanz at apple.com> writes:
> beanz created this revision.
> beanz added a reviewer: bogner.
> beanz added a subscriber: cfe-commits.
>
> This commit re-lands r259862. The underlying cause of the build
> breakage was an incorrectly written capabilities test. In
> tools/Driver/CMakeLists.txt I was attempting to check if a linker flag
> worked, the test was passing it to the compiler, not the linker. CMake
> doesn't have a linker test, so we have a hand-rolled one.
>
> Original Patch Review: http://reviews.llvm.org/D16896
>
> Original Summary:
> With this change generating clang order files using dtrace uses the
> following workflow:
>
> cmake <whatever options you want>
>
> ninja generate-order-file
>
> ninja clang
>
> This patch works by setting a default path to the order file (which
> can be overridden by the user). If the order file doesn't exist during
> configuration CMake will create an empty one.
>
> CMake then ties up the dependencies between the clang link job and the
> order file, and generate-order-file overwrites CLANG_ORDER_FILE with
> the new order file.

This looks it'll work. I do have one comment / concern below, but as
long as that's addressed this LGTM.

> http://reviews.llvm.org/D16999
>
> Files:
>   CMakeLists.txt
>   tools/driver/CMakeLists.txt
>   utils/perf-training/CMakeLists.txt
>
> Index: utils/perf-training/CMakeLists.txt
> ===================================================================
> --- utils/perf-training/CMakeLists.txt
> +++ utils/perf-training/CMakeLists.txt
> @@ -55,9 +55,8 @@
>      COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/perf-helper.py clean ${CMAKE_CURRENT_BINARY_DIR} dtrace
>      COMMENT "Clearing old dtrace data")
>  
> -
>    add_custom_target(generate-order-file
> -    COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/perf-helper.py gen-order-file --binary $<TARGET_FILE:clang> --output ${CMAKE_CURRENT_BINARY_DIR}/clang.order ${CMAKE_CURRENT_BINARY_DIR}
> +    COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/perf-helper.py gen-order-file --binary $<TARGET_FILE:clang> --output ${CLANG_ORDER_FILE} ${CMAKE_CURRENT_BINARY_DIR}
>      COMMENT "Generating order file"
>      DEPENDS generate-dtrace-logs)
>  endif()
> Index: tools/driver/CMakeLists.txt
> ===================================================================
> --- tools/driver/CMakeLists.txt
> +++ tools/driver/CMakeLists.txt
> @@ -87,8 +87,24 @@
>    set(TOOL_INFO_BUILD_VERSION)
>  endif()
>  
> -if(CLANG_ORDER_FILE AND EXISTS CLANG_ORDER_FILE)
> -  target_link_libraries(clang "-Wl,-order_file,${CLANG_ORDER_FILE}")
> +if(CLANG_ORDER_FILE)
> +  include(CMakePushCheckState)
> +
> +  function(check_linker_flag flag out_var)
> +    cmake_push_check_state()
> +    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} ${flag}")
> +    check_cxx_compiler_flag("" ${out_var})
> +    cmake_pop_check_state()
> +  endfunction()
> +
> +  # This is a test to ensure the actual order file works with the linker.
> +  check_linker_flag("-Wl,-order_file,${CLANG_ORDER_FILE}"
> +    LINKER_ORDER_FILE_WORKS)
> +  
> +  if(LINKER_ORDER_FILE_WORKS)
> +    target_link_libraries(clang "-Wl,-order_file,${CLANG_ORDER_FILE}")
> +    set_target_properties(clang PROPERTIES LINK_DEPENDS ${CLANG_ORDER_FILE})
> +  endif()
>  endif()
>  
>  if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
> Index: CMakeLists.txt
> ===================================================================
> --- CMakeLists.txt
> +++ CMakeLists.txt
> @@ -586,18 +586,19 @@
>    add_subdirectory(docs)
>  endif()
>  
> -if(EXISTS "${CMAKE_CURRENT_BINARY_DIR}/clang.order")
> -  file(REMOVE "${CMAKE_CURRENT_BINARY_DIR}/clang.order")
> -endif()
> -
> -if(CLANG_ORDER_FILE STREQUAL "${CMAKE_CURRENT_BINARY_DIR}/clang.order")
> +# this line is needed as a cleanup to ensure that any CMakeCaches with the old
> +# default value get updated to the new default.
> +if(CLANG_ORDER_FILE STREQUAL "")
>    unset(CLANG_ORDER_FILE CACHE)
> -  unset(CLANG_ORDER_FILE)
>  endif()
>  
> -set(CLANG_ORDER_FILE "" CACHE FILEPATH
> +set(CLANG_ORDER_FILE ${CMAKE_CURRENT_BINARY_DIR}/clang.order CACHE FILEPATH
>    "Order file to use when compiling clang in order to improve startup time.")
>  
> +if(CLANG_ORDER_FILE AND NOT EXISTS ${CLANG_ORDER_FILE})
> +  file(WRITE ${CLANG_ORDER_FILE} "\n")
> +endif()

What happens if someone typos their order file? Won't we end up writing
an empty file somewhere random on their filesystem?

I think we should only do the "create an empty file" thing if the order
file is inside ${CMAKE_CURRENT_BINARY_DIR} - if someone specifies
-DCLANG_ORDER_FILE=/path/to/nonexistent they should get an error.

> +
>  if (CLANG_BUILT_STANDALONE OR CMAKE_VERSION VERSION_EQUAL 3 OR
>      CMAKE_VERSION VERSION_GREATER 3)
>    # Generate a list of CMake library targets so that other CMake projects can
>