[CMake] Disable GCC -Wnonnull

[fzakaria]

Summary

I noticed during the build that GCC would emit a ton of nonnull
warnings.

Example:

/usr/local/google/home/fmzakari/code/github.com/llvm/llvm-project/clang/include/clang/AST/ExternalASTSource.h:378:54: warning: ‘this’ pointer is null [-Wnonnull]
  378 |       Ptr = reinterpret_cast<uint64_t>((Source->*Get)(Ptr >> 1));
      |                                        ~~~~~~~~~~~~~~^~~~~~~~~~

Upon investigation there are actually valid places where we are passing
null as source such as:

return cast_or_null<FriendDecl>(NextFriend.get(nullptr));

if (!Bases.isOffset())
       return Bases.get(nullptr);

There is an assertion to catch this but that is only in debug builds.

Added a new if protection branch and kept the assertion to keep the code
change very minimal.

[rsmith]

This branch doesn't look necessary or correct to me. We should never call this function on a lazy pointer in offset mode unless we have an external source that produced the offset, and it's certainly not appropriate to cast the offset to a pointer and return it.

Do you have a testcase that is crashing here?

[MaskRay]

I agree with the analysis. My understanding is that we don't worsen the code quality for Clang to work around a GCC warning.
And generally we care strongly about Clang warnings, but less about GCC warnings, especially in .cpp files. However, this is in a header and perhaps we should pay some attention, as it may affect Clang library users.

[fzakaria]

Updated instead to disable it in CMake

[fzakaria]

@rsmith and @MaskRay thank you for the feedback.
I think this is more in line with what we discussed on the #LLVM chat

[MaskRay]

Perhaps -Wno-nonnull should be GCC specific (i.e. not apply to Clang).

This option will disable certain warnings related to Clang _Nonnull (seems unused in llvm-project) and __attribute__((nonnull)) (used in a few places).

[MaskRay]

Perhaps this can be placed beside llvm/cmake/config-ix.cmake -Wmaybe-uninitialized?

[rsmith]

Seems reasonable to me. Given that this affects all of LLVM I'd like to wait a day or so to see if anyone has concerns.

llvm/cmake/modules/HandleLLVMOptions.cmake

774

It'd be useful to leave a comment for future readers that we're disabling this due to false positives.

[fzakaria]

Added comment to explain why we are disabling

[fzakaria]

@rsmith -- sounds good.
Please commit on my behalf when you are ready as I don't have commit access.

Thank you for the review.

[MaskRay]

llvm/cmake/modules/HandleLLVMOptions.cmake

773

Use an imperative sentence, i.e. Disable ... and append a full stop.

CMAKE_COMPILER_IS_GNUCXX is deprecated . Use CMAKE_<LANG>_COMPILER_ID

[fzakaria]

Address feedback from maskray@

• imperative comment
• switch to CMAKE_GNU_COMPILER_ID for variable

[fzakaria]

@MaskRay thanks for the review.

[MaskRay]

In D155857#4525421, @fzakaria wrote:

Address feedback from maskray@

• imperative comment
• switch to CMAKE_GNU_COMPILER_ID for variable

Does append_if(CMAKE_GNU_COMPILER_ID "-Wno-nonnull" CMAKE_CXX_FLAGS) work? build.ninja with a GCC doesn't have -Wno-nonnull...

[fzakaria]

Ugh you are right.
So weird --

This works:

if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
  append("-Wno-nonnull" CMAKE_CXX_FLAGS)
endif()

I read the CMake documentation and it looked like the variable I used should have worked.
https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER_ID.html#variable:CMAKE_%3CLANG%3E_COMPILER_ID

/shrug

Should I change to the version above or use the deprecated older variable?

[fzakaria]

The previous CMake variable wasn't working.

Changed it to the pattern I see in the codebase.
Confirmed it via ninja command and grepping.

❯ rg --color=always no-nonnull build.ninja | head -n 1
  FLAGS = -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O2 -g -DNDEBUG  -fno-exceptions -funwind-tables -fno-rtti -gsplit-dwarf -std=c++17

[fzakaria]

Changed to the if condition.

llvm/cmake/modules/HandleLLVMOptions.cmake

if (LLVM_ENABLE_PEDANTIC AND LLVM_COMPILER_IS_GCC_COMPATIBLE)
     add_flag_if_supported("-Wc++98-compat-extra-semi" CXX98_COMPAT_EXTRA_SEMI_FLAG)
   endif()
 
   add_flag_if_supported("-Wimplicit-fallthrough" IMPLICIT_FALLTHROUGH_FLAG)
   add_flag_if_supported("-Wcovered-switch-default" COVERED_SWITCH_DEFAULT_FLAG)
   append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS)
   append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" CMAKE_CXX_FLAGS)
 
+  # Disable -Wnonnull for GCC warning as it is emitting a lot of false positives.
+  if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+    append("-Wno-nonnull" CMAKE_CXX_FLAGS)
+  endif()
+
   # Disable -Wclass-memaccess, a C++-only warning from GCC 8 that fires on
   # LLVM's ADT classes.
   check_cxx_compiler_flag("-Wclass-memaccess" CXX_SUPPORTS_CLASS_MEMACCESS_FLAG)
   append_if(CXX_SUPPORTS_CLASS_MEMACCESS_FLAG "-Wno-class-memaccess" CMAKE_CXX_FLAGS)
 
   # Disable -Wredundant-move and -Wpessimizing-move on GCC>=9. GCC wants to
   # remove std::move in code like "A foo(ConvertibleToA a) {
   # return std::move(a); }", but this code does not compile (or uses the copy