[CMake] Use llvm-ar etal for external project build on Darwin

[phosek]

Summary

This is needed for the runtimes build since libc++ and libc++abi
archive merging relies on ar to extract files out of the archive,
even on Darwin.

[smeenai]

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

LGTM barring the question about the WIN32 -> MSVC change.

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

55–56

What's the reason for this part of the change?

[phosek]

In D74107#1862126, @smeenai wrote:

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

They're affected by CMAKE_SYSTEM_NAME which is usually set for the runtimes build so it's not necessarily the host. I'm still not sure whether this condition is desirable though since most of these tools either support APPLE or WIN32 platforms, or CMake will just ignore them and use the appropriate tool for the given platform. This code predates runtimes build and my changes so I don't know what the motivation behind that condition was.

LGTM barring the question about the WIN32 -> MSVC change.

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

55–56

AFAIK these tools should be still applicable to mingw so this is more precise, but I'm fine keeping the condition as is.

[smeenai]

In D74107#1862274, @phosek wrote:

In D74107#1862126, @smeenai wrote:

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

They're affected by CMAKE_SYSTEM_NAME which is usually set for the runtimes build so it's not necessarily the host. I'm still not sure whether this condition is desirable though since most of these tools either support APPLE or WIN32 platforms, or CMake will just ignore them and use the appropriate tool for the given platform. This code predates runtimes build and my changes so I don't know what the motivation behind that condition was.

Right, but the relevant calls to llvm_ExternalProject_Add from inside llvm/runtimes/CMakeLists.txt all happen when it's being included from LLVM's CMake (i.e. it's not the configure for the actual runtimes build), so at that point CMAKE_SYSTEM_NAME will just reflect the host, right?

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

55–56

llvm-ar, llvm-nm, and llvm-objdump are potentially applicable to MSVC too :) In fact MSVC's llvm-shlib build actually uses llvm-nm: https://github.com/llvm/llvm-project/blob/03a2d0045d2c11f4601bb2cbf5cdd5389ed2d72a/llvm/tools/llvm-shlib/CMakeLists.txt#L162

[phosek]

In D74107#1862361, @smeenai wrote:

In D74107#1862274, @phosek wrote:

In D74107#1862126, @smeenai wrote:

I've never really understood this block, since the APPLE and WIN32/MSVC conditionals here will reflect the host you're building on and not your actual runtimes target, but this change itself looks fine.

They're affected by CMAKE_SYSTEM_NAME which is usually set for the runtimes build so it's not necessarily the host. I'm still not sure whether this condition is desirable though since most of these tools either support APPLE or WIN32 platforms, or CMake will just ignore them and use the appropriate tool for the given platform. This code predates runtimes build and my changes so I don't know what the motivation behind that condition was.

Right, but the relevant calls to llvm_ExternalProject_Add from inside llvm/runtimes/CMakeLists.txt all happen when it's being included from LLVM's CMake (i.e. it's not the configure for the actual runtimes build), so at that point CMAKE_SYSTEM_NAME will just reflect the host, right?

Good point, I've changed the code to instead grep for CMAKE_SYSTEM_NAME in the arguments passed to the external project.

[smeenai]

LGTM

llvm/cmake/modules/LLVMExternalProjectUtils.cmake

 #   )
 function(llvm_ExternalProject_Add name source_dir)
   cmake_parse_arguments(ARG
     "USE_TOOLCHAIN;EXCLUDE_FROM_ALL;NO_INSTALL;ALWAYS_CLEAN"
     "SOURCE_DIR"
     "CMAKE_ARGS;TOOLCHAIN_TOOLS;RUNTIME_LIBRARIES;DEPENDS;EXTRA_TARGETS;PASSTHROUGH_PREFIXES;STRIP_TOOL"
     ${ARGN})
   canonicalize_tool_name(${name} nameCanon)
+
+  foreach(arg ${ARG_CMAKE_ARGS})
+    if(arg MATCHES "^-DCMAKE_SYSTEM_NAME=")
+      string(REGEX REPLACE "^-DCMAKE_SYSTEM_NAME=(.*)$" "\\1" _cmake_system_name "${arg}")
+    endif()
+  endforeach()
+
   if(NOT ARG_TOOLCHAIN_TOOLS)
-    set(ARG_TOOLCHAIN_TOOLS clang lld)
-    if(NOT APPLE AND NOT WIN32)
-      list(APPEND ARG_TOOLCHAIN_TOOLS llvm-ar llvm-lipo llvm-ranlib llvm-nm llvm-objcopy llvm-objdump llvm-strip)
+    set(ARG_TOOLCHAIN_TOOLS clang lld llvm-ar llvm-lipo llvm-ranlib llvm-nm llvm-objdump)
+    if(NOT _cmake_system_name STREQUAL Darwin)
+      # TODO: These tools don't fully support Mach-O format yet.
+      list(APPEND ARG_TOOLCHAIN_TOOLS llvm-objcopy llvm-strip)
     endif()
   endif()
   foreach(tool ${ARG_TOOLCHAIN_TOOLS})
     if(TARGET ${tool})
       list(APPEND TOOLCHAIN_TOOLS ${tool})
       list(APPEND TOOLCHAIN_BINS $<TARGET_FILE:${tool}>)
     endif()
   endforeach()
foreach(variableName ${variableNames})
       if(variableName MATCHES "^${prefix}")
         string(REPLACE ";" "|" value "${${variableName}}")
         list(APPEND PASSTHROUGH_VARIABLES
           -D${variableName}=${value})
       endif()
     endforeach()
   endforeach()
 
-  foreach(arg ${ARG_CMAKE_ARGS})
-    if(arg MATCHES "^-DCMAKE_SYSTEM_NAME=")
-      string(REGEX REPLACE "^-DCMAKE_SYSTEM_NAME=(.*)$" "\\1" _cmake_system_name "${arg}")
-    endif()
-  endforeach()
-
   if(ARG_USE_TOOLCHAIN AND NOT CMAKE_CROSSCOMPILING)
     if(CLANG_IN_TOOLCHAIN)
       if(_cmake_system_name STREQUAL Windows)
         set(compiler_args -DCMAKE_C_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang-cl${CMAKE_EXECUTABLE_SUFFIX}
                           -DCMAKE_CXX_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang-cl${CMAKE_EXECUTABLE_SUFFIX}
                           -DCMAKE_ASM_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang-cl${CMAKE_EXECUTABLE_SUFFIX})
       else()
         set(compiler_args -DCMAKE_C_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang${CMAKE_EXECUTABLE_SUFFIX}