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

[libc++abi] Fixing up LIBCXXABI_ADDITIONAL_COMPILE_FLAGS #97608

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

zibi2
Copy link
Contributor

@zibi2 zibi2 commented Jul 3, 2024

This is the continuation of #96112 which implements proposal from Louis.
Using PRIVATE option on target_compile_options() fixes the issue of propagating the option into lib++.

@zibi2 zibi2 requested a review from a team as a code owner July 3, 2024 17:11
@zibi2 zibi2 self-assigned this Jul 3, 2024
@llvmbot llvmbot added the libc++abi libc++abi C++ Runtime Library. Not libc++. label Jul 3, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 3, 2024

@llvm/pr-subscribers-libcxxabi

Author: Zibi Sarbinowski (zibi2)

Changes

This is the continuation of #96112 which implements proposal from Louis.
Using PRIVATE option on target_compile_options() fixes the issue of propagating the option into lib++.


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

1 Files Affected:

  • (modified) libcxxabi/src/CMakeLists.txt (+2-2)
diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index d6fcd72dcb1bd..c1a7bcb14eb19 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -182,7 +182,7 @@ set_target_properties(cxxabi_shared_objects
 if (CMAKE_POSITION_INDEPENDENT_CODE OR NOT DEFINED CMAKE_POSITION_INDEPENDENT_CODE)
   set_target_properties(cxxabi_shared_objects PROPERTIES POSITION_INDEPENDENT_CODE ON) # must set manually because it's an object library
 endif()
-target_compile_options(cxxabi_shared_objects PUBLIC "${LIBCXXABI_ADDITIONAL_COMPILE_FLAGS}")
+target_compile_options(cxxabi_shared_objects PRIVATE "${LIBCXXABI_ADDITIONAL_COMPILE_FLAGS}")
 
 if (LIBCXXABI_ENABLE_SHARED)
   add_library(cxxabi_shared SHARED)
@@ -262,7 +262,7 @@ set_target_properties(cxxabi_static_objects
     CXX_STANDARD_REQUIRED OFF # TODO: Make this REQUIRED once we don't need to accommodate the LLVM documentation builders using an ancient CMake
     COMPILE_FLAGS "${LIBCXXABI_COMPILE_FLAGS}"
 )
-target_compile_options(cxxabi_static_objects PUBLIC "${LIBCXXABI_ADDITIONAL_COMPILE_FLAGS}")
+target_compile_options(cxxabi_static_objects PRIVATE "${LIBCXXABI_ADDITIONAL_COMPILE_FLAGS}")
 
 if(LIBCXXABI_HERMETIC_STATIC_LIBRARY)
   target_add_compile_flags_if_supported(cxxabi_static_objects PRIVATE -fvisibility=hidden)

@zibi2 zibi2 requested a review from ldionne July 3, 2024 17:11
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.

LGTM once CI is green.

@zibi2 zibi2 merged commit 18b575d into llvm:main Jul 5, 2024
58 checks passed
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
This is the continuation of llvm#96112 which implements proposal from Louis.
Using PRIVATE option on target_compile_options() fixes the issue of
propagating the option into lib++.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++abi libc++abi C++ Runtime Library. Not libc++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants