[Sema] -Wzero-as-null-pointer-constant: don't warn for __null

[zeux]

The implementation of -Wzero-as-null-pointer-constant was done before the following fix has been committed to GCC:

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=752e7593b0f19af233a0b7e72daab8413662b605;hp=298434c916c14e8adca2cab8a746aee29038c5b3

As a result, clang and gcc diverge on the use of __null and, consequently, on the use of NULL on systems like Linux/macOS where NULL is defined as __null.

This is a problem for compatibility between gcc and clang, particularly for code bases that support C++98 or for single-source libraries that are implemented in C, but compiled as C++ via inclusion into a C++ translation unit. Code like this can not be changed to use nullptr, as it needs to maintain compatibility with C before C23 or C++ before C++11, but warns on the use of NULL in clang.

The warning Wzero-as-null-pointer-constant is still useful with this change, as it allows to change 0 to NULL, which fixes
gcc warnings and helps the reader distinguish between pointers and non-pointers. Users who require a full C++11 modernization pass can still use clang-tidy for that purpose.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Arseny Kapoulkine (zeux)

Changes

The implementation of -Wzero-as-null-pointer-constant was done before the following fix has been committed to GCC:

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=752e7593b0f19af233a0b7e72daab8413662b605;hp=298434c916c14e8adca2cab8a746aee29038c5b3

As a result, clang and gcc diverge on the use of __null and, consequently, on the use of NULL on systems like Linux/macOS where NULL is defined as __null.

This is a problem for compatibility between gcc and clang, particularly for code bases that support C++98 or for single-source libraries that are implemented in C, but compiled as C++ via inclusion into a C++ translation unit. Code like this can not be changed to use nullptr, as it needs to maintain compatibility with C before C23 or C++ before C++11, but warns on the use of NULL in clang.

The warning Wzero-as-null-pointer-constant is still useful with this change, as it allows to change 0 to NULL, which fixes
gcc warnings and helps the reader distinguish between pointers and non-pointers. Users who require a full C++11 modernization pass can still use clang-tidy for that purpose.

---

Full diff: https://github.com/llvm/llvm-project/pull/69126.diff

2 Files Affected:

• (modified) clang/lib/Sema/Sema.cpp (+7-1)
• (modified) clang/test/SemaCXX/warn-zero-nullptr.cpp (+4-4)

diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 67533ccbdf347c7..acb765559e6a8d4 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -590,7 +590,11 @@ void Sema::diagnoseZeroToNullptrConversion(CastKind Kind, const Expr *E) {
 
   if (Kind != CK_NullToPointer && Kind != CK_NullToMemberPointer)
     return;
-  if (E->IgnoreParenImpCasts()->getType()->isNullPtrType())
+
+  const Expr *EStripped = E->IgnoreParenImpCasts();
+  if (EStripped->getType()->isNullPtrType())
+    return;
+  if (isa<GNUNullExpr>(EStripped))
     return;
 
   if (Diags.isIgnored(diag::warn_zero_as_null_pointer_constant,
@@ -612,6 +616,8 @@ void Sema::diagnoseZeroToNullptrConversion(CastKind Kind, const Expr *E) {
 
   // If it is a macro from system header, and if the macro name is not "NULL",
   // do not warn.
+  // Note that uses of "NULL" will be ignored above on systems that define it
+  // as __null.
   SourceLocation MaybeMacroLoc = E->getBeginLoc();
   if (Diags.getSuppressSystemWarnings() &&
       SourceMgr.isInSystemMacro(MaybeMacroLoc) &&
diff --git a/clang/test/SemaCXX/warn-zero-nullptr.cpp b/clang/test/SemaCXX/warn-zero-nullptr.cpp
index 45f05fa5703b1c3..684572f8ef67d05 100644
--- a/clang/test/SemaCXX/warn-zero-nullptr.cpp
+++ b/clang/test/SemaCXX/warn-zero-nullptr.cpp
@@ -16,10 +16,10 @@ int (S::*mp1) = 0; // expected-warning{{zero as null pointer constant}}
 void (*fp1)() = 0; // expected-warning{{zero as null pointer constant}}
 void* p1 = 0; // expected-warning{{zero as null pointer constant}}
 
-// NULL is an integer constant expression, so warn on it too:
-void* p2 = __null; // expected-warning{{zero as null pointer constant}}
-void (*fp2)() = __null; // expected-warning{{zero as null pointer constant}}
-int (S::*mp2) = __null; // expected-warning{{zero as null pointer constant}}
+// __null is not treated as an integer constant expression for GCC compatibility
+void* p2 = __null;
+void (*fp2)() = __null;
+int (S::*mp2) = __null;
 
 void f0(void* v = MACRO); // expected-warning{{zero as null pointer constant}}
 void f1(void* v = NULL); // expected-warning{{zero as null pointer constant}}

[zeux]

Note: a limitation of this change is that we will still warn on use of NULL when it is defined as 0, which could be a problem for clang-cl. This could be lifted by removing the "NULL" macro name check, but unfortunately that is insufficient to detect the use of NULL inside macro expansion (such as MCRO(NULL) in the test file). Because of this, this change is conservative in that it aligns the logic with GCC but doesn't attempt to handle NULL fully. I can revise this if this seems like an issue worth addressing here.

This change revises the logic of 809df34 so
cc @LebedevRI as the author of that adjustment, @nico as the original author, and @AaronBallman since we briefly discussed this on Discord.

[zeux]

Ping

[AaronBallman]

I think the changes are reasonable, but should come with a release note so users know about the change in diagnostic behavior. (We have a section in clang/docs/ReleaseNotes.rst for diagnostic improvements.)

Otherwise, I think the changes LGTM

[zeux]

Thanks! I've updated this PR with a release note.

[AaronBallman]

LGTM!

[zeux]

Let me know if I need to do anything else here -- I don't have commit rights so someone else will need to press the merge button :)