Enable -Wsuggest-override in the LLVM build

[logan-5]

Summary

This patch adds Clang's new (and GCC's old) -Wsuggest-override to the warning flags for the LLVM build. The warning is a stronger form of -Winconsistent-missing-override which warns _everywhere_ that override is missing, not just in places where it's inconsistent within a class.

Some directories in the monorepo need the warning disabled for compatibility's, or sanity's, sake; in particular, libcxx/libcxxabi (I've added @ldionne as a reviewer to confirm this), and any code implementing or interoperating with googletest, googlemock, or google benchmark (which do not themselves use override). This patch adds -Wno-suggest-override to the relevant CMakeLists.txt's to accomplish this.

[xbolva00]

+1

[nemanjai]

There is a lot of instances where this warning fires in the unit tests since gtest does not appear to use the override keyword. This in turn causes failures on bootstrap builds that use -Werror.
An example: http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/10717

Can we either get this pulled until the failures in bootstrap are fixed or quickly fix the code that causes the failures?

[logan-5]

In D84126#2165585, @nemanjai wrote:

There is a lot of instances where this warning fires in the unit tests since gtest does not appear to use the override keyword. This in turn causes failures on bootstrap builds that use -Werror.
An example: http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/10717

Can we either get this pulled until the failures in bootstrap are fixed or quickly fix the code that causes the failures?

I've got fixes for these failures waiting on review (https://reviews.llvm.org/D84244). Perhaps I'll go ahead and commit them now, then revert them and fix it differently if the reviewers request it.

[logan-5]

All the failures related to this patch are now resolved as far as I can tell. I'm very sorry for the trouble.

[erichkeane]

Ive got a bit a of a problem here: This flag (suggest-override) on GCC 8.3 (and others) warns on missing-override EVEN WHEN the function is marked 'final'. This was not fixed until GCC 9.2: https://godbolt.org/z/55KeM3

The result is my build gave a few thousand diagnostics for this. I'd suggest disabling this unless the GCC version is >9.1.

[logan-5]

In D84126#2166952, @erichkeane wrote:

I'd suggest disabling this unless the GCC version is >9.1.

I've got a patch addressing exactly this issue here, hoping to get it landed soon: https://reviews.llvm.org/D84292

Thanks again everyone for your patience. After this experience, I can promise next time I do anything like this, it'll go much more smoothly.

[labath]

Instead of adding -Wno-suggest-override to every user it might be cleaner to add the flag to the INTERFACE_COMPILE_OPTIONS property of the relevant targets (gtest, gmock, and gbenchmark ?). That way, anything which links against these will inherit it automatically.

[logan-5]

In D84126#2171469, @labath wrote:

Instead of adding -Wno-suggest-override to every user it might be cleaner to add the flag to the INTERFACE_COMPILE_OPTIONS property of the relevant targets (gtest, gmock, and gbenchmark ?). That way, anything which links against these will inherit it automatically.

Thanks, that's a good idea -- I'll look into that as a follow-up patch.

libcxx/CMakeLists.txt

function(cxx_add_warning_flags target)
   target_compile_definitions(${target} PUBLIC -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
   target_add_compile_flags_if_supported(${target} PRIVATE -Wall -Wextra -W -Wwrite-strings
                                                           -Wno-unused-parameter -Wno-long-long
                                                           -Werror=return-type -Wextra-semi)
   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
       -Wno-ignored-attributes # FIXME: Caused by _LIBCPP_NODEBUG_TYPE not being supported on older clangs
     )
     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-literal-suffix
       -Wno-c++14-compat
-      -Wno-noexcept-type)
+      -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)

libcxxabi/CMakeLists.txt

 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)

lldb/unittests/CMakeLists.txt

 add_custom_target(LLDBUnitTests)
 set_target_properties(LLDBUnitTests PROPERTIES FOLDER "lldb tests")
 add_dependencies(lldb-test-deps LLDBUnitTests)
 
 include_directories(${LLDB_SOURCE_ROOT})
 include_directories(${LLDB_PROJECT_ROOT}/unittests)
 
+if (CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG)
+  add_definitions("-Wno-suggest-override")
+endif()
+
 set(LLDB_GTEST_COMMON_INCLUDE ${CMAKE_CURRENT_SOURCE_DIR}/gtest_common.h)
 if (MSVC)
   list(APPEND LLVM_COMPILE_FLAGS /FI ${LLDB_GTEST_COMMON_INCLUDE})
 else ()
   list(APPEND LLVM_COMPILE_FLAGS -include ${LLDB_GTEST_COMMON_INCLUDE})
 endif ()
 
 if (LLDB_BUILT_STANDALONE)

llvm/cmake/modules/HandleLLVMOptions.cmake

if (NOT CMAKE_COMPILER_IS_GNUCXX AND NOT WIN32)
     set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
     append_if(CXX_WONT_WARN_ON_FINAL_NONVIRTUALDTOR
               "-Wnon-virtual-dtor" CMAKE_CXX_FLAGS)
   endif()
 
   # Enable -Wdelete-non-virtual-dtor if available.
   add_flag_if_supported("-Wdelete-non-virtual-dtor" DELETE_NON_VIRTUAL_DTOR_FLAG)
 
+  # Enable -Wsuggest-override if available.
+  add_flag_if_supported("-Wsuggest-override" SUGGEST_OVERRIDE_FLAG)
+
   # Check if -Wcomment is OK with an // comment ending with '\' if the next
   # line is also a // comment.
   set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
   set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -Werror -Wcomment")
   CHECK_C_SOURCE_COMPILES("// \\\\\\n//\\nint main() {return 0;}"
                           C_WCOMMENT_ALLOWS_LINE_WRAP)
   set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
   if (NOT C_WCOMMENT_ALLOWS_LINE_WRAP)

llvm/utils/benchmark/CMakeLists.txt

else()
   add_cxx_compiler_flag(-pedantic-errors)
   add_cxx_compiler_flag(-Wshorten-64-to-32)
   add_cxx_compiler_flag(-Wfloat-equal)
   add_cxx_compiler_flag(-fstrict-aliasing)
   if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
     add_cxx_compiler_flag(-fno-exceptions)
   endif()
 
+  if (CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG)
+    add_cxx_compiler_flag(-Wno-suggest-override)
+  endif()
+
   if (HAVE_CXX_FLAG_FSTRICT_ALIASING)
     if (NOT CMAKE_CXX_COMPILER_ID STREQUAL "Intel") #ICC17u2: Many false positives for Wstrict-aliasing
       add_cxx_compiler_flag(-Wstrict-aliasing)
     endif()
   endif()
   # ICC17u2: overloaded virtual function "benchmark::Fixture::SetUp" is only partially overridden
   # (because of deprecated overload)
   add_cxx_compiler_flag(-wd654)

llvm/utils/unittest/CMakeLists.txt

if(SUPPORTS_VARIADIC_MACROS_FLAG)
   add_definitions("-Wno-variadic-macros")
 endif()
 if(SUPPORTS_GNU_ZERO_VARIADIC_MACRO_ARGUMENTS_FLAG)
   add_definitions("-Wno-gnu-zero-variadic-macro-arguments")
 endif()
 if(CXX_SUPPORTS_COVERED_SWITCH_DEFAULT_FLAG)
   add_definitions("-Wno-covered-switch-default")
 endif()
+if(CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG)
+  add_definitions("-Wno-suggest-override")
+endif()
 
 set(LLVM_REQUIRES_RTTI 1)
 add_definitions( -DGTEST_HAS_RTTI=0 )
 
 if (NOT LLVM_ENABLE_THREADS)
   add_definitions( -DGTEST_HAS_PTHREAD=0 )
 endif()