[runtimes] Synchronize warnings flags between libc++/libc++abi/libunwind

[philnik]

Summary

This mostly keeps the same warning flags. The most important exceptions are -Wpedantic and -Wconversion, which are now removed from libc++abi and libunwind.

[phosek]

runtimes/cmake/Modules/WarningFlags.cmake uses target_add_compile_flags_if_supported which is defined in https://github.com/llvm/llvm-project/blob/2b2b8409e6848361577be404bd0ae47a097f0e0c/libcxx/cmake/Modules/HandleLibcxxFlags.cmake#L246. That function uses other functions defined in the same file. That file is also not explicitly included here.

I'd prefer to first move https://github.com/llvm/llvm-project/blob/2b2b8409e6848361577be404bd0ae47a097f0e0c/libcxx/cmake/Modules/HandleLibcxxFlags.cmake to runtimes/cmake/Modules and replace https://github.com/llvm/llvm-project/blob/2b2b8409e6848361577be404bd0ae47a097f0e0c/libcxxabi/cmake/Modules/HandleLibcxxabiFlags.cmake and https://github.com/llvm/llvm-project/blob/2b2b8409e6848361577be404bd0ae47a097f0e0c/libunwind/cmake/Modules/HandleLibunwindFlags.cmake.

[philnik]

Address comments

[ldionne]

Can you record in the commit message what the changes to the warnings are? It seems like the libcxxabi and libunwind warning flags were the same, so those are synced to the libcxx warnings. However, that synchronization removes -Wconversion from the libcxxabi and libunwind flags (the TODO you added). Is that all?

Thanks for the refactoring by the way, this is a great way to get where we needed to be while leaving the code better behind you.

libcxx/CMakeLists.txt

843

Here you use cxx_add_warning_flags but you have not included WarningFlags.cmake. I think that's your Windows CI issue. IDK why it works on other platforms, that's almost concerning.

runtimes/cmake/Modules/WarningFlags.cmake

69–72

FWIW I strongly suspect this is not needed anymore. Let's try to get rid of it in a different patch.

[philnik]

Try to fix CI

[philnik]

Try to fix CI

libcxx/CMakeLists.txt

if (C_SUPPORTS_COMMENT_LIB_PRAGMA)
     endif()
     if (LIBCXX_HAS_RT_LIB)
       target_compile_definitions(${target} PRIVATE -D_LIBCPP_LINK_RT_LIB)
     endif()
   endif()
   target_compile_options(${target} PUBLIC "${LIBCXX_ADDITIONAL_COMPILE_FLAGS}")
 endfunction()
 
-# Warning flags ===============================================================
-function(cxx_add_warning_flags target)
-  target_compile_definitions(${target} PUBLIC -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-  if (MSVC)
-    # -W4 is the cl.exe/clang-cl equivalent of -Wall. (In cl.exe and clang-cl,
-    # -Wall is equivalent to -Weverything in GCC style compiler drivers.)
-    target_add_compile_flags_if_supported(${target} PRIVATE -W4)
-  else()
-    target_add_compile_flags_if_supported(${target} PRIVATE -Wall)
-  endif()
-  target_add_compile_flags_if_supported(${target} PRIVATE -Wextra -W -Wwrite-strings
-                                                          -Wno-unused-parameter -Wno-long-long
-                                                          -Werror=return-type -Wextra-semi -Wundef
-                                                          -Wformat-nonliteral)
-  if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
-    target_add_compile_flags_if_supported(${target} PRIVATE
-      -Wno-user-defined-literals
-      -Wno-covered-switch-default
-      -Wno-suggest-override
-    )
-    if (LIBCXX_TARGETING_CLANG_CL)
-      target_add_compile_flags_if_supported(${target} PRIVATE
-        -Wno-c++98-compat
-        -Wno-c++98-compat-pedantic
-        -Wno-c++11-compat
-        -Wno-undef
-        -Wno-reserved-id-macro
-        -Wno-gnu-include-next
-        -Wno-gcc-compat # For ignoring "'diagnose_if' is a clang extension" warnings
-        -Wno-zero-as-null-pointer-constant # FIXME: Remove this and fix all occurrences.
-        -Wno-deprecated-dynamic-exception-spec # For auto_ptr
-        -Wno-sign-conversion
-        -Wno-old-style-cast
-        -Wno-deprecated # FIXME: Remove this and fix all occurrences.
-        -Wno-shift-sign-overflow # FIXME: Why do we need this with clang-cl but not clang?
-        -Wno-double-promotion # FIXME: remove me
-      )
-    endif()
-  elseif("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
-    target_add_compile_flags_if_supported(${target} PRIVATE
-      -Wno-attributes
-      -Wno-literal-suffix
-      -Wno-c++14-compat
-      -Wno-noexcept-type
-      -Wno-suggest-override)
-  endif()
-  if (LIBCXX_ENABLE_WERROR)
-    target_add_compile_flags_if_supported(${target} PRIVATE -Werror)
-    target_add_compile_flags_if_supported(${target} PRIVATE -WX)
-  else()
-    # TODO(EricWF) Remove this. We shouldn't be suppressing errors when -Werror is
-    # added elsewhere.
-    target_add_compile_flags_if_supported(${target} PRIVATE -Wno-error)
-  endif()
-  if (LIBCXX_ENABLE_PEDANTIC)
-    target_add_compile_flags_if_supported(${target} PRIVATE -pedantic)
-  endif()
-endfunction()
-
 # Exception flags =============================================================
 function(cxx_add_exception_flags target)
   if (LIBCXX_ENABLE_EXCEPTIONS)
     # Catches C++ exceptions only and tells the compiler to assume that extern C
     # functions never throw a C++ exception.
     target_add_compile_flags_if_supported(${target} PUBLIC -EHsc)
   else()
     target_add_compile_flags_if_supported(${target} PUBLIC -EHs- -EHa-)
if (WIN32 AND LIBCXX_ENABLE_STATIC_ABI_LIBRARY)
   # If linking libcxxabi statically into libcxx, skip the dllimport attributes
   # on symbols we refer to from libcxxabi.
   add_definitions(-D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS)
 endif()
 
 # Setup all common build flags =================================================
 function(cxx_add_common_build_flags target)
   cxx_add_basic_build_flags(${target})
-  cxx_add_warning_flags(${target})
+  cxx_add_warning_flags(${target} ${LIBCXX_ENABLE_WERROR} ${LIBCXX_ENABLE_PEDANTIC})
   cxx_add_windows_flags(${target})
   cxx_add_exception_flags(${target})
   cxx_add_rtti_flags(${target})
   cxx_add_module_flags(${target})
   cxx_link_system_libraries(${target})
 endfunction()
 
 #===============================================================================

libcxxabi/CMakeLists.txt

 include(CMakeDependentOption)
 include(HandleCompilerRT)
 
 # Define options.
 option(LIBCXXABI_ENABLE_EXCEPTIONS
   "Provide support for exceptions in the runtime.
   When disabled, libc++abi does not support stack unwinding and other exceptions-related features." ON)
 option(LIBCXXABI_ENABLE_ASSERTIONS "Enable assertions independent of build mode." ON)
-option(LIBCXXABI_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
+option(LIBCXXABI_ENABLE_PEDANTIC "Compile with pedantic enabled." OFF)
 option(LIBCXXABI_ENABLE_WERROR "Fail and stop if a warning is triggered." OFF)
 option(LIBCXXABI_USE_LLVM_UNWINDER "Build and use the LLVM unwinder." OFF)
 option(LIBCXXABI_ENABLE_STATIC_UNWINDER "Statically link the LLVM unwinder." OFF)
 option(LIBCXXABI_USE_COMPILER_RT "Use compiler-rt instead of libgcc" OFF)
 option(LIBCXXABI_ENABLE_THREADS "Build with threads enabled" ON)
 option(LIBCXXABI_HAS_PTHREAD_API "Ignore auto-detection and force use of pthread API" OFF)
 option(LIBCXXABI_HAS_WIN32_THREAD_API "Ignore auto-detection and force use of win32 thread API" OFF)
 option(LIBCXXABI_HAS_EXTERNAL_THREAD_API

 # Let the library headers know they are currently being used to build the
 # library.
 add_definitions(-D_LIBCXXABI_BUILDING_LIBRARY)
 
 # libcxxabi needs to, for various reasons, include the libcpp headers as if
 # it is being built as part of libcxx.
 add_definitions(-D_LIBCPP_BUILDING_LIBRARY)
 
-add_compile_flags_if_supported(-Werror=return-type)
-
-# Get warning flags
-add_compile_flags_if_supported(-W)
-add_compile_flags_if_supported(-Wall)
-add_compile_flags_if_supported(-Wchar-subscripts)
-add_compile_flags_if_supported(-Wconversion)
-add_compile_flags_if_supported(-Wmismatched-tags)
-add_compile_flags_if_supported(-Wmissing-braces)
-add_compile_flags_if_supported(-Wnewline-eof)
-add_compile_flags_if_supported(-Wunused-function)
-add_compile_flags_if_supported(-Wshadow)
-add_compile_flags_if_supported(-Wshorten-64-to-32)
-add_compile_flags_if_supported(-Wsign-compare)
-add_compile_flags_if_supported(-Wsign-conversion)
-add_compile_flags_if_supported(-Wstrict-aliasing=2)
-add_compile_flags_if_supported(-Wstrict-overflow=4)
-add_compile_flags_if_supported(-Wunused-parameter)
-add_compile_flags_if_supported(-Wunused-variable)
-add_compile_flags_if_supported(-Wwrite-strings)
-add_compile_flags_if_supported(-Wundef)
-
-add_compile_flags_if_supported(-Wno-suggest-override)
-
-if (LIBCXXABI_ENABLE_WERROR)
-  add_compile_flags_if_supported(-Werror)
-  add_compile_flags_if_supported(-WX)
-else()
-  add_compile_flags_if_supported(-Wno-error)
-  add_compile_flags_if_supported(-WX-)
-endif()
-if (LIBCXXABI_ENABLE_PEDANTIC)
-  add_compile_flags_if_supported(-pedantic)
-endif()
-
 # Get feature flags.
 add_compile_flags_if_supported(-fstrict-aliasing)
 
 # Exceptions
 if (LIBCXXABI_ENABLE_EXCEPTIONS)
   # Catches C++ exceptions only and tells the compiler to assume that extern C
   # functions never throw a C++ exception.
   add_compile_flags_if_supported(-EHsc)

libcxxabi/src/CMakeLists.txt

 if (LLVM_ENABLE_MODULES)
   string(REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
 endif()
 
 if (NOT TARGET pstl::ParallelSTL)
   message(STATUS "Could not find ParallelSTL, libc++abi will not attempt to use it but the build may fail if the libc++ in use needs it to be available.")
 endif()
 
+include(WarningFlags)
+
 # Build the shared library.
 add_library(cxxabi_shared_objects OBJECT EXCLUDE_FROM_ALL ${LIBCXXABI_SOURCES} ${LIBCXXABI_HEADERS})
+cxx_add_warning_flags(cxxabi_shared_objects ${LIBCXXABI_ENABLE_WERROR} ${LIBCXXABI_ENABLE_PEDANTIC})
 if (LIBCXXABI_USE_LLVM_UNWINDER)
   if (LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY)
     target_link_libraries(cxxabi_shared_objects PUBLIC unwind_shared_objects) # propagate usage requirements
     target_sources(cxxabi_shared_objects PUBLIC $<TARGET_OBJECTS:unwind_shared_objects>)
   else()
     target_link_libraries(cxxabi_shared_objects PUBLIC unwind_shared)
   endif()
 endif()
if (LIBCXXABI_ENABLE_EXCEPTIONS)
         reexport_symbols("${CMAKE_CURRENT_SOURCE_DIR}/../lib/personality-v0.exp")
       endif()
     endif()
   endif()
 endif()
 
 # Build the static library.
 add_library(cxxabi_static_objects OBJECT EXCLUDE_FROM_ALL ${LIBCXXABI_SOURCES} ${LIBCXXABI_HEADERS})
+cxx_add_warning_flags(cxxabi_static_objects ${LIBCXXABI_ENABLE_WERROR} ${LIBCXXABI_ENABLE_PEDANTIC})
 if (LIBCXXABI_USE_LLVM_UNWINDER AND LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_STATIC_LIBRARY)
   target_link_libraries(cxxabi_static_objects PUBLIC unwind_static_objects) # propagate usage requirements
   target_sources(cxxabi_static_objects PUBLIC $<TARGET_OBJECTS:unwind_static_objects>)
 endif()
 target_link_libraries(cxxabi_static_objects PRIVATE cxx-headers ${LIBCXXABI_STATIC_LIBRARIES} ${LIBCXXABI_LIBRARIES})
 target_link_libraries(cxxabi_static_objects PUBLIC cxxabi-headers)
 set_target_properties(cxxabi_static_objects
   PROPERTIES

libunwind/CMakeLists.txt

if (LIBUNWIND_ENABLE_CET)
   if (NOT CXX_SUPPORTS_FCF_PROTECTION_EQ_FULL_FLAG)
     message(SEND_ERROR "Compiler doesn't support CET -fcf-protection option!")
   endif()
   if (NOT CXX_SUPPORTS_MSHSTK_FLAG)
     message(SEND_ERROR "Compiler doesn't support CET -mshstk option!")
   endif()
 endif()
 
-# Get warning flags
-add_compile_flags_if_supported(-W)
-add_compile_flags_if_supported(-Wall)
-add_compile_flags_if_supported(-Wchar-subscripts)
-add_compile_flags_if_supported(-Wconversion)
-add_compile_flags_if_supported(-Wmismatched-tags)
-add_compile_flags_if_supported(-Wmissing-braces)
-add_compile_flags_if_supported(-Wnewline-eof)
-add_compile_flags_if_supported(-Wno-unused-function)
-add_compile_flags_if_supported(-Wshadow)
-add_compile_flags_if_supported(-Wshorten-64-to-32)
-add_compile_flags_if_supported(-Wsign-compare)
-add_compile_flags_if_supported(-Wsign-conversion)
-add_compile_flags_if_supported(-Wstrict-aliasing=2)
-add_compile_flags_if_supported(-Wstrict-overflow=4)
-add_compile_flags_if_supported(-Wunused-parameter)
-add_compile_flags_if_supported(-Wunused-variable)
-add_compile_flags_if_supported(-Wwrite-strings)
-add_compile_flags_if_supported(-Wundef)
-
-add_compile_flags_if_supported(-Wno-suggest-override)
-
 if (WIN32)
   # The headers lack matching dllexport attributes (_LIBUNWIND_EXPORT);
   # silence the warning instead of cluttering the headers (which aren't
   # necessarily the ones that the callers will use anyway) with the
   # attributes.
   add_compile_flags_if_supported(-Wno-dll-attribute-on-redeclaration)
 endif()
 
-if (LIBUNWIND_ENABLE_WERROR)
-  add_compile_flags_if_supported(-Werror)
-  add_compile_flags_if_supported(-WX)
-else()
-  add_compile_flags_if_supported(-Wno-error)
-  add_compile_flags_if_supported(-WX-)
-endif()
-
-if (LIBUNWIND_ENABLE_PEDANTIC)
-  add_compile_flags_if_supported(-pedantic)
-endif()
-
 # Get feature flags.
 # Exceptions
 # Catches C++ exceptions only and tells the compiler to assume that extern C
 # functions never throw a C++ exception.
 add_cxx_compile_flags_if_supported(-fstrict-aliasing)
 add_cxx_compile_flags_if_supported(-EHsc)
 
 # Don't run the linker in this CMake check.

libunwind/src/CMakeLists.txt

set_property(SOURCE ${LIBUNWIND_CXX_SOURCES}
              APPEND_STRING PROPERTY COMPILE_FLAGS " ${LIBUNWIND_CXX_FLAGS}")
 set_property(SOURCE ${LIBUNWIND_C_SOURCES}
              APPEND_STRING PROPERTY COMPILE_FLAGS " ${LIBUNWIND_C_FLAGS}")
 
 # NOTE: avoid implicit dependencies on C++ runtimes.  libunwind uses C++ for
 # ease, but does not rely on C++ at runtime.
 set(CMAKE_CXX_IMPLICIT_LINK_LIBRARIES "")
 
+include(WarningFlags)
+
 # Build the shared library.
 add_library(unwind_shared_objects OBJECT EXCLUDE_FROM_ALL ${LIBUNWIND_SOURCES} ${LIBUNWIND_HEADERS})
+cxx_add_warning_flags(unwind_shared_objects ${LIBUNWIND_ENABLE_WERROR} ${LIBUNWIND_ENABLE_PEDANTIC})
 if(CMAKE_C_COMPILER_ID STREQUAL MSVC)
   target_compile_options(unwind_shared_objects PRIVATE /GR-)
 else()
   target_compile_options(unwind_shared_objects PRIVATE -fno-rtti)
 endif()
 target_link_libraries(unwind_shared_objects PRIVATE unwind-headers ${LIBUNWIND_LIBRARIES})
 set_target_properties(unwind_shared_objects
   PROPERTIES
if (LIBUNWIND_ENABLE_SHARED)
   list(APPEND LIBUNWIND_BUILD_TARGETS "unwind_shared")
   if (LIBUNWIND_INSTALL_SHARED_LIBRARY)
     list(APPEND LIBUNWIND_INSTALL_TARGETS "unwind_shared")
   endif()
 endif()
 
 # Build the static library.
 add_library(unwind_static_objects OBJECT EXCLUDE_FROM_ALL ${LIBUNWIND_SOURCES} ${LIBUNWIND_HEADERS})
+cxx_add_warning_flags(unwind_static_objects ${LIBUNWIND_ENABLE_WERROR} ${LIBUNWIND_ENABLE_PEDANTIC})
 if(CMAKE_C_COMPILER_ID STREQUAL MSVC)
   target_compile_options(unwind_static_objects PRIVATE /GR-)
 else()
   target_compile_options(unwind_static_objects PRIVATE -fno-rtti)
 endif()
 target_link_libraries(unwind_static_objects PRIVATE unwind-headers ${LIBUNWIND_LIBRARIES})
 set_target_properties(unwind_static_objects
   PROPERTIES

runtimes/cmake/Modules/WarningFlags.cmake

+include(HandleFlags)
+
+# Warning flags ===============================================================
+function(cxx_add_warning_flags target enable_werror enable_pedantic)
+  target_compile_definitions(${target} PUBLIC -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+  if (MSVC)
+    # -W4 is the cl.exe/clang-cl equivalent of -Wall. (In cl.exe and clang-cl,
+    # -Wall is equivalent to -Weverything in GCC style compiler drivers.)
+    target_add_compile_flags_if_supported(${target} PRIVATE -W4)
+  else()
+    target_add_compile_flags_if_supported(${target} PRIVATE -Wall)
+  endif()
+  # TODO: Should -Wconversion be enabled?
+  target_add_compile_flags_if_supported(${target} PRIVATE
+      -Wall
+      -Wextra
+      -Wnewline-eof
+      -Wshadow
+      -Wwrite-strings
+      -Wno-unused-parameter
+      -Wno-long-long
+      -Werror=return-type
+      -Wextra-semi
+      -Wundef
+      -Wformat-nonliteral
+      )
+
+  if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
+    target_add_compile_flags_if_supported(${target} PRIVATE
+      -Wno-user-defined-literals
+      -Wno-covered-switch-default
+      -Wno-suggest-override
+    )
+    if (LIBCXX_TARGETING_CLANG_CL)
+      target_add_compile_flags_if_supported(${target} PRIVATE
+        -Wno-c++98-compat
+        -Wno-c++98-compat-pedantic
+        -Wno-c++11-compat
+        -Wno-undef
+        -Wno-reserved-id-macro
+        -Wno-gnu-include-next
+        -Wno-gcc-compat # For ignoring "'diagnose_if' is a clang extension" warnings
+        -Wno-zero-as-null-pointer-constant # FIXME: Remove this and fix all occurrences.
+        -Wno-deprecated-dynamic-exception-spec # For auto_ptr
+        -Wno-sign-conversion
+        -Wno-old-style-cast
+        -Wno-deprecated # FIXME: Remove this and fix all occurrences.
+        -Wno-shift-sign-overflow # FIXME: Why do we need this with clang-cl but not clang?
+        -Wno-double-promotion # FIXME: remove me
+      )
+    endif()
+
+  elseif("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
+
+    target_add_compile_flags_if_supported(${target} PRIVATE
+      -Wstrict-aliasing=2
+      -Wstrict-overflow=4
+      -Wno-attributes
+      -Wno-literal-suffix
+      -Wno-c++14-compat
+      -Wno-noexcept-type
+      -Wno-suggest-override
+      )
+
+  endif()
+  if (${enable_werror})
+    target_add_compile_flags_if_supported(${target} PRIVATE -Werror)
+    target_add_compile_flags_if_supported(${target} PRIVATE -WX)
+  else()
+    # TODO(EricWF) Remove this. We shouldn't be suppressing errors when -Werror is
+    # added elsewhere.
+    target_add_compile_flags_if_supported(${target} PRIVATE -Wno-error)
+  endif()
+  if (${enable_pedantic})
+    target_add_compile_flags_if_supported(${target} PRIVATE -pedantic)
+  endif()
+endfunction()