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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/tools/llvm-libgcc/CMakeLists.txt:7
+  project(LLVM_LIBGCC)
+  if(NOT LLVM_LIBGCC_LIBUNWIND_DIR)
+    message(FATAL_ERROR
----------------
Perhaps we can default the path since the layout is now fairly uniform due to the merged repository layout.


================
Comment at: llvm/tools/llvm-libgcc/CMakeLists.txt:36
+set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/lib${LLVMLIB_DIR_SUFFIX}")
+set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/lib${LLVMLIB_DIR_SUFFIX}")
+
----------------
I think that `bin` rather than `lib` is appropriate.  The `CMAKE_RUNTIME_OUTPUT_DIRECTORY` is used for the executables and dlls on Windows.


================
Comment at: llvm/tools/llvm-libgcc/FindCompilerRT.cmake:1
+include(FindPackageHandleStandardArgs)
+include(CheckLibraryExists)
----------------
I think that this is not really the way to do this.  This should follow the standard CMake expectations around the behaviour of `Find*.cmake`.  Alternatively, you could name this something else and completely elide the `find_*` usage.


================
Comment at: llvm/tools/llvm-libgcc/FindCompilerRT.cmake:91
+
+if(NOT CompilerRT_FOUND)
+  find_external_compiler_rt()
----------------
This should be hoisted into the top level CMake.  The `FindCompilerRT.cmake` should only search for the prebuilt version.


================
Comment at: llvm/tools/llvm-libgcc/FindLLVMLibunwind.cmake:1
+include(FindPackageHandleStandardArgs)
+
----------------
Similar


================
Comment at: llvm/tools/llvm-libgcc/lib/CMakeLists.txt:17
+    "-o" "${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver"
+)
+
----------------
`add_custom_command` by itself is difficult to use, please create a target to track the output so that dependencies can be wired properly.


================
Comment at: llvm/tools/llvm-libgcc/lib/CMakeLists.txt:30
+  unwind_shared PRIVATE
+  -Wl,-nostdlib
+  -Wl,--whole-archive
----------------
Is there a reason to not use `-nostdlib` as a driver level option?


================
Comment at: llvm/tools/llvm-libgcc/lib/CMakeLists.txt:33
+  -Wl,${LLVM_LIBGCC_SYSROOT}/lib${LLVMLIB_DIR_SUFFIX}/${LLVM_LIBGCC_UNWIND_STATIC}
+  -Wl,${LLVM_LIBGCC_SYSROOT}/lib${LLVMLIB_DIR_SUFFIX}/${LLVM_LIBGCC_COMPILER_RT}
+  -Wl,--version-script,${LLVM_LIBGCC_GCC_S_VER}
----------------
Is there a guarantee that we are not building these libraries?  If we are building these libraries, I think that it would be better to use the target objects (i.e. `$<TARGET_OBJECTS:unwind>` rather than the `--whole-archive`, `--no-whole-archive` path.


================
Comment at: llvm/tools/llvm-libgcc/lib/CMakeLists.txt:34
+  -Wl,${LLVM_LIBGCC_SYSROOT}/lib${LLVMLIB_DIR_SUFFIX}/${LLVM_LIBGCC_COMPILER_RT}
+  -Wl,--version-script,${LLVM_LIBGCC_GCC_S_VER}
+  -Wl,--no-whole-archive
----------------
I think that this should be outside of the `-(`, `-)`.


================
Comment at: llvm/tools/llvm-libgcc/lib/CMakeLists.txt:38
+check_library_exists(m sin "" LLVM_LIBGCC_HAS_LIBM)
+check_library_exists(c printf "" LLVM_LIBGCC_HAS_LIBC)
+target_link_libraries(
----------------
This seems like an odd check.  I think that we should drop it, libc is required and unless you are suggesting that it is possible to have a freestanding build of this library, we should assume that it is available.  (I think that it is still problematic for non-Unix platforms due to spelling but we can deal with that if/when necessary as libgcc is not something that really applies there).


================
Comment at: llvm/tools/llvm-libgcc/lib/CMakeLists.txt:55
+  COMMAND ${CMAKE_COMMAND} -E create_symlink
+    "libunwind.so" "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/libgcc_s.so")
+add_custom_target(synth_libgcc_s1 ALL
----------------
I would rather use `${CMAKE_SHARED_LIBRARY_PREFIX}` and `${CMAKE_SHARED_LIBRARY_SUFFIX}`. rather than `lib` and `.so`.


================
Comment at: llvm/tools/llvm-libgcc/lib/CMakeLists.txt:59
+  COMMAND ${CMAKE_COMMAND} -E create_symlink
+    "libunwind.so.1" "${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/libgcc_s.so.1")
+
----------------
I think that the versioned symlinks should point to the same names not to the peers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108416