Skip to content
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

[Fuchsia][cmake] Avoid referencing cxx_shared in compiler-rt #112257

Conversation

ilovepi
Copy link
Contributor

@ilovepi ilovepi commented Oct 14, 2024

After #80007 Fuchsia builds are
now always building cxx_shared for arm64 and x64 Linux. Ultimately, this
is because the LIBCXX_ENABLE_SHARED is not used in compiler-rt to select
the correct libc++ target, and because cxx_shared is now always defined,
it is selected as a dependency when building runtimes tests.

Created using spr 1.3.4
@llvmbot
Copy link
Member

llvmbot commented Oct 14, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Paul Kirth (ilovepi)

Changes

After #80007 Fuchsia builds are
now always building cxx_shared for arm64 and x64 Linux. Ultimately, this
is because the LIBCXX_ENABLE_SHARED is not used in compiler-rt to select
the correct libc++ target, and because cxx_shared is now always defined,
it is selected as a dependency when building runtimes tests.


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

2 Files Affected:

  • (modified) compiler-rt/CMakeLists.txt (+3-3)
  • (modified) compiler-rt/test/fuzzer/CMakeLists.txt (+3-4)
diff --git a/compiler-rt/CMakeLists.txt b/compiler-rt/CMakeLists.txt
index deb6994f481854..98c28556755525 100644
--- a/compiler-rt/CMakeLists.txt
+++ b/compiler-rt/CMakeLists.txt
@@ -616,7 +616,7 @@ if(COMPILER_RT_USE_LLVM_UNWINDER)
   if (COMPILER_RT_ENABLE_STATIC_UNWINDER)
     list(APPEND COMPILER_RT_UNWINDER_LINK_LIBS "$<TARGET_LINKER_FILE:unwind_static>")
   else()
-    list(APPEND COMPILER_RT_UNWINDER_LINK_LIBS "$<TARGET_LINKER_FILE:$<IF:$<TARGET_EXISTS:unwind_shared>,unwind_shared,unwind_static>>")
+    list(APPEND COMPILER_RT_UNWINDER_LINK_LIBS "$<TARGET_LINKER_FILE:$<IF:$<BOOL:${LIBUNWIND_ENABLE_SHARED}>,unwind_shared,unwind_static>>")
   endif()
 endif()
 
@@ -629,7 +629,7 @@ if (COMPILER_RT_CXX_LIBRARY STREQUAL "libcxx")
   if (COMPILER_RT_STATIC_CXX_LIBRARY)
     set(COMPILER_RT_CXX_LINK_LIBS "$<TARGET_LINKER_FILE:cxx_static>")
   else()
-    set(COMPILER_RT_CXX_LINK_LIBS "$<TARGET_LINKER_FILE:$<IF:$<TARGET_EXISTS:cxx_shared>,cxx_shared,cxx_static>>")
+    set(COMPILER_RT_CXX_LINK_LIBS "$<TARGET_LINKER_FILE:$<IF:$<BOOL:${LIBCXX_ENABLE_SHARED}>,cxx_shared,cxx_static>>")
   endif()
 elseif (COMPILER_RT_CXX_LIBRARY STREQUAL "none")
   # We aren't using any C++ standard library so avoid including the default one.
@@ -671,7 +671,7 @@ if (SANITIZER_TEST_CXX_LIBNAME STREQUAL "libc++")
     if (SANITIZER_USE_STATIC_TEST_CXX)
       list(APPEND SANITIZER_TEST_CXX_LIBRARIES "$<TARGET_LINKER_FILE:cxx_static>")
     else()
-      list(APPEND SANITIZER_TEST_CXX_LIBRARIES "$<TARGET_LINKER_FILE:$<IF:$<TARGET_EXISTS:cxx_shared>,cxx_shared,cxx_static>>")
+      list(APPEND SANITIZER_TEST_CXX_LIBRARIES "$<TARGET_LINKER_FILE:$<IF:$<BOOL:${LIBCXX_ENABLE_SHARED}>,cxx_shared,cxx_static>>")
     endif()
     # We are using the in tree libc++ so avoid including the default one.
     append_list_if(COMPILER_RT_HAS_NOSTDINCXX_FLAG -nostdinc++ COMPILER_RT_UNITTEST_CFLAGS)
diff --git a/compiler-rt/test/fuzzer/CMakeLists.txt b/compiler-rt/test/fuzzer/CMakeLists.txt
index b19f52e591fc0f..982225f294d238 100644
--- a/compiler-rt/test/fuzzer/CMakeLists.txt
+++ b/compiler-rt/test/fuzzer/CMakeLists.txt
@@ -73,11 +73,10 @@ endmacro()
 
 test_fuzzer("default")
 if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
