Warn on binding reference to null in copy initialization

[nicholas]

Summary

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!

[aaron.ballman]

lib/Sema/SemaInit.cpp

3513

Can this take a const Expr * instead?

3632

const Expr * (or const auto *) instead?

test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp

39

Missing > in the diagnostic text (I know it's not required, but it looks a bit strange if that's the only part missing from the diagnostic text).

[nicholas]

Richard Smith gave me some review feedback in person, the diagnostic should not be generated when setting up the Steps for the initializer sequence, but instead when InitializerSequence::Perform is called. This appears to work correctly and also catches a few more cases.

I did not expand this to SK_BindReferenceToTemporary, please review this decision. It's also missing missing bit-field and vector element checks that SK_BindReference has.

Also, I'd appreciate comments on the factoring with CheckForNullPointerDereference which was copied+pasted from SemaExpr.cpp. Please review this issue too, I did not expect to land the patch the way it's currently written.

test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp

39

What follows lambda is the path to the file, then the closing )>. I'd rather not include that part. I could make it up with .* just to balance the parens, but I think that's not worth it.

[aaron.ballman]

This generally LGTM, but you should wait for @rsmith to sign off before committing.

test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp

39

Yeah, totally not worth it then. Thank you for the explanation.

[rsmith]

In D19851#420762, @nicholas wrote:

I did not expand this to SK_BindReferenceToTemporary, please review this decision. It's also missing missing bit-field and vector element checks that SK_BindReference has.

That's fine. SK_BindReferenceToTemporary can never bind to a dereferenced null pointer.

lib/Sema/SemaInit.cpp

3514–3518

This comment doesn't make sense for this case. Binding a reference to a dereferenced null pointer isn't something that people would expect to trap. But the idea is the same: we might delete people's code and they're probably not expecting that.

3519–3522

It seems -- borderline -- worth factoring out these four lines between here and SemaExpr. Maybe isProvablyEmptyLvalue or similar, if you feel inclined to do so?

3523

I don't think we need to, or should, care whether the type is volatile-qualified here. The reference binding has undefined behavior either way. In SemaExpr we had this check because we wanted to warn people that we were going to delete their code, which we didn't do in the volatile case, but in this case we may delete the code even if it's a reference-to-volatile.

[nicholas]

lib/Sema/SemaInit.cpp

3514–3518

We don't delete it, generally we "create a null reference" which is exactly what some programmers expect, when they think that references are just syntactically different pointers.

3519–3522

I'll pass. Initially I thought the two CheckForNullPointerDereference functions might be the same but for a diag::ID argument, but they're growing more and more different.

As for isProvablyEmptyLvalue we would still need to get the UO for the diagnostic anyways.

[nicholas]

(Whoops, forgot to generate diff with full context for phab.)

[nicholas]

Unsure why, but the previous diff didn't have the 'volatile' check removed.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

5369

Maybe "binding dereferenced null pointer [...]"?

lib/Sema/SemaInit.cpp

6203

Something fishy in the indentation here?

[nicholas]

Closed by r269572.

include/clang/Basic/DiagnosticSemaKinds.td

def err_typecheck_unary_expr : Error<
   "invalid argument type %0 to unary expression">;
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<
   "ISO C++ does not allow indirection on operand of type %0">,
   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<
   "dereference of type %1 that was reinterpret_cast from type %0 has undefined "
   "behavior">,
   InGroup<UndefinedReinterpretCast>, DefaultIgnore;
 
 def err_objc_object_assignment : Error<

lib/Sema/SemaInit.cpp

