This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Disable GCC -Wnonnull
ClosedPublic

Authored by fzakaria on Jul 20 2023, 9:25 AM.

Details

Reviewers
MaskRay
rsmith
Group Reviewers
Restricted Project
Commits
rG1f8f8760b586: [CMake] Disable GCC -Wnonnull
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.

Diff Detail

Event Timeline

fzakaria created this revision.Jul 20 2023, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 9:25 AM
fzakaria requested review of this revision.Jul 20 2023, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 9:25 AM
rsmith requested changes to this revision.Jul 20 2023, 9:31 AM
rsmith added a subscriber: 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?

This revision now requires changes to proceed.Jul 20 2023, 9:31 AM

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.

MaskRay added a reviewer: Restricted Project.Jul 20 2023, 10:15 AM
fzakaria updated this revision to Diff 542614.Jul 20 2023, 11:42 AM

Updated instead to disable it in CMake

Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 11:42 AM

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

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).

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

rsmith accepted this revision as: rsmith.Jul 20 2023, 12:13 PM

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.

This revision is now accepted and ready to land.Jul 20 2023, 12:13 PM
fzakaria updated this revision to Diff 542663.Jul 20 2023, 1:56 PM

Added comment to explain why we are disabling

fzakaria marked an inline comment as done.Jul 20 2023, 1:56 PM

@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 added inline comments.Jul 21 2023, 8:03 PM
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 updated this revision to Diff 543238.Jul 22 2023, 5:50 PM

Address feedback from maskray@

  • imperative comment
  • switch to CMAKE_GNU_COMPILER_ID for variable
fzakaria marked an inline comment as done.Jul 22 2023, 5:51 PM

@MaskRay thanks for the review.

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...

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 updated this revision to Diff 543244.Jul 22 2023, 8:18 PM

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

Changed to the if condition.

MaskRay accepted this revision.Jul 23 2023, 8:16 PM
MaskRay retitled this revision from [clang] fix nonnull warnings during build to [CMake] Disable GCC -Wnonnull.
This revision was landed with ongoing or failed builds.Jul 23 2023, 8:19 PM
Closed by commit rG1f8f8760b586: [CMake] Disable GCC -Wnonnull (authored by fzakaria, committed by MaskRay). · Explain Why
This revision was automatically updated to reflect the committed changes.