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

[Riyaz V Puthiyapurayil via Phabricator via llvm-commits]

RVP marked 3 inline comments as done.
RVP 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}")
----------------
phosek wrote:
> 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.
I am not sure what is making it difficult to follow. Is it that the macro contains a `return()`? What if we turned the macro into a function `check_if_compiler_rt_library_exists` that sets the `...-NOTFOUND` variable and inlined the `return()` into `find_compiler_rt_library`? That way, we won't need to duplicate all that code.  I have no problem changing it the way you suggest, if you feel strongly that duplicating the code actually makes it easier to understand (but I think it is better not to duplicate).


================
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}")
----------------
phosek wrote:
> Either `COMPILER_RT_LIBRARY_${name}_${target}-NOTFOUND` or `NOTFOUND` is more idiomatic in CMake.
Will do this.


================
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
----------------
phosek wrote:
> 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.
Will do.


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

https://reviews.llvm.org/D74133