-  if(TARGET cxx_shared)
-    test_fuzzer("libc++" DEPS cxx_shared)
-  endif()
-  if(TARGET cxx_static)
+  if(LIBCXX_ENABLE_STATIC)
     test_fuzzer("static-libc++" DEPS cxx_static)
+  elseif(LIBCXX_ENABLE_SHARED)
+    test_fuzzer("libc++" DEPS cxx_shared)
   endif()
 endif()
 

ilovepi and others added 3 commits October 14, 2024 14:06
Co-authored-by: Petr Hosek <phosek@google.com>
Created using spr 1.3.4
Created using spr 1.3.4
@ilovepi ilovepi merged commit c4131cb into main Oct 14, 2024
5 of 6 checks passed
@ilovepi ilovepi deleted the users/ilovepi/spr/fuchsiacmake-avoid-referencing-cxx_shared-in-compiler-rt branch October 14, 2024 21:41
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

Does someone understand how the layering works for compiler-rt? Libc++ links against compiler-rt itself, so how can we depend on libc++ from compiler-rt?

@ilovepi
Copy link
Contributor Author

ilovepi commented Oct 15, 2024

Does someone understand how the layering works for compiler-rt? Libc++ links against compiler-rt itself, so how can we depend on libc++ from compiler-rt?

Aren't the builtins the only dependency? IIRC builtins are special in the runtimes build, and is handled that way to break the cyclic dependency in the build graph. @petrhosek can speak more to that though.

@petrhosek
Copy link
Member

Does someone understand how the layering works for compiler-rt? Libc++ links against compiler-rt itself, so how can we depend on libc++ from compiler-rt?

Aren't the builtins the only dependency? IIRC builtins are special in the runtimes build, and is handled that way to break the cyclic dependency in the build graph. @petrhosek can speak more to that though.

That's correct, we build builtins separately by using https://github.com/llvm/llvm-project/tree/main/compiler-rt/lib/builtins as the top-level CMake build directory, and then build the rest of compiler-rt as part of the runtimes build.

libc++ is currently only used by libFuzzer, ORC and XRay plus all unit tests (since it's a dependency of GoogleTest), the other runtimes in compiler-rt like sanitizers only depend on libc++abi. libFuzzer is a bit special in that it build its own copy of libc++ (in a subbuild) which is statically linked into a relocatable object.

DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
…2257)

After llvm#80007 Fuchsia builds are
now always building cxx_shared for arm64 and x64 Linux. Ultimately, this
is because the LIBCXX_ENABLE_SHARED is not used in compiler-rt to select
the correct libc++ target, and because cxx_shared is now always defined,
it is selected as a dependency when building runtimes tests.

---------

Co-authored-by: Petr Hosek <phosek@google.com>
@ldionne
Copy link
Member

ldionne commented Oct 16, 2024

Thanks for the explanation!

That's correct, we build builtins separately by using https://github.com/llvm/llvm-project/tree/main/compiler-rt/lib/builtins as the top-level CMake build directory

Would it make sense to separate this out of compiler-rt then? This sounds like a slightly brittle setup dependency-wise.

libc++ is currently only used by libFuzzer, ORC and XRay plus all unit tests (since it's a dependency of GoogleTest)

Is there a reason why libFuzzer, ORC and XRay need to explicitly mention the just-built libc++? Couldn't they use whatever standard library is provided by the compiler, which in the case of a bootstrapping build would be the just-built libc++? That would be cleaner than having subparts of compiler-rt depending directly on libc++ targets.

the other runtimes in compiler-rt like sanitizers only depend on libc++abi

Do you know what they depend on libc++abi for?

bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
…2257)

After llvm#80007 Fuchsia builds are
now always building cxx_shared for arm64 and x64 Linux. Ultimately, this
is because the LIBCXX_ENABLE_SHARED is not used in compiler-rt to select
the correct libc++ target, and because cxx_shared is now always defined,
it is selected as a dependency when building runtimes tests.

---------

Co-authored-by: Petr Hosek <phosek@google.com>
EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
…2257)

After llvm#80007 Fuchsia builds are
now always building cxx_shared for arm64 and x64 Linux. Ultimately, this
is because the LIBCXX_ENABLE_SHARED is not used in compiler-rt to select
the correct libc++ target, and because cxx_shared is now always defined,
it is selected as a dependency when building runtimes tests.

---------

Co-authored-by: Petr Hosek <phosek@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants