[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 ↗

(On Diff #542614)

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 ↗

(On Diff #542663)

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.

clang/include/clang/AST/ExternalASTSource.h

public:
   ///
   /// \param Source the external AST source.
   ///
   /// \returns a pointer to the AST node.
   T *get(ExternalASTSource *Source) const {
     if (isOffset()) {
       assert(Source &&
              "Cannot deserialize a lazy pointer without an AST source");
+      if (Source != nullptr) {
         Ptr = reinterpret_cast<uint64_t>((Source->*Get)(Ptr >> 1));
       }
+    }
     return reinterpret_cast<T*>(Ptr);
   }
 
   /// Retrieve the address of the AST node pointer. Deserializes the pointee if
   /// necessary.
   T **getAddressOfPointer(ExternalASTSource *Source) const {
     // Ensure the integer is in pointer form.
     (void)get(Source);