Skip to content

[llvm-libgcc][CMake] Refactor llvm-libgcc #65455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MaskRay merged 8 commits into from
Sep 19, 2023
Merged

[llvm-libgcc][CMake] Refactor llvm-libgcc #65455

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eb938ef
[llvm-libgcc][CMake] Fix broken libgcc_s.so
ur4t Aug 17, 2023
1b8a9ba
[llvm-libgcc][CMake] Refactor llvm-libgcc CMakeLists
ur4t Aug 19, 2023
c790fd0
[llvm-libgcc][CMake] Fix mismatched target names of llvm-libgcc
ur4t Sep 7, 2023
358ff00
[llvm-libgcc][CMake] Refuse improper COMPILER_RT_BUILD_BUILTINS
ur4t Sep 7, 2023
6f7088b
[llvm-libgcc][CMake] Fix broken component llvm-libgcc
ur4t Sep 8, 2023
b61aed7
[llvm-libgcc][CMake] Restore to inhibit co-existence of llvm-libgcc a…
ur4t Sep 8, 2023
656d900
[llvm-libgcc][CMake] Remove uncessary dependency on compiler-rt or li…
ur4t Sep 8, 2023
60eccd0
[llvm-libgcc][CMake] Improve standalone build for llvm-libgcc
ur4t Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 110 additions & 18 deletions llvm-libgcc/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@
cmake_minimum_required(VERSION 3.20.0)
#===============================================================================
# Setup Project
#===============================================================================

if (NOT IS_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/../libunwind")
message(FATAL_ERROR "llvm-libgcc requires being built in a monorepo layout with libunwind available")
endif()
cmake_minimum_required(VERSION 3.20.0)

set(LLVM_COMMON_CMAKE_UTILS "${CMAKE_CURRENT_SOURCE_DIR}/../cmake")

list(APPEND CMAKE_MODULE_PATH
# Add path for custom modules
list(INSERT CMAKE_MODULE_PATH 0
"${CMAKE_CURRENT_SOURCE_DIR}/cmake"
"${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules"
"${CMAKE_CURRENT_SOURCE_DIR}/../runtimes/cmake/Modules"
"${LLVM_COMMON_CMAKE_UTILS}"
"${LLVM_COMMON_CMAKE_UTILS}/Modules"
)

project(llvm-libgcc LANGUAGES C CXX ASM)
Copy link
Contributor

@cjdb cjdb Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being removed?

Copy link
Contributor Author

@ur4t ur4t Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libunwind/CMakeLists.txt does not contains a project(libunwind LANGUAGES C CXX ASM). To keep consistent with libunwind/CMakeLists.txt, project(llvm-libgcc LANGUAGES C CXX ASM) is not added.

Copy link
Contributor

@cjdb cjdb Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compiler-rt does have it, and I'd prefer to keep this regardless.

Copy link
Contributor Author

@ur4t ur4t Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now llvm-libgcc use the same approach as compiler-rt in 60eccd0.

Copy link
Contributor

@cjdb cjdb Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following what you mean, sorry. Since this is the driver for compiler-rt and libunwind, it makes sense to me for this project to indicate this information.

Copy link
Contributor Author

@ur4t ur4t Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compiler-rt/CMakeLists.txt detects whether compiler-rt is built as a subproject of runtimes or a standalone project. In standalone mode it sets options to detect llvm and declares the project name with project(CompilerRT C CXX ASM).

Now llvm-libgcc/CMakeLists.txt does the same: in standalone mode, it tells added compiler-rt that we are not built as a subproject of runtimes and we have to detect llvm ourselves.

Here "standalone" means cmake -B build -S compiler-rt or cmake -B build -S llvm-libgcc, not cmake -B build -S runtimes nor cmake -B build -S llvm. Even in the whole llvm-project source tree, directly using compiler-rt/CMakeLists.txt or llvm-libgcc/CMakeLists.txt is "standalone".

If included by other CMakeLists.txt, runtimes/CMakeLists.txt for example, it does not matter whether there is a project(CompilerRT C CXX ASM) or project(llvm-libgcc LANGUAGES C CXX ASM) declaration actually performed. To keep consistent with compiler-rt, llvm-libgcc/CMakeLists.txt is adjusted correspondingly in 60eccd0.

Correspondingly lines from compiler-rt/CMakeLists.txt :

# Check if compiler-rt is built as a standalone project.
if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR COMPILER_RT_STANDALONE_BUILD)
  project(CompilerRT C CXX ASM)
  set(COMPILER_RT_STANDALONE_BUILD TRUE)
  set_property(GLOBAL PROPERTY USE_FOLDERS ON)
endif()

Correspondingly lines in llvm-libgcc/CMakeLists.txt:

# Check if llvm-libgcc is built as a standalone project
if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR LLVM_LIBGCC_STANDALONE_BUILD)
  project(llvm-libgcc LANGUAGES C CXX ASM)
  set(COMPILER_RT_STANDALONE_BUILD ON)
  set_property(GLOBAL PROPERTY USE_FOLDERS ON)
  set(LLVM_LIBGCC_COMPILER_RT_BINARY_DIR "compiler-rt")
  set(LLVM_LIBGCC_LIBUNWIND_BINARY_DIR "libunwind")
