[libc++abi] Use target_compile_options to pass LIBCXXABI_ADDITIONAL_COMPILE_FLAGS

[ldionne]

We use target_compile_options to pass the libc++ variant of this flag, so we should be consistent for libc++abi. This is actually not only a matter of consistency: target_compile_options handles duplicate CMake options in a certain way (it removes duplicates but has an escape hatch using the "SHELL:" prefix), and it is important for both libc++ and libc++abi options to be handled in the same way.

[llvmbot]

@llvm/pr-subscribers-libcxxabi

Author: Louis Dionne (ldionne)

Changes

We use target_compile_options to pass the libc++ variant of this flag, so we should be consistent for libc++abi. This is actually not only a matter of consistency: target_compile_options handles duplicate CMake options in a certain way (it removes duplicates but has an escape hatch using the "SHELL:" prefix), and it is important for both libc++ and libc++abi options to be handled in the same way.

---

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

2 Files Affected:

• (modified) libcxxabi/CMakeLists.txt (-1)
• (modified) libcxxabi/src/CMakeLists.txt (+2)

diff --git a/libcxxabi/CMakeLists.txt b/libcxxabi/CMakeLists.txt
index 52ba52f3439fb..43400c6e8d9af 100644
--- a/libcxxabi/CMakeLists.txt
+++ b/libcxxabi/CMakeLists.txt
@@ -244,7 +244,6 @@ if (${CMAKE_SYSTEM_NAME} MATCHES "AIX")
   add_flags_if_supported("-mdefault-visibility-export-mapping=explicit")
   set(CMAKE_AIX_EXPORT_ALL_SYMBOLS OFF)
 endif()
-add_compile_flags("${LIBCXXABI_ADDITIONAL_COMPILE_FLAGS}")
 add_library_flags("${LIBCXXABI_ADDITIONAL_LIBRARIES}")
 
 # Configure compiler. Must happen after setting the target flags.
diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index c54ced4dc3ea8..d6fcd72dcb1bd 100644
--- a/libcxxabi/src/CMakeLists.txt
+++ b/libcxxabi/src/CMakeLists.txt
@@ -182,6 +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}")
 
 if (LIBCXXABI_ENABLE_SHARED)
   add_library(cxxabi_shared SHARED)
@@ -261,6 +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}")
 
 if(LIBCXXABI_HERMETIC_STATIC_LIBRARY)
   target_add_compile_flags_if_supported(cxxabi_static_objects PRIVATE -fvisibility=hidden)

[zibi2]

Unfortunately this breaks our buids which use LIBCXXABI_ADDITIONAL_COMPILE_FLAGS.

The set_target_properties() sets COMPILE_OPTIONS which add these flags libcxx but intended to be libcxxabi flags only.
I believe this is beause libcxx depends on building libcxxabi first and the options set during libcxxabi build are also used for libcxx. Prior to this commit they were added only to LIBCXXABI_COMPILE_FLAGS using this macro:

macro(add_compile_flags)
  foreach(f ${ARGN})
    list(APPEND LIBCXXABI_COMPILE_FLAGS ${f})
  endforeach()
endmacro()

@ldionne Can this be reverted back until we find an alternative way to separate LIBCXXABI_COMPILE_FLAGS from LIBCXX_COMPILE_FLAGS?

[ldionne]

Reverting this patch will break us :-). I think we can find a way to fix this forward though. It seems like these flags should be added as PRIVATE instead. Does the following patch fix your issues?

diff --git a/libcxxabi/src/CMakeLists.txt b/libcxxabi/src/CMakeLists.txt
index d6fcd72dcb1b..c1a7bcb14eb1 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]

Reverting this patch will break us :-). I think we can find a way to fix this forward though. It seems like these flags should be added as PRIVATE instead. Does the following patch fix your issues?

Yes, it does thank you for the suggestion. The PRIVATE parameter fixed the issue. I created new PR for this in #97608