[libcxx-commits] [PATCH] D108416: [llvm-libgcc] initial commit

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: llvm-libgcc/CMakeLists.txt:10
+# Add path for custom modules
+list(INSERT CMAKE_MODULE_PATH 0
+  "${CMAKE_CURRENT_SOURCE_DIR}/cmake"
----------------
compnerd wrote:
> Any reason for insert rather that append?
I copied this directly from either `libunwind/CMakeLists.txt` or `runtimes/CMakeLists.txt`. Looks as if it's being prepended though?


================
Comment at: llvm-libgcc/CMakeLists.txt:36
+set(COMPILER_RT_BUILD_BUILTINS On)
+set(COMPILER_RT_BUILTINS_HIDE_SYMBOLS Off)
+add_subdirectory("../compiler-rt" "compiler-rt")
----------------
compnerd wrote:
> I tend to prefer all caps for the `On` and `Off`, but its equivalent really.
Acknowledged.


================
Comment at: llvm-libgcc/docs/LLVMLibgcc.rst:96
+
+It's very important to notice that neither `compiler-rt`, nor `llvm-libgcc`, are
+listed in `LLVM_ENABLE_RUNTIMES`. llvm-libgcc makes these subprojects, and adding
----------------
zero9178 wrote:
> > neither `compiler-rt`, nor `llvm-libgcc`, are listed in `LLVM_ENABLE_RUNTIMES`
> Is this a typo and is meant to be `libunwind` instead of `llvm-libgcc`?
Thanks!


================
Comment at: llvm-libgcc/docs/LLVMLibgcc.rst:97
+It's very important to notice that neither `compiler-rt`, nor `llvm-libgcc`, are
+listed in `LLVM_ENABLE_RUNTIMES`. llvm-libgcc makes these subprojects, and adding
+them to this list will cause you problems.
----------------
MaskRay wrote:
> I think some folks prefer that compiler-rt is added to LLVM_ENABLE_RUNTIMES. How difficult is it to make this work?
> 
> Note that current plan is that libunwind in DLLVM_ENABLE_PROJECTS will be unsupported.
compiler-rt must be listed in neither `LLVM_ENABLE_RUNTIMES`, nor `LLVM_ENABLE_PROJECTS` when llvm-libgcc is present. llvm-libgcc will take care of that for you. You can still have `LLVM_ENABLE_RUNTIMES=compiler-rt`, but not when llvm-libgcc is also in the list.


================
Comment at: llvm-libgcc/generate_version_script.py:1
+#!/usr/bin/python
+
----------------
MaskRay wrote:
> /usr/bin/env python3
> 
> Add a file-level comment what this script does.
Done, but what's the benefit of this?


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:19
+set_target_properties(
+  gcc_s_version_script PROPERTIES
+  OUTPUT_PATH "${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver")
----------------
compnerd wrote:
> I think that breaking after the `(` is a bit unnecessary:
> 
> ```
> set_target_properties(gcc_s_version_script PROPERTIES
>   OUTPUT_PATH "${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver")
> ```
> 
> Similar throughout.
I find the current way to improve readability a lot.


================
Comment at: llvm-libgcc/lib/CMakeLists.txt:56
+
+set(LLVM_LIBGCC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX} CACHE PATH "")
+install(TARGETS libgcc_s
----------------
compnerd wrote:
> Why is this cached?
I think this was user-settable at one point, but per yesterday, this entire variable is redundant.


================
Comment at: runtimes/CMakeLists.txt:181-200
+# llvm-libgcc incorporates both compiler-rt and libunwind as subprojects with very
+# specific flags, which causes clashes when they're independently built too.
+list(FIND "${runtimes}" "llvm-libgcc" RUNTIMES_HAS_LLVM_LIBGCC)
+if(NOT RUNTIMES_HAS_LLVM_LIBGCC EQUAL -1)
+  list(FIND "${runtimes}" "compiler-rt" RUNTIMES_HAS_COMPILER_RT)
+  list(FIND "${LLVM_ENABLE_PROJECTS}" "compiler-rt" RUNTIMES_PROJECTS_HAS_COMPILER_RT)
+  if(NOT RUNTIMES_HAS_COMPILER_RT EQUAL -1 OR NOT RUNTIMES_PROJECTS_HAS_COMPILER_RT EQUAL -1)
----------------
ldionne wrote:
> I think this can be simplified like this:
> 
> ```
> if("llvm-libgcc" IN_LIST runtimes)
>   if("compiler-rt" IN_LIST runtimes OR "compiler-rt" IN_LIST LLVM_ENABLE_PROJECTS)
>     message(FATAL_ERROR
>       "Attempting to build both compiler-rt and llvm-libgcc will cause irreconcilable "
>       "target clashes. Please choose one or the other, but not both.")
>   endif()
> 
>   if("libunwind" IN_LIST runtimes)
>     message(
>       FATAL_ERROR
>       "Attempting to build both libunwind and llvm-libgcc will cause irreconcilable "
>       "target clashes. Please choose one or the other, but not both.")
>   endif()
> endif()
> ```
Thanks, this is way simpler!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108416