else()
  set(LLVM_LIBGCC_COMPILER_RT_BINARY_DIR "../compiler-rt")
  set(LLVM_LIBGCC_LIBUNWIND_BINARY_DIR "../libunwind")
endif()

And I am a bit confused how should we indicate "this is the driver for compiler-rt and libunwind". It is completely acceptable to perform project(llvm-libgcc LANGUAGES C CXX ASM) declaration unconditionally, but does it makes sense in this scenario?

Copy link
Contributor

@cjdb cjdb Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, thanks for highlighting the change to llvm-libgcc/CMakeLists.txt, which I'd missed.

set(LLVM_LIBGCC_LIBUNWIND_PATH "${CMAKE_CURRENT_LIST_DIR}/../libunwind"
CACHE PATH "Specify path to libunwind source.")
set(LLVM_LIBGCC_COMPILER_RT_PATH "${CMAKE_CURRENT_LIST_DIR}/../compiler-rt"
CACHE PATH "Specify path to compiler-rt source.")

include(GNUInstallDirs)

if(NOT LLVM_LIBGCC_EXPLICIT_OPT_IN)
message(FATAL_ERROR
Expand All @@ -25,18 +32,103 @@ if(NOT LLVM_LIBGCC_EXPLICIT_OPT_IN)
"to your CMake invocation and try again.")
endif()

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/lib${LLVMLIB_DIR_SUFFIX}")
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/lib${LLVMLIB_DIR_SUFFIX}")
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/bin")
if(HAVE_COMPILER_RT AND (NOT COMPILER_RT_BUILD_BUILTINS OR COMPILER_RT_BUILTINS_HIDE_SYMBOLS))
message(FATAL_ERROR
"llvm-libgcc requires compiler-rt builtins with default symbol visibility, "
"add -DCOMPILER_RT_BUILD_BUILTINS=Yes -DCOMPILER_RT_BUILTINS_HIDE_SYMBOLS=No "
"to your CMake invocation and try again.")
endif()

#===============================================================================
# Configure System
#===============================================================================

if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
set(LLVM_LIBGCC_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${LLVM_DEFAULT_TARGET_TRIPLE})
set(LLVM_LIBGCC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE} CACHE PATH
"Path where built llvm-libgcc libraries should be installed.")
if(LIBCXX_LIBDIR_SUBDIR)
string(APPEND LLVM_LIBGCC_LIBRARY_DIR /${LLVM_LIBGCC_LIBDIR_SUBDIR})
string(APPEND LLVM_LIBGCC_INSTALL_LIBRARY_DIR /${LLVM_LIBGCC_LIBDIR_SUBDIR})
endif()
else()
if(LLVM_LIBRARY_OUTPUT_INTDIR)
set(LLVM_LIBGCC_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
else()
set(LLVM_LIBGCC_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib${LLVM_LIBGCC_LIBDIR_SUFFIX})
endif()
set(LLVM_LIBGCC_INSTALL_LIBRARY_DIR lib${LLVM_LIBGCC_LIBDIR_SUFFIX} CACHE PATH
"Path where built llvm-libgcc libraries should be installed.")
endif()

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${LLVM_LIBGCC_LIBRARY_DIR})
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${LLVM_LIBGCC_LIBRARY_DIR})
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${LLVM_LIBGCC_LIBRARY_DIR})

#===============================================================================
# Configure System
#===============================================================================

set(COMPILER_RT_BUILD_BUILTINS ON)
set(COMPILER_RT_BUILTINS_HIDE_SYMBOLS OFF)

set(COMPILER_RT_BUILD_BUILTINS On)
set(COMPILER_RT_BUILTINS_HIDE_SYMBOLS Off)
add_subdirectory("../compiler-rt" "compiler-rt")
if(NOT HAVE_COMPILER_RT)
Copy link
Contributor

@cjdb cjdb Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and libunwind) need to be unconditional, since you shouldn't be building compiler-rt and libunwind independently when building llvm-libgcc.

Copy link
Contributor Author

@ur4t ur4t Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are added unconditionally. HAVE_COMPILER_RT and HAVE_LIBUNWIND are set by runtimes/CMakeLists.txt if LLVM_ENABLE_RUNTIMES contains compiler-rt and libunwind. These checks are added to avoid target clashes when they are explicitly specified by users.

Copy link
Contributor

@cjdb cjdb Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design is that you should not be adding compiler-rt or libunwind to LLVM_ENABLE_RUNTIMES. Why is this being changed?

Copy link
Contributor Author

@ur4t ur4t Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was changed, and now it is restored in b61aed7.

I tried to make them co-exist happily and found that it is quite hard to set necessary options without further modifying the order of runtime projects in runtimes/CMakeLists.txt.

This finding is documented in the commit message to explain why we chose this approach.

add_subdirectory(${LLVM_LIBGCC_COMPILER_RT_PATH} "../compiler-rt")
endif()

set(LIBUNWIND_ENABLE_STATIC ON)
set(LIBUNWIND_ENABLE_SHARED ON)
set(LIBUNWIND_USE_COMPILER_RT ON)

if(NOT HAVE_LIBUNWIND)
add_subdirectory(${LLVM_LIBGCC_LIBUNWIND_PATH} "../libunwind")
endif()

add_custom_target(gcc_s.ver
SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/gcc_s.ver.in
COMMAND ${CMAKE_C_COMPILER} -E
-xc ${CMAKE_CURRENT_SOURCE_DIR}/gcc_s.ver.in
-o ${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver
)

add_dependencies(unwind_shared gcc_s.ver)

construct_compiler_rt_default_triple()

target_link_options(unwind_shared PUBLIC
-Wl,--version-script,${CMAKE_CURRENT_BINARY_DIR}/gcc_s.ver
)

target_link_libraries(unwind_shared PUBLIC
$<TARGET_OBJECTS:clang_rt.builtins-${COMPILER_RT_DEFAULT_TARGET_ARCH}>
m
)

#===============================================================================
# Install Symlinks
#===============================================================================

get_compiler_rt_install_dir(${COMPILER_RT_DEFAULT_TARGET_ARCH} install_dir_builtins)
string(REGEX REPLACE "^lib/" "" install_dir_builtins "${install_dir_builtins}")
string(FIND "${install_dir_builtins}" "clang" install_path_contains_triple)
if(install_path_contains_triple EQUAL -1)
set(builtins_suffix "-${COMPILER_RT_DEFAULT_TARGET_ARCH}")
else()
string(PREPEND install_dir_builtins "../")
endif()
set(LLVM_LIBGCC_COMPILER_RT ${install_dir_builtins}/libclang_rt.builtins${builtins_suffix}.a)

set(LIBUNWIND_ENABLE_STATIC On)
set(LIBUNWIND_ENABLE_SHARED Off)
set(LIBUNWIND_HAS_COMMENT_LIB_PRAGMA Off)
set(LIBUNWIND_USE_COMPILER_RT On)
add_subdirectory("../libunwind" "libunwind")
add_custom_target(llvm-libgcc ALL
DEPENDS unwind_shared
COMMAND ${CMAKE_COMMAND} -E create_symlink ${LLVM_LIBGCC_COMPILER_RT} libgcc.a
COMMAND ${CMAKE_COMMAND} -E create_symlink libunwind.a libgcc_eh.a
COMMAND ${CMAKE_COMMAND} -E create_symlink libunwind.so libgcc_s.so.1.0
COMMAND ${CMAKE_COMMAND} -E create_symlink libgcc_s.so.1.0 libgcc_s.so.1
COMMAND ${CMAKE_COMMAND} -E create_symlink libgcc_s.so.1 libgcc_s.so
)

add_subdirectory(lib)
foreach(VAR libgcc.a libgcc_eh.a libgcc_s.so.1.0 libgcc_s.so.1 libgcc_s.so)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${VAR}
DESTINATION ${LLVM_LIBGCC_INSTALL_LIBRARY_DIR}
COMPONENT llvm-libgcc)
endforeach()
17 changes: 9 additions & 8 deletions llvm-libgcc/docs/LLVMLibgcc.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,15 @@ build with these compiler-rt symbols exposed.
-DLLVM_LIBGCC_EXPLICIT_OPT_IN=Yes
$ ninja -C build-primary install

It's very important to notice that neither ``compiler-rt``, nor ``libunwind``,
are listed in ``LLVM_ENABLE_RUNTIMES``. llvm-libgcc makes these subprojects, and
adding them to this list will cause you problems due to there being duplicate
targets. As such, configuring the runtimes build will reject explicitly mentioning
either project with ``llvm-libgcc``.

To avoid issues when building with ``-DLLVM_ENABLE_RUNTIMES=all``, ``llvm-libgcc``
is not included, and all runtimes targets must be manually listed.
Enabling llvm-libgcc now implies ``compiler-rt`` and ``libunwind`` are enabled,
but it's okay to explicitly enable ``compiler-rt`` and ``libunwind``. When
``compiler-rt`` is explicitly enabled, ``-DCOMPILER_RT_BUILTINS_HIDE_SYMBOLS=No``
is required to keep symbols in ``compiler-rt`` visible for linking together
with ``libunwind``.

As mentioned above, llvm-libgcc is not for the casual user, thus it is not
included in runtimes with setting ``-DLLVM_ENABLE_RUNTIMES=all``, and
``-DLLVM_LIBGCC_EXPLICIT_OPT_IN=Yes`` is required as double insurance.

## Verifying your results

Expand Down
0 llvm-libgcc/lib/gcc_s.ver → llvm-libgcc/gcc_s.ver.in
View file
Open in desktop
File renamed without changes.
86 changes: 0 additions & 86 deletions llvm-libgcc/lib/CMakeLists.txt
View file
Open in desktop

This file was deleted.

Empty file removed llvm-libgcc/lib/blank.c
View file
Open in desktop
Copy link
Contributor

@cjdb cjdb Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy you're able to delete this file.

Copy link
Contributor Author

@ur4t ur4t Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the refactored approach, we modify libunwind.so added in libunwind directly and no custom shared library is required, so the dummy source file can be deleted.

Empty file.
17 changes: 0 additions & 17 deletions runtimes/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,23 +212,6 @@ if(LLVM_INCLUDE_TESTS)
umbrella_lit_testsuite_begin(check-runtimes)
endif()

# llvm-libgcc incorporates both compiler-rt and libunwind as subprojects with very
# specific flags, which causes clashes when they're independently built too.
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()

# We do this in two loops so that HAVE_* is set for each runtime before the
# other runtimes are added.
foreach(entry ${runtimes})
Expand Down