if (!Constructor->isInvalidDecl() &&
       }
     }
   }
 
   // Perform overload resolution and return the result.
   return CandidateSet.BestViableFunction(S, DeclLoc, Best);
 }
 
 /// \brief Attempt initialization by constructor (C++ [dcl.init]), which
 /// enumerates the constructors of the initialized entity and performs overload
 /// resolution to select the best.
 /// \param IsListInit     Is this list-initialization?
 /// \param IsInitListCopy Is this non-list-initialization resulting from a
 ///                       list-initialization from {x} where x is the same
 ///                       type as the entity?
 static void TryConstructorInitialization(Sema &S,
                                          const InitializedEntity &Entity,
                                          const InitializationKind &Kind,
                                          MultiExprArg Args, QualType DestType,
                                          InitializationSequence &Sequence,
                                          bool IsListInit = false,
                                          bool IsInitListCopy = false) {
   assert((!IsListInit || (Args.size() == 1 && isa<InitListExpr>(Args[0]))) &&
          "IsListInit must come with a single initializer list argument.");
 
   // The type we're constructing needs to be complete.
   if (!S.isCompleteType(Kind.getLocation(), DestType)) {
static void TryConstructorInitialization(Sema &S,
   // C++11 [over.match.list]p1:
   //   In copy-list-initialization, if an explicit constructor is chosen, the
   //   initializer is ill-formed.
   if (IsListInit && !Kind.AllowExplicit() && CtorDecl->isExplicit()) {
     Sequence.SetFailed(InitializationSequence::FK_ExplicitConstructor);
     return;
   }
 
   // Add the constructor initialization step. Any cv-qualification conversion is
   // subsumed by the initialization.
   bool HadMultipleCandidates = (CandidateSet.size() > 1);
   Sequence.AddConstructorInitializationStep(
       CtorDecl, Best->FoundDecl.getAccess(), DestType, HadMultipleCandidates,
       IsListInit | IsInitListCopy, AsInitializerList);
 }
 
 static bool
static void CheckMoveOnConstruction(Sema &S, const Expr *InitExpr,
 
   LParen = ArgLoc.getLocWithOffset(-1);
 
   S.Diag(CE->getLocStart(), diag::note_remove_move)
       << FixItHint::CreateRemoval(SourceRange(CallBegin, LParen))
       << FixItHint::CreateRemoval(SourceRange(RParen, RParen));
 }
 
+static void CheckForNullPointerDereference(Sema &S, const Expr *E) {
+  // Check to see if we are dereferencing a null pointer.  If so, this is
+  // undefined behavior, so warn about it.  This only handles the pattern
+  // "*null", which is a very syntactic check.
+  if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E->IgnoreParenCasts()))
+    if (UO->getOpcode() == UO_Deref &&
+        UO->getSubExpr()->IgnoreParenCasts()->
+  isNullPointerConstant(S.Context, Expr::NPC_ValueDependentIsNotNull)) {
+    S.DiagRuntimeBehavior(UO->getOperatorLoc(), UO,
+                          S.PDiag(diag::warn_binding_null_to_reference)
+                            << UO->getSubExpr()->getSourceRange());
+  }
+}
+
 ExprResult
 InitializationSequence::Perform(Sema &S,
                                 const InitializedEntity &Entity,
                                 const InitializationKind &Kind,
                                 MultiExprArg Args,
                                 QualType *ResultType) {
   if (Failed()) {
     Diagnose(S, Entity, Kind, Args);
case SK_BindReference:
       // to the result of a cast to reference type.
       if (const InitializedEntity *ExtendingEntity =
               getEntityForTemporaryLifetimeExtension(&Entity))
         if (performReferenceExtension(CurInit.get(), ExtendingEntity))
           warnOnLifetimeExtension(S, Entity, CurInit.get(),
                                   /*IsInitializerList=*/false,
                                   ExtendingEntity->getDecl());
 
+      CheckForNullPointerDereference(S, CurInit.get());
       break;
 
     case SK_BindReferenceToTemporary: {
       // Make sure the "temporary" is actually an rvalue.
       assert(CurInit.get()->isRValue() && "not a temporary");
 
       // Check exception specifications
       if (S.CheckExceptionSpecCompatibility(CurInit.get(), DestType))

test/CXX/expr/expr.prim/expr.prim.lambda/p5.cpp

 // RUN: %clang_cc1 -std=c++11 %s -Winvalid-noreturn -verify
 
 // An attribute-specifier-seq in a lambda-declarator appertains to the
 // type of the corresponding function call operator.
 void test_attributes() {
   auto nrl = [](int x) -> int { if (x > 0) return x; }; // expected-warning{{control may reach end of non-void lambda}}
 
   // FIXME: GCC accepts the [[gnu::noreturn]] attribute here.
   auto nrl2 = []() [[gnu::noreturn]] { return; }; // expected-warning{{attribute 'noreturn' ignored}}
 }
 
 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;
 };
 
 void test_quals() {
   // This function call operator is declared const (9.3.1) if and only
   // if the lambda- expression's parameter-declaration-clause is not
   // followed by mutable.
   auto l = [=](){}; // expected-note{{method is not marked volatile}}
   const decltype(l) lc = l;
   l();
   lc();
 
   auto ml = [=]() mutable{}; // expected-note{{method is not marked const}} \
                              // expected-note{{method is not marked volatile}} 
   const decltype(ml) mlc = ml;
   ml();
   mlc(); // expected-error{{no matching function for call to object of type}}
 
   // It is neither virtual nor declared volatile.
   volatile decltype(l) lv = l;
   volatile decltype(ml) mlv = ml;
   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
 // parameter-declaration-clause of a lambda-declarator.
 int test_default_args() {
   return [](int i = 5, int j = 17) { return i+j;}(5, 6);
 }
 

test/Parser/cxx-casting.cpp

 char postfix_expr_test()
 {
   return reinterpret_cast<char*>(0xdeadbeef)[0];
 }
 
 // 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;
 typedef A* a;
 void test2(char x, struct B * b) {
   (void)const_cast<::c>(&x);
 #if __cplusplus <= 199711L
   // expected-error@-2 {{found '<::' after a const_cast which forms the digraph '<:' (aka '[') and a ':', did you mean '< ::'?}}

test/SemaCXX/cstyle-cast.cpp

void t_529_2()
   int ar[1];
   (void)(const int*)(ar);
   (void)(void (*)())(t_529_2);
 
   (void)(void*)(0);
   (void)(void*)((int*)0);
   (void)(volatile const void*)((const int*)0);
   (void)(A*)((B*)0);
-  (void)(A&)(*((B*)0));
+  (void)(A&)(*((B*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   (void)(const B*)((C1*)0);
-  (void)(B&)(*((C1*)0));
+  (void)(B&)(*((C1*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   (void)(A*)((D*)0);
-  (void)(const A&)(*((D*)0));
+  (void)(const A&)(*((D*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   (void)(int B::*)((int A::*)0);
   (void)(void (B::*)())((void (A::*)())0);
   (void)(A*)((E*)0); // C-style cast ignores access control
   (void)(void*)((const int*)0); // const_cast appended
 
   (void)(int)(Co1());
   (void)(Co2)(1);
   (void)(Co3)((Co4)(Co3()));

test/SemaCXX/functional-cast.cpp

void t_529_2()
   typedef void *voidp;
   (void)voidp(0);
   (void)voidp((int*)0);
   typedef volatile const void *vcvoidp;
   (void)vcvoidp((const int*)0);
   typedef A *Ap;
   (void)Ap((B*)0);
   typedef A &Ar;
-  (void)Ar(*((B*)0));
+  (void)Ar(*((B*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   typedef const B *cBp;
   (void)cBp((C1*)0);
   typedef B &Br;
-  (void)Br(*((C1*)0));
+  (void)Br(*((C1*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   (void)Ap((D*)0);
   typedef const A &cAr;
-  (void)cAr(*((D*)0));
+  (void)cAr(*((D*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   typedef int B::*Bmp;
   (void)Bmp((int A::*)0);
   typedef void (B::*Bmfp)();
   (void)Bmfp((void (A::*)())0);
   (void)Ap((E*)0); // functional-style cast ignores access control
   (void)voidp((const int*)0); // const_cast appended
 
   (void)int(Co1());

test/SemaCXX/new-delete.cpp

struct S {
     S() { new Inner[1]; }
   };
 
   struct T {
   };
 
   template<typename X>
   void tfn() {
-    new (*(PlacementArg*)0) T[1];
+    new (*(PlacementArg*)0) T[1]; // expected-warning 2 {{binding null pointer to reference has undefined behavior}}
   }
 
   void fn() {
-    tfn<int>();
+    tfn<int>();  // expected-note {{in instantiation of function template specialization 'r150682::tfn<int>' requested here}}
   }
 
 }
 
 namespace P12023 {
   struct CopyCounter
   {
       CopyCounter();

test/SemaCXX/static-cast.cpp

void t_529_2()
   int ar[1];
   (void)static_cast<const int*>(ar);
   (void)static_cast<void (*)()>(t_529_2);
 
   (void)static_cast<void*>(0);
   (void)static_cast<void*>((int*)0);
   (void)static_cast<volatile const void*>((const int*)0);
   (void)static_cast<A*>((B*)0);
-  (void)static_cast<A&>(*((B*)0));
+  (void)static_cast<A&>(*((B*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   (void)static_cast<const B*>((C1*)0);
-  (void)static_cast<B&>(*((C1*)0));
+  (void)static_cast<B&>(*((C1*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   (void)static_cast<A*>((D*)0);
-  (void)static_cast<const A&>(*((D*)0));
+  (void)static_cast<const A&>(*((D*)0)); // expected-warning {{binding null pointer to reference has undefined behavior}}
   (void)static_cast<int B::*>((int A::*)0);
   (void)static_cast<void (B::*)()>((void (A::*)())0);
 
   (void)static_cast<int>(Co1());
   (void)static_cast<Co2>(1);
   (void)static_cast<Co3>(static_cast<Co4>(Co3()));
 
   // Bad code below