[PATCH] D19851: Warn on binding reference to null in copy initialization

[Nick Lewycky via cfe-commits]

nicholas created this revision.
nicholas added a reviewer: cfe-commits.

The attached patch adds a warning when placing a call like:
  func(*static_cast<mystruct*>(nullptr));
to the existing -Wnull-dereference warning.

The existing warning catches the case where the empty lvalue undergoes lvalue-to-rvalue conversion. The attached patch adds the check when it is used for copy initialization.

There's some significant opportunity for refactoring with the static CheckForNullPointerDereference function in SemaExpr.cpp (not to be confused with the one I'm adding to SemaInit.cpp). Also, I've chosen different warning text since all cases where the existing warning fires get compiled to nothing, while cases where this warning fires may generate object code (creating "null references" usually).

Please review!

http://reviews.llvm.org/D19851

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaInit.cpp
  test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp
  test/Parser/cxx-casting.cpp

Index: test/Parser/cxx-casting.cpp
===================================================================
--- test/Parser/cxx-casting.cpp
+++ test/Parser/cxx-casting.cpp
@@ -37,7 +37,7 @@
 // This was being incorrectly tentatively parsed.
 namespace test1 {
   template <class T> class A {}; // expected-note 2{{here}}
-  void foo() { A<int>(*(A<int>*)0); }
+  void foo() { A<int>(*(A<int>*)0); } // expected-warning {{binding null pointer to reference has undefined behavior}}
 }
 
 typedef char* c;
Index: test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp
===================================================================
--- test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp
+++ test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp
@@ -11,7 +11,7 @@
 
 template<typename T>
 struct bogus_override_if_virtual : public T {
-  bogus_override_if_virtual() : T(*(T*)0) { }
+  bogus_override_if_virtual() : T(*(T*)0) { } // expected-warning {{binding null pointer to reference has undefined behavior}}
   int operator()() const;
 };
 
@@ -36,7 +36,7 @@
   lv(); // expected-error{{no matching function for call to object of type}}
   mlv(); // expected-error{{no matching function for call to object of type}}
 
-  bogus_override_if_virtual<decltype(l)> bogus;
+  bogus_override_if_virtual<decltype(l)> bogus; // expected-note{{in instantiation of member function 'bogus_override_if_virtual<(lambda}}
 }
 
 // Core issue 974: default arguments (8.3.6) may be specified in the
Index: lib/Sema/SemaInit.cpp
===================================================================
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -3510,6 +3510,23 @@
   return CandidateSet.BestViableFunction(S, DeclLoc, Best);
 }
 
+static void CheckForNullPointerDereference(Sema &S, Expr *E) {
+  // Check to see if we are dereferencing a null pointer.  If so,
+  // and if not volatile-qualified, this is undefined behavior that the
+  // optimizer will delete, so warn about it.  People sometimes try to use this
+  // to get a deterministic trap and are surprised by clang's behavior.  This
+  // only handles the pattern "*null", which is a very syntactic check.
+  if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts()))
+    if (UO->getOpcode() == UO_Deref &&
+        UO->getSubExpr()->IgnoreParenCasts()->
+          isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull) &&
+        !UO->getType().isVolatileQualified()) {
+    S.DiagRuntimeBehavior(UO->getOperatorLoc(), UO,
+                          S.PDiag(diag::warn_binding_null_to_reference)
+                            << UO->getSubExpr()->getSourceRange());
+  }
+}
+
 /// \brief Attempt initialization by constructor (C++ [dcl.init]), which
 /// enumerates the constructors of the initialized entity and performs overload
 /// resolution to select the best.
@@ -3629,6 +3646,10 @@
     return;
   }
 
+  for (Expr *Arg : Args) {
+    CheckForNullPointerDereference(S, Arg);
+  }
+
   // Add the constructor initialization step. Any cv-qualification conversion is
   // subsumed by the initialization.
   bool HadMultipleCandidates = (CandidateSet.size() > 1);
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5359,6 +5359,8 @@
   InGroup<DiagGroup<"void-ptr-dereference">>;
 def warn_indirection_through_null : Warning<
   "indirection of non-volatile null pointer will be deleted, not trap">, InGroup<NullDereference>;
+def warn_binding_null_to_reference : Warning<
+  "binding null pointer to reference has undefined behavior">, InGroup<NullDereference>;
 def note_indirection_through_null : Note<
   "consider using __builtin_trap() or qualifying pointer with 'volatile'">;
 def warn_pointer_indirection_from_incompatible_type : Warning<


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D19851.55941.patch
Type: text/x-patch
Size: 3878 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160503/a0d9bdb6/attachment.bin>