<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">In this email I’m going to focus on patch feedback. I’ll send a separate one to talk about the higher-level discussions.<div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 8, 2016, at 4:38 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com" class="">mail@justinbogner.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Chris Bieneman <</span><a href="mailto:beanz@apple.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">beanz@apple.com</a><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">> writes:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">beanz created this revision.<br class="">beanz added reviewers: chandlerc, bogner.<br class="">beanz added a subscriber: llvm-commits.<br class=""><br class="">There are a few LLVM projects that produce runtime libraries. Ideally<br class="">runtime libraries should be built differently than other projects,<br class="">specifically they should be built using the just-built toolchain.<br class=""><br class="">There is support for building compiler-rt in this way from the clang<br class="">build. Moving this logic into the LLVM build is interesting because it<br class="">provides a simpler way to extend the just-built toolchain to include<br class="">LLD and the LLVM object file tools.<br class=""><br class="">Once this functionality is better fleshed out and tested we’ll want to<br class="">encapsulate it in a module that can be used for clang standalone<br class="">builds, and we’ll want to make it the default way to build compiler-rt.<br class=""><br class="">With this patch applied there is no immediate change in the build.<br class="">Moving compiler-rt out from llvm/projects into llvm/runtimes enables<br class="">the functionality.<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">This seems reasonable, but I am a little worried about how transitioning</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">to the new system will work. Will everyone have to move their</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">compiler-rt checkout? Will we continue to support compiler-rt in either</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">place? Both of these are workable, but neither is great. Thoughts?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">This code has a few improvements over the method provided by<br class="">LLVM_BUILD_EXTERNAL_COMPILER_RT. Specifically the sub-ninja command is<br class="">always invoked, so changes to compiler-rt source files will get built<br class="">properly, so this patch can be used for iterative development with<br class="">just-built tools.<br class=""><br class=""><a href="http://reviews.llvm.org/D20992" class="">http://reviews.llvm.org/D20992</a><br class=""><br class="">Files:<br class=""> CMakeLists.txt<br class=""> cmake/modules/LLVMExternalProjectUtils.cmake<br class=""> runtimes/CMakeLists.txt<br class=""><br class="">Index: runtimes/CMakeLists.txt<br class="">===================================================================<br class="">--- /dev/null<br class="">+++ runtimes/CMakeLists.txt<br class="">@@ -0,0 +1,16 @@<br class="">+include(LLVMExternalProjectUtils)<br class="">+<br class="">+# Discover the projects that use CMake in the subdirectories.<br class="">+# Note that explicit cmake invocation is required every time a new project is<br class="">+# added or removed.<br class="">+<br class="">+add_custom_target(runtimes)<br class="">+<br class="">+file(GLOB entries *)<br class="">+foreach(entry ${entries})<br class="">+ if(IS_DIRECTORY ${entry} AND EXISTS ${entry}/CMakeLists.txt)<br class="">+ get_filename_component(projName ${entry} NAME)<br class="">+ llvm_ExternalProject_Add(${projName} ${entry} USE_TOOLCHAIN)<br class="">+ add_dependencies(runtimes ${projName})<br class="">+ endif()<br class="">+endforeach(entry)<br class="">Index: cmake/modules/LLVMExternalProjectUtils.cmake<br class="">===================================================================<br class="">--- cmake/modules/LLVMExternalProjectUtils.cmake<br class="">+++ cmake/modules/LLVMExternalProjectUtils.cmake<br class="">@@ -29,7 +29,8 @@<br class=""># Extra targets in the subproject to generate targets for<br class=""># )<br class="">function(llvm_ExternalProject_Add name source_dir)<br class="">- cmake_parse_arguments(ARG "USE_TOOLCHAIN;EXCLUDE_FROM_ALL;NO_INSTALL"<br class="">+ cmake_parse_arguments(ARG<br class="">+ "USE_TOOLCHAIN;EXCLUDE_FROM_ALL;NO_INSTALL;ALWAYS_CLEAN"<br class=""> "SOURCE_DIR"<br class=""> "CMAKE_ARGS;TOOLCHAIN_TOOLS;RUNTIME_LIBRARIES;DEPENDS;EXTRA_TARGETS" ${ARGN})<br class=""> canonicalize_tool_name(${name} nameCanon)<br class="">@@ -52,6 +53,10 @@<br class=""> endif()<br class=""> endforeach()<br class=""><br class="">+ if(ARG_ALWAYS_CLEAN)<br class="">+ set(always_clean clean)<br class="">+ endif()<br class="">+<br class=""> list(FIND TOOLCHAIN_TOOLS clang FOUND_CLANG)<br class=""> if(FOUND_CLANG GREATER -1)<br class=""> set(CLANG_IN_TOOLCHAIN On)<br class="">@@ -135,6 +140,14 @@<br class=""> CMAKE_ARGS ${${nameCanon}_CMAKE_ARGS}<br class=""> ${compiler_args}<br class=""> -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}<br class="">+ -DLLVM_CONFIG_PATH=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-config<br class="">+ -DLLVM_LIBRARY_OUTPUT_INTDIR=${LLVM_LIBRARY_OUTPUT_INTDIR}<br class="">+ -DLLVM_RUNTIME_OUTPUT_INTDIR=${LLVM_RUNTIME_OUTPUT_INTDIR}<br class="">+ -DLLVM_LIBDIR_SUFFIX=${LLVM_LIBDIR_SUFFIX}<br class="">+ -DLLVM_ENABLE_WERROR=${LLVM_ENABLE_WERROR}<br class="">+ -DPACKAGE_VERSION=${PACKAGE_VERSION}<br class="">+ -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}<br class="">+ -DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">How did you decide which variables need to be passed through like this?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">The set seems somewhat arbitrary, but I may be missing something</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">obvious.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>I’m trying to define a standardized interface for runtime projects. If you look in the clang/runtimes/CMakeLists.txt file there is crazy handling to make this work for compiler-rt. This is my stab at trying to standardize that so that it could be applied to all runtime projects.</div><div><br class=""></div><div>Making this work for compiler-rt did require a small patch to compiler-rt’s build system (r271749). I expect similar patches will be needed for other runtime projects too. This will need to be taken into account in any migration plan.</div><div><br class=""></div><div>If you want I can go through each one and explain why I included them, but the general idea is that the CMAKE_* options are needed to pass through expected settings or deal with non-standard system configurations (like your build tool not being in PATH). The LLVM_* variables are my attempt at defining a standard interface for runtime projects so that their build behavior will seem as if they are in-tree configured projects. This makes it so the clang in your build tree will properly find the runtime libraries.</div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""> ${ARG_CMAKE_ARGS}<br class=""> ${PASSTHROUGH_VARIABLES}<br class=""> INSTALL_COMMAND ""<br class="">@@ -152,7 +165,7 @@<br class=""> ExternalProject_Add_Step(${name} force-rebuild<br class=""> COMMAND ${run_build}<br class=""> COMMENT "Forcing rebuild of ${name}"<br class="">- DEPENDEES configure clean<br class="">+ DEPENDEES configure ${always_clean}<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I'm not sure I understand what this does. If I had to guess I'd say that</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">when ALWAYS_CLEAN is passed the rebuild of the external project always</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">invokes clean first, but if it's not passed we'll just invoke the</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">external build and allow it to be incremental if appropriate. Is that</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">right?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>The ALWAYS_CLEAN functionality is used by the test-suite. It is so that the test-suite target always rebuilds everything. Your guess is correct for how it works.</div><div><br class=""></div><div>-Chris</div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""> DEPENDS ${ALWAYS_REBUILD} ${ARG_DEPENDS} ${TOOLCHAIN_BINS}<br class=""> ${cmake_3_4_USES_TERMINAL} )<br class=""> endif()<br class="">Index: CMakeLists.txt<br class="">===================================================================<br class="">--- CMakeLists.txt<br class="">+++ CMakeLists.txt<br class="">@@ -720,6 +720,8 @@<br class=""> add_subdirectory(tools)<br class="">endif()<br class=""><br class="">+add_subdirectory(runtimes)<br class="">+<br class="">if( LLVM_INCLUDE_EXAMPLES )<br class=""> add_subdirectory(examples)<br class="">endif()<br class="">@@ -730,7 +732,8 @@<br class=""> llvm_ExternalProject_Add(test-suite ${LLVM_MAIN_SRC_DIR}/projects/test-suite<br class=""> USE_TOOLCHAIN<br class=""> EXCLUDE_FROM_ALL<br class="">- NO_INSTALL)<br class="">+ NO_INSTALL<br class="">+ ALWAYS_CLEAN)<br class=""> endif()<br class=""> add_subdirectory(test)<br class=""> add_subdirectory(unittests)</blockquote></div></blockquote></div><br class=""></div></body></html>