[PATCH] D41660: [cmake] Add new linux toolchain file

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In https://reviews.llvm.org/D41660#965686, @hintonda wrote:

> In https://reviews.llvm.org/D41660#965656, @smeenai wrote:
>
> >
>
>
> Cache files are preferred since they are only loaded once


Isn't that precisely the behavior needed for cross-compilation though? You want all of your CMake configuration checks (which are independent CMake configures) to load your toolchain file, which is what you get automatically (and cache files don't behave that way).

>From what I understand, the if part of the top-level `if(DEFINED SYSROOT)` is essentially functioning as a cache file to set up the stage2 build, and the else part is used as a toolchain file for that build. I think it would be cleaner to separate the two out; other cache files seem to be split out into stage1 and stage2 caches, for example (over here it would be stage1 cache and a stage2 toolchain, but the concept is similar).



================
Comment at: cmake/caches/linux-toolchain.cmake:2
+# This file is intended to support cross compiling a linux toolchain
+# on any host system, includind Darwin.
+#
----------------
Cross-compilation terminology is kinda weird, and traditionally, the "host" is actually the system the built binaries will be run on (Linux in this case), whereas the build machine is the "build" (but of course that word is super ambiguous). I think LLVM generally sticks to that terminology though, e.g. `LLVM_HOST_TRIPLE`.


================
Comment at: cmake/caches/linux-toolchain.cmake:84
+  else()
+    set(BOOTSTRAP_STAGE2_PROJECTS "clang;libcxx;libcxxabi;libunwind" CACHE STRING "" FORCE)
+  endif()
----------------
Nit: write this out as a list instead of a string with semicolons? (I know they're equivalent, but the list reads nicer IMO.)


================
Comment at: cmake/caches/linux-toolchain.cmake:88
+  # Required on non-elf hosts.
+  set(ADDITIONAL_CLANG_BOOTSTRAP_DEPS "lld;llvm-ar;llvm-ranlib" CACHE STRING "")
+
----------------
Not exactly related, but I wonder why the LLVM build needs ranlib (rather than just invoking ar appropriately).


================
Comment at: cmake/caches/linux-toolchain.cmake:102
+else()
+  set(CMAKE_SYSTEM_NAME Linux CACHE STRING "" FORCE)
+
----------------
The CMake documentation for CMAKE_SYSTEM_NAME says CMAKE_SYSTEM_VERSION should also be set when cross-compiling (though I haven't seen any ill effects from not doing so). Setting CMAKE_SYSTEM_PROCESSOR probably doesn't hurt either.


Repository:
  rC Clang

https://reviews.llvm.org/D41660