[Sema] -Wzero-as-null-pointer-constant: don't warn for system macros other than NULL.

[lebedev.ri]

Summary

The warning was initially introduced in D32914 by @thakis,
and the concerns were raised there, and later in rL302247
and PR33771.

I do believe that it makes sense to relax the diagnostic
e.g. in this case, when the expression originates from the
system header, which can not be modified. This prevents
adoption for the diagnostic for codebases which use pthreads
(PTHREAD_MUTEX_INITIALIZER), gtest, etc.

As @malcolm.parsons suggests, it *may* make sense to also
not warn for the template types, but it is not obvious to
me how to do that in here.

Though, it still makes sense to complain about NULL macro.

While there, add more tests.

[thakis]

As said on the bug, this matches gcc's behavior, and with this you won't see this warning for NULL. I don't think this is better.

[lebedev.ri]

In D38954#898586, @thakis wrote:

As said on the bug, this matches gcc's behavior,

https://bugs.llvm.org/show_bug.cgi?id=33771#c3

and with this you won't see this warning for NULL.

Finally, an argument that can actually be addressed.

I don't think this is better.

[lebedev.ri]

Address @thakis review notes: do make sure that we still warn on NULL.
Any other special macros/cases?

[lebedev.ri]

Ping.
Rebased, added release notes note.

[Rakete1111]

lib/Sema/Sema.cpp

445 ↗

(On Diff #119833)

That comment doesn't really add anything IMO. It just says what the code just below says.

447 ↗

(On Diff #119833)

Please use MaybeMacroLoc there too.

[lebedev.ri]

Use MaybeMacroLoc variable in the other place too.

lib/Sema/Sema.cpp

445 ↗

(On Diff #119833)

I don't disagree. It seems to be the common pattern for the code in this file.
Having both the code and the comment might help convey the idea better.

[lebedev.ri]

• Rebased
• Handle -Wsystem-headers properly, as per irc disscussion.

It was suggested that findMacroSpelling() might be slow, and isNullPointerConstant() should be used, but i'm not sure any of the NullPointerConstantKind's can replace direct check for NULL specifically...

[thakis]

Can you build some large-ish codebase (say, LLVM) with and without this patch and make sure that this doesn't measurably add to build perf? (With the warning turned on, obviously.)

Other than that, this looks good to me.

test/SemaCXX/warn-zero-nullptr.cpp

68 ↗

(On Diff #120285)

Did you mean to fix this before commit?

[lebedev.ri]

In D38954#906900, @thakis wrote:

Can you build some large-ish codebase (say, LLVM) with and without this patch and make sure that this doesn't measurably add to build perf? (With the warning turned on, obviously.)

Other than that, this looks good to me.

Very contrived example:

$ head -n 3 test.cpp 
#include <cstddef>

int main(int argc, char* argv[]) {
$ perl -e 'print "if(argv == NULL) return 1;\n"x1000000; print "\n"' >> test.cpp
$ tail -n 2 test.cpp 
}

So this is a file with 1 million comparisons of pointer with NULL.

$ time clang -fsyntax-only -Wzero-as-null-pointer-constant -std=c++11 test.cpp -w

real    0m8.197s
user    0m8.071s
sys     0m0.124s
$ time clang -fsyntax-only -Wzero-as-null-pointer-constant -std=c++11 test.cpp -w

real    0m7.881s
user    0m7.728s
sys     0m0.152s
$ time clang -fsyntax-only -Wzero-as-null-pointer-constant -std=c++11 test.cpp -w

real    0m7.212s
user    0m7.063s
sys     0m0.148s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -std=c++11 test.cpp -w

real    0m11.200s
user    0m11.070s
sys     0m0.128s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -std=c++11 test.cpp -w

real    0m11.141s
user    0m11.019s
sys     0m0.121s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -std=c++11 test.cpp -w

real    0m11.254s
user    0m11.127s
sys     0m0.126s

So there absolutely is some penalty, but it does not appear to be huge, at least on this contrived example.
I *could* try benchmark-building llvm, but i'm still sporadically experiencing a crash, so the timings will not be comparable.

test/SemaCXX/warn-zero-nullptr.cpp

68 ↗

(On Diff #120285)

Eh, i'm conflicted about this one.
This is more of "for future consideration"

[lebedev.ri]

Rebased now that D39301 has landed.

[lebedev.ri]

Hm, that's a lot of overhead. Granted, it's a synthetic benchmark, but I think it'd be good to try this on some real codebase to make sure it really is negligible overhead in real-world scenarios.

Hm wait, i'm comparing apples and oranges here, my local build at least has the assertions enabled.
Local build, without the patch:

$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.130s
user    0m9.985s
sys     0m0.144s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.195s
user    0m10.081s
sys     0m0.112s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.099s
user    0m9.977s
sys     0m0.120s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.110s
user    0m9.979s
sys     0m0.130s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.063s
user    0m9.969s
sys     0m0.092s

Local build, with the patch:

$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.534s
user    0m10.399s
sys     0m0.133s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.478s
user    0m10.286s
sys     0m0.159s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.359s
user    0m10.226s
sys     0m0.133s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.292s
user    0m10.146s
sys     0m0.145s
$ time /build/llvm-build-Clang-release/bin/clang -fsyntax-only -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11 test.cpp -w;

real    0m10.383s
user    0m10.305s
sys     0m0.077s

So it seems the overhead is around +0.5s, not +3.0s

[lebedev.ri]

Add Diags.isIgnored() early return + move C++11 early return earlier too.
At least partially should help avoid the penalty of findMacroSpelling()
I don't think i see any way to avoid findMacroSpelling() :/

[thakis]

In general, "I don't see any way to prevent $slow_thing" just means the patch can't move ahead. But in this case, this looks like fairly small overhead for a worst-case input, so this won't be slow on real-world code. So lgtm.

cfe/trunk/docs/ReleaseNotes.rst

- For C code, ``-Wsign-compare``, ``-Wtautological-constant-compare`` and
   ``-Wtautological-constant-out-of-range-compare`` were adjusted to use the
   underlying datatype of ``enum``.
 
 - ``-Wnull-pointer-arithmetic`` now warns about performing pointer arithmetic
   on a null pointer. Such pointer arithmetic has an undefined behavior if the
   offset is nonzero. It also now warns about arithmetic on a null pointer
   treated as a cast from integer to pointer (GNU extension).
 
+- ``-Wzero-as-null-pointer-constant`` was adjusted not to warn on null pointer
+  constants that originate from system macros, except ``NULL`` macro.
+
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Bitrig OS was merged back into OpenBSD, so Bitrig support has been
   removed from Clang/LLVM.
 
 - The default value of _MSC_VER was raised from 1800 to 1911, making it
   compatible with the Visual Studio 2015 and 2017 C++ standard library headers.

cfe/trunk/lib/Sema/Sema.cpp

void Sema::diagnoseNullableToNonnullConversion(QualType DstType,
   Optional<NullabilityKind> TypeNullability = DstType->getNullability(Context);
   if (!TypeNullability || *TypeNullability != NullabilityKind::NonNull)
     return;
 
   Diag(Loc, diag::warn_nullability_lost) << SrcType << DstType;
 }
 
 void Sema::diagnoseZeroToNullptrConversion(CastKind Kind, const Expr* E) {
+  if (Diags.isIgnored(diag::warn_zero_as_null_pointer_constant,
+                      E->getLocStart()))
+    return;
+  // nullptr only exists from C++11 on, so don't warn on its absence earlier.
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
   if (Kind != CK_NullToPointer && Kind != CK_NullToMemberPointer)
     return;
   if (E->IgnoreParenImpCasts()->getType()->isNullPtrType())
     return;
-  // nullptr only exists from C++11 on, so don't warn on its absence earlier.
-  if (!getLangOpts().CPlusPlus11)
+
+  // If it is a macro from system header, and if the macro name is not "NULL",
+  // do not warn.
+  SourceLocation MaybeMacroLoc = E->getLocStart();
+  if (Diags.getSuppressSystemWarnings() &&
+      SourceMgr.isInSystemMacro(MaybeMacroLoc) &&
+      !findMacroSpelling(MaybeMacroLoc, "NULL"))
     return;
 
   Diag(E->getLocStart(), diag::warn_zero_as_null_pointer_constant)
       << FixItHint::CreateReplacement(E->getSourceRange(), "nullptr");
 }
 
 /// ImpCastExprToType - If Expr is not of type 'Type', insert an implicit cast.
 /// If there is already an implicit cast, merge into the existing one.

cfe/trunk/test/SemaCXX/Inputs/warn-zero-nullptr.h

+#define NULL (0)
+#define SYSTEM_MACRO (0)
+#define OTHER_SYSTEM_MACRO (NULL)

cfe/trunk/test/SemaCXX/warn-zero-nullptr.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s -Wzero-as-null-pointer-constant -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -verify %s -isystem %S/Inputs -Wzero-as-null-pointer-constant -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -verify %s -isystem %S/Inputs -DSYSTEM_WARNINGS -Wzero-as-null-pointer-constant -Wsystem-headers -std=c++11
+
+#include <warn-zero-nullptr.h>
+
+#define MACRO (0)
+#define MCRO(x) (x)
 
 struct S {};
 
 int (S::*mp0) = nullptr;
 void (*fp0)() = nullptr;
 void* p0 = nullptr;
 
 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}}
 
-void f0(void* v = 0); // expected-warning{{zero as null pointer constant}}
-void f1(void* v);
+void f0(void* v = MACRO); // expected-warning{{zero as null pointer constant}}
+void f1(void* v = NULL); // expected-warning{{zero as null pointer constant}}
+void f2(void* v = MCRO(0)); // expected-warning{{zero as null pointer constant}}
+void f3(void* v = MCRO(NULL)); // expected-warning{{zero as null pointer constant}}
+void f4(void* v = 0); // expected-warning{{zero as null pointer constant}}
+void f5(void* v);
 
 void g() {
   f1(0); // expected-warning{{zero as null pointer constant}}
 }
 
 // Warn on these too. Matches gcc and arguably makes sense.
 void* pp = (decltype(nullptr))0; // expected-warning{{zero as null pointer constant}}
 void* pp2 = static_cast<decltype(nullptr)>(0); // expected-warning{{zero as null pointer constant}}
 
 // Shouldn't warn.
 namespace pr34362 {
 struct A { operator int*() { return nullptr; } };
 void func() { if (nullptr == A()) {} }
 void func2() { if ((nullptr) == A()) {} }
 }
+
+template <typename T> void TmplFunc0(T var) {}
+void Func0Test() {
+  TmplFunc0<int>(0);
+  TmplFunc0<int*>(0); // expected-warning {{zero as null pointer constant}}
+  TmplFunc0<void*>(0); // expected-warning {{zero as null pointer constant}}
+}
+
+// FIXME: this one probably should not warn.
+template <typename T> void TmplFunc1(int a, T default_value = 0) {} // expected-warning{{zero as null pointer constant}} expected-warning{{zero as null pointer constant}}
+void FuncTest() {
+  TmplFunc1<int>(0);
+  TmplFunc1<int*>(0); // expected-note {{in instantiation of default function argument expression for 'TmplFunc1<int *>' required here}}
+  TmplFunc1<void*>(0);  // expected-note {{in instantiation of default function argument expression for 'TmplFunc1<void *>' required here}}
+}
+
+template<typename T>
+class TemplateClass0 {
+ public:
+  explicit TemplateClass0(T var) {}
+};
+void TemplateClass0Test() {
+  TemplateClass0<int> a(0);
+  TemplateClass0<int*> b(0); // expected-warning {{zero as null pointer constant}}
+  TemplateClass0<void*> c(0); // expected-warning {{zero as null pointer constant}}
+}
+
+template<typename T>
+class TemplateClass1 {
+ public:
+// FIXME: this one should *NOT* warn.
+  explicit TemplateClass1(int a, T default_value = 0) {} // expected-warning{{zero as null pointer constant}} expected-warning{{zero as null pointer constant}}
+};
+void IgnoreSubstTemplateType1() {
+  TemplateClass1<int> a(1);
+  TemplateClass1<int*> b(1); // expected-note {{in instantiation of default function argument expression for 'TemplateClass1<int *>' required here}}
+  TemplateClass1<void*> c(1); // expected-note {{in instantiation of default function argument expression for 'TemplateClass1<void *>' required here}}
+}
+
+#ifndef SYSTEM_WARNINGS
+// Do not warn on *any* other macros from system headers, even if they
+// expand to/their expansion contains NULL.
+void* sys_init = SYSTEM_MACRO;
+void* sys_init2 = OTHER_SYSTEM_MACRO;
+#else
+void* sys_init = SYSTEM_MACRO; // expected-warning {{zero as null pointer constant}}
+void* sys_init2 = OTHER_SYSTEM_MACRO; // expected-warning {{zero as null pointer constant}}
+#endif