[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria created this revision.
Herald added a project: All.
fzakaria requested review of this revision.
Herald added subscribers: cfe-commits, wangpc.
Herald added a project: clang.

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((Source->*Get)(Ptr >> 1));
        |                                        ~~^~

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

  return cast_or_null(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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155857

Files:
  clang/include/clang/AST/ExternalASTSource.h


Index: clang/include/clang/AST/ExternalASTSource.h
===
--- clang/include/clang/AST/ExternalASTSource.h
+++ clang/include/clang/AST/ExternalASTSource.h
@@ -375,7 +375,9 @@
 if (isOffset()) {
   assert(Source &&
  "Cannot deserialize a lazy pointer without an AST source");
-  Ptr = reinterpret_cast((Source->*Get)(Ptr >> 1));
+  if (Source != nullptr) {
+Ptr = reinterpret_cast((Source->*Get)(Ptr >> 1));
+  }
 }
 return reinterpret_cast(Ptr);
   }


Index: clang/include/clang/AST/ExternalASTSource.h
===
--- clang/include/clang/AST/ExternalASTSource.h
+++ clang/include/clang/AST/ExternalASTSource.h
@@ -375,7 +375,9 @@
 if (isOffset()) {
   assert(Source &&
  "Cannot deserialize a lazy pointer without an AST source");
-  Ptr = reinterpret_cast((Source->*Get)(Ptr >> 1));
+  if (Source != nullptr) {
+Ptr = reinterpret_cast((Source->*Get)(Ptr >> 1));
+  }
 }
 return reinterpret_cast(Ptr);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria updated this revision to Diff 542614.
fzakaria added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Updated instead to disable it in CMake


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

Files:
  llvm/cmake/modules/HandleLLVMOptions.cmake


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,9 @@
   append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS)
   append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" 
CMAKE_CXX_FLAGS)
 
+
+  append_if(CMAKE_COMPILER_IS_GNUCXX "-Wno-nonnull" CMAKE_CXX_FLAGS)
+
   # 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)


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,9 @@
   append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS)
   append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" CMAKE_CXX_FLAGS)
 
+
+  append_if(CMAKE_COMPILER_IS_GNUCXX "-Wno-nonnull" CMAKE_CXX_FLAGS)
+
   # 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)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria updated this revision to Diff 542663.
fzakaria added a comment.

Added comment to explain why we are disabling


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

Files:
  llvm/cmake/modules/HandleLLVMOptions.cmake


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,9 @@
   append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS)
   append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" 
CMAKE_CXX_FLAGS)
 
+  # Disabling this GCC warning as it is emitting a lot of false positives
+  append_if(CMAKE_COMPILER_IS_GNUCXX "-Wno-nonnull" CMAKE_CXX_FLAGS)
+
   # 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)


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,9 @@
   append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS)
   append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" CMAKE_CXX_FLAGS)
 
+  # Disabling this GCC warning as it is emitting a lot of false positives
+  append_if(CMAKE_COMPILER_IS_GNUCXX "-Wno-nonnull" CMAKE_CXX_FLAGS)
+
   # 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)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria added a comment.

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

Thank you for the review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria updated this revision to Diff 543238.
fzakaria added a comment.

Address feedback from maskray@

- imperative comment
- switch to CMAKE_GNU_COMPILER_ID for variable


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

Files:
  llvm/cmake/modules/HandleLLVMOptions.cmake


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,9 @@
   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 as it is emitting a lot of false positives.
+  append_if(CMAKE_GNU_COMPILER_ID "-Wno-nonnull" CMAKE_CXX_FLAGS)
+
   # 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)


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,9 @@
   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 as it is emitting a lot of false positives.
+  append_if(CMAKE_GNU_COMPILER_ID "-Wno-nonnull" CMAKE_CXX_FLAGS)
+
   # 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)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria marked an inline comment as done.
fzakaria added a comment.

@MaskRay thanks for the review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria updated this revision to Diff 543244.
fzakaria added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

Files:
  llvm/cmake/modules/HandleLLVMOptions.cmake


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,11 @@
   append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS)
   append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" 
CMAKE_CXX_FLAGS)
 
+  # Disable -Wnonnull forthis 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)


Index: llvm/cmake/modules/HandleLLVMOptions.cmake
===
--- llvm/cmake/modules/HandleLLVMOptions.cmake
+++ llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -770,6 +770,11 @@
   append_if(USE_NO_UNINITIALIZED "-Wno-uninitialized" CMAKE_CXX_FLAGS)
   append_if(USE_NO_MAYBE_UNINITIALIZED "-Wno-maybe-uninitialized" CMAKE_CXX_FLAGS)
 
+  # Disable -Wnonnull forthis 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)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155857: [clang] fix nonnull warnings during build

[Farid Zakaria via Phabricator via cfe-commits]

fzakaria added a comment.

Changed to the if condition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155857/new/

https://reviews.llvm.org/D155857

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits