Skip to content

[libc] Fix startup utilities failing to install in full build mode #82522

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.

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
merged 3 commits into from
Feb 21, 2024

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Feb 21, 2024

Summary:
Currently, doing ninja install will fail in fullbuild mode due to the
startup utilities not being built by default. This was hidden previously
by the fact that if tests were run, it would build the startup utilities
and thus they would be present.

This patch solves this issue by making the libc-startup target a
dependncy on the final library. Furthermore we simply factor out the
library install directory into the base CMake directory next to the
include directory handling. This change makes the crt files get
installed in lib/x86_64-unknown-linu-gnu instead of just lib.

This fixes an error I had where doing a runtimes failed to install its
libraries because the install step always errored.

@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2024

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
Currently, doing ninja install will fail in fullbuild mode due to the
startup utilities not being built by default. This was hidden previously
by the fact that if tests were run, it would build the startup utilities
and thus they would be present.

This patch solves this issue by making the libc-startup target a
dependncy on the final library. Furthermore we simply factor out the
library install directory into the base CMake directory next to the
include directory handling. This change makes the crt files get
installed in lib/x86_64-unknown-linu-gnu instead of just lib.

This fixes an error I had where doing a runtimes failed to install its
libraries because the install step always errored.


Full diff: https://github.com/llvm/llvm-project/pull/82522.diff

3 Files Affected:

  • (modified) libc/CMakeLists.txt (+9)
  • (modified) libc/lib/CMakeLists.txt (+1-9)
  • (modified) libc/startup/linux/CMakeLists.txt (+1-1)
diff --git a/libc/CMakeLists.txt b/libc/CMakeLists.txt
index 3d775736616745..616beae13d9aaa 100644
--- a/libc/CMakeLists.txt
+++ b/libc/CMakeLists.txt
@@ -225,6 +225,15 @@ else()
   set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR})
 endif()
 
+if(LIBC_TARGET_TRIPLE)
+  set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LIBC_TARGET_TRIPLE})
+elseif(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT LIBC_GPU_BUILD)
+  set(LIBC_INSTALL_LIBRARY_DIR
+      lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE})
+else()
+  set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX})
+endif()
+
 if(LIBC_TARGET_ARCHITECTURE_IS_GPU)
   include(prepare_libc_gpu_build)
   set(LIBC_ENABLE_UNITTESTS OFF)
diff --git a/libc/lib/CMakeLists.txt b/libc/lib/CMakeLists.txt
index af7ef2de93dd41..99f8c6a8be9da4 100644
--- a/libc/lib/CMakeLists.txt
+++ b/libc/lib/CMakeLists.txt
@@ -35,19 +35,11 @@ foreach(archive IN ZIP_LISTS
   )
   if(LLVM_LIBC_FULL_BUILD)
     target_link_libraries(${archive_1} PUBLIC libc-headers)
+    add_dependencies(${archive_1} libc-startup)
   endif()
   list(APPEND added_archive_targets ${archive_1})
 endforeach()
 
-if(LIBC_TARGET_TRIPLE)
-  set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX}/${LIBC_TARGET_TRIPLE})
-elseif(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT LIBC_GPU_BUILD)
-  set(LIBC_INSTALL_LIBRARY_DIR
-      lib${LLVM_LIBDIR_SUFFIX}/${LLVM_DEFAULT_TARGET_TRIPLE})
-else()
-  set(LIBC_INSTALL_LIBRARY_DIR lib${LLVM_LIBDIR_SUFFIX})
-endif()
-
 install(
   TARGETS ${added_archive_targets}
   ARCHIVE DESTINATION ${LIBC_INSTALL_LIBRARY_DIR}
diff --git a/libc/startup/linux/CMakeLists.txt b/libc/startup/linux/CMakeLists.txt
index 39bcca9cdba9fe..3837737d478a2a 100644
--- a/libc/startup/linux/CMakeLists.txt
+++ b/libc/startup/linux/CMakeLists.txt
@@ -131,7 +131,7 @@ foreach(target IN LISTS startup_components)
   set(fq_target_name libc.startup.linux.${target})
   add_dependencies(libc-startup ${fq_target_name})
   install(FILES $<TARGET_OBJECTS:${fq_target_name}>
-          DESTINATION ${CMAKE_INSTALL_LIBDIR}
+    DESTINATION ${LIBC_INSTALL_LIBRARY_DIR}
           RENAME $<TARGET_PROPERTY:${fq_target_name},OUTPUT_NAME>
           COMPONENT libc)
 endforeach()

Summary:
Currently, doing `ninja install` will fail in fullbuild mode due to the
startup utilities not being built by default. This was hidden previously
by the fact that if tests were run, it would build the startup utilities
and thus they would be present.

This patch solves this issue by making the `libc-startup` target a
dependncy on the final library. Furthermore we simply factor out the
library install directory into the base CMake directory next to the
include directory handling. This change makes the `crt` files get
installed in `lib/x86_64-unknown-linu-gnu` instead of just `lib`.

This fixes an error I had where doing a runtimes failed to install its
libraries because the install step always errored.
@jhuber6 jhuber6 force-pushed the FixFullBuildInstall branch from 5b933fc to 2dba88c Compare February 21, 2024 20:14
@jhuber6
Copy link
Contributor Author

jhuber6 commented Feb 21, 2024

Also realized that actually installing the startup utilities breaks compiling with the compiler. I remember we previously hid these behind install-libc or something. So we likely want to move the installation off of the standard target altogether.

@@ -225,6 +225,15 @@ else()
set(LIBC_INSTALL_INCLUDE_DIR ${CMAKE_INSTALL_INCLUDEDIR})
endif()

if(LIBC_TARGET_TRIPLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment a bit about the logic where (i.e. why checking TARGET_TRIPLE)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the existing logic that's been moved, so it's not related to the patch. But LIBC_TARGET_TRIPLE is set whenever building using https://libc.llvm.org/full_cross_build.html#runtimes-cross-build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is to install these things in subdirectories, like libcxx and libc end up in lib/x86_64-unknown-linux-gnu.

@@ -35,19 +35,11 @@ foreach(archive IN ZIP_LISTS
)
if(LLVM_LIBC_FULL_BUILD)
target_link_libraries(${archive_1} PUBLIC libc-headers)
add_dependencies(${archive_1} libc-startup)
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any fullbuild targets we support that don't have startup code yet? (E.g. risc-v linux?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think linux and gpu are the only ones with startup code currently. But good catch, should be if TARGET.

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

LGTM, hopefully we can eventually remove this from EXCLUDE_FROM_ALL but that will probably take a while

@jhuber6 jhuber6 merged commit 049e142 into llvm:main Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants