[PATCH] D74133: [compiler-rt] Build with correct ABI (PR38025)

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: compiler-rt/cmake/Modules/HandleCompilerRT.cmake:4
+# IF not, emit a message and quit.
+macro(check_compiler_rt_library errflag name target library_file)
+  if(errflag OR NOT EXISTS "${library_file}")
----------------
I'd consider just inlining this, I don't think a bit of code deduplication is worth the extra complexity and I find the current version more difficult to follow.


================
Comment at: compiler-rt/cmake/Modules/HandleCompilerRT.cmake:7
+    message(STATUS "Failed to find compiler-rt ${name} library for ${target}")
+    set(COMPILER_RT_LIBRARY_${name}_${target} "<not-found>" CACHE INTERNAL
+        "compiler-rt ${name} library for ${target}")
----------------
Either `COMPILER_RT_LIBRARY_${name}_${target}-NOTFOUND` or `NOTFOUND` is more idiomatic in CMake.


================
Comment at: compiler-rt/cmake/Modules/HandleCompilerRT.cmake:28
   endif()
-  get_property(SANITIZER_CXX_FLAGS CACHE CMAKE_CXX_FLAGS PROPERTY VALUE)
-  string(REPLACE " " ";" SANITIZER_CXX_FLAGS "${SANITIZER_CXX_FLAGS}")
-  list(APPEND CLANG_COMMAND ${SANITIZER_CXX_FLAGS})
-  execute_process(
+  if(NOT COMPILER_RT_LIBRARY_builtins_${target})
+    # builtins is not already in cache; so get it by calling clang
----------------
I'd use `if(NOT DEFINED COMPILER_RT_LIBRARY_builtins_${target})` as a check here to differentiate the case when variable was set from the case when variable is considered to be false by CMake.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74133/new/

https://reviews.llvm.org/D74133