Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

This patch extends some existing warnings to utilize the knowledge about the gsl::Pointer and gsl::Owner attributes.

Differential Revision: https://reviews.llvm.org/D64256

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@368072 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/DiagnosticSemaKinds.td

@@ -8107,6 +8107,10 @@ def warn_dangling_member : Warning<
   "%select{binds to|is}2 a temporary object "
   "whose lifetime is shorter than the lifetime of the constructed object">,
   InGroup<DanglingField>;
+def warn_dangling_lifetime_pointer_member : Warning<
+  "initializing pointer member %0 to point to a temporary object "
+  "whose lifetime is shorter than the lifetime of the constructed object">,
+  InGroup<DanglingField>;
 def note_lifetime_extending_member_declared_here : Note<
   "%select{%select{reference|'std::initializer_list'}0 member|"
   "member with %select{reference|'std::initializer_list'}0 subobject}1 "
@@ -8125,6 +8129,10 @@ def warn_new_dangling_reference : Warning<
   "temporary bound to reference member of allocated object "
   "will be destroyed at the end of the full-expression">,
   InGroup<DanglingField>;
+def warn_dangling_lifetime_pointer : Warning<
+  "object backing the pointer "
+  "will be destroyed at the end of the full-expression">,
+  InGroup<Dangling>;
 def warn_new_dangling_initializer_list : Warning<
   "array backing "
   "%select{initializer list subobject of the allocated object|"

lib/Sema/SemaInit.cpp

@@ -6513,6 +6513,7 @@ struct IndirectLocalPathEntry {
     VarInit,
     LValToRVal,
     LifetimeBoundCall,
+    GslPointerInit
   } Kind;
   Expr *E;
   const Decl *D = nullptr;
@@ -6557,6 +6558,40 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
                                                   Expr *Init, ReferenceKind RK,
                                                   LocalVisitor Visit);
 
+template <typename T> static bool isRecordWithAttr(QualType Type) {
+  if (auto *RD = Type->getAsCXXRecordDecl())
+    return RD->getCanonicalDecl()->hasAttr<T>();
+  return false;
+}
+
+static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
+                                    LocalVisitor Visit) {
+  auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
+    Path.push_back({IndirectLocalPathEntry::GslPointerInit, Arg, D});
+    if (Arg->isGLValue())
+      visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
+                                            Visit);
+    else
+      visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
+    Path.pop_back();
+  };
+
+  if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
+    const FunctionDecl *Callee = MCE->getDirectCallee();
+    if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
+      if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()))
+        VisitPointerArg(Callee, MCE->getImplicitObjectArgument());
+    return;
+  }
+
+  if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
+    const auto *Ctor = CCE->getConstructor();
+    const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl();
+    if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())
+      VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]);
+  }
+}
+
 static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
   const TypeSourceInfo *TSI = FD->getTypeSourceInfo();
   if (!TSI)
@@ -6678,8 +6713,10 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
                                        true);
   }
 
-  if (isa<CallExpr>(Init))
+  if (isa<CallExpr>(Init)) {
+    handleGslAnnotatedTypes(Path, Init, Visit);
     return visitLifetimeBoundArguments(Path, Init, Visit);
+  }
 
   switch (Init->getStmtClass()) {
   case Stmt::DeclRefExprClass: {
@@ -6905,8 +6942,10 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
     }
   }
 
-  if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init))
+  if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init)) {
+    handleGslAnnotatedTypes(Path, Init, Visit);
     return visitLifetimeBoundArguments(Path, Init, Visit);
+  }
 
   switch (Init->getStmtClass()) {
   case Stmt::UnaryOperatorClass: {
@@ -6988,6 +7027,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
     case IndirectLocalPathEntry::AddressOf:
     case IndirectLocalPathEntry::LValToRVal:
     case IndirectLocalPathEntry::LifetimeBoundCall:
+    case IndirectLocalPathEntry::GslPointerInit:
       // These exist primarily to mark the path as not permitting or
       // supporting lifetime extension.
       break;
@@ -7000,6 +7040,11 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
   return E->getSourceRange();
 }
 
+static bool pathOnlyInitializesGslPointer(IndirectLocalPath &Path) {
+  return !Path.empty() &&
+         Path.back().Kind == IndirectLocalPathEntry::GslPointerInit;
+}
+
 void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
                                     Expr *Init) {
   LifetimeResult LR = getEntityLifetime(&Entity);
@@ -7016,12 +7061,31 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
     SourceRange DiagRange = nextPathEntryRange(Path, 0, L);
     SourceLocation DiagLoc = DiagRange.getBegin();
 
+    auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
+    bool IsTempGslOwner = MTE && isRecordWithAttr<OwnerAttr>(MTE->getType());
+    bool IsLocalGslOwner =
+        isa<DeclRefExpr>(L) && isRecordWithAttr<OwnerAttr>(L->getType());
+
+    // Skipping a chain of initializing gsl::Pointer annotated objects.
+    // We are looking only for the final source to find out if it was
+    // a local or temporary owner or the address of a local variable/param. We
+    // do not want to follow the references when returning a pointer originating
+    // from a local owner to avoid the following false positive:
+    //   int &p = *localOwner;
+    //   someContainer.add(std::move(localOwner));
+    //   return p;
+    if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) &&
+        !(IsLocalGslOwner && !pathContainsInit(Path)))
+      return true;
+
+    bool IsGslPtrInitWithGslTempOwner =
+        IsTempGslOwner && pathOnlyInitializesGslPointer(Path);
+
     switch (LK) {
     case LK_FullExpression:
       llvm_unreachable("already handled this");
 
     case LK_Extended: {
-      auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
       if (!MTE) {
         // The initialized entity has lifetime beyond the full-expression,
         // and the local entity does too, so don't warn.
@@ -7031,6 +7095,11 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
         return false;
       }
 
+      if (IsGslPtrInitWithGslTempOwner) {
+        Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;
+        return false;
+      }
+
       // Lifetime-extend the temporary.
       if (Path.empty()) {
         // Update the storage duration of the materialized temporary.
@@ -7072,6 +7141,14 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
         // temporary, the program is ill-formed.
         if (auto *ExtendingDecl =
                 ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) {
+          if (IsGslPtrInitWithGslTempOwner) {
+            Diag(DiagLoc, diag::warn_dangling_lifetime_pointer_member)
+                << ExtendingDecl << DiagRange;
+            Diag(ExtendingDecl->getLocation(),
+                 diag::note_ref_or_ptr_member_declared_here)
+                << true;
+            return false;
+          }
           bool IsSubobjectMember = ExtendingEntity != &Entity;
           Diag(DiagLoc, shouldLifetimeExtendThroughPath(Path)
                             ? diag::err_dangling_member
@@ -7112,7 +7189,7 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
 
         if (auto *Member =
                 ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) {
-          bool IsPointer = Member->getType()->isAnyPointerType();
+          bool IsPointer = !Member->getType()->isReferenceType();
           Diag(DiagLoc, IsPointer ? diag::warn_init_ptr_member_to_parameter_addr
                                   : diag::warn_bind_ref_member_to_parameter)
               << Member << VD << isa<ParmVarDecl>(VD) << DiagRange;
@@ -7126,10 +7203,13 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
 
     case LK_New:
       if (isa<MaterializeTemporaryExpr>(L)) {
-        Diag(DiagLoc, RK == RK_ReferenceBinding
-                          ? diag::warn_new_dangling_reference
-                          : diag::warn_new_dangling_initializer_list)
-            << !Entity.getParent() << DiagRange;
+        if (IsGslPtrInitWithGslTempOwner)
+          Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;
+        else
+          Diag(DiagLoc, RK == RK_ReferenceBinding
+                            ? diag::warn_new_dangling_reference
+                            : diag::warn_new_dangling_initializer_list)
+              << !Entity.getParent() << DiagRange;
       } else {
         // We can't determine if the allocation outlives the local declaration.
         return false;
@@ -7172,7 +7252,8 @@ void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
         break;
 
       case IndirectLocalPathEntry::LifetimeBoundCall:
-        // FIXME: Consider adding a note for this.
+      case IndirectLocalPathEntry::GslPointerInit:
+        // FIXME: Consider adding a note for these.
         break;
 
       case IndirectLocalPathEntry::DefaultInit: {

test/Sema/warn-lifetime-analysis-nocfg.cpp

@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int &operator*();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int &operator*();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long &operator*();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long &operator*();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{&i};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p(&i) {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return &j; // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer returningLocalPointer() {
+  MyIntPointer localPointer;
+  return localPointer; // ok
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+int *noFalsePositive(MyIntOwner &o) {
+  MyIntPointer p = o;
+  return &*p; // ok
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p2 = MyLongOwnerWithConversion{}; // TODO ?
+  global2 = MyLongOwnerWithConversion{}; // TODO ?
+}
+
+struct IntVector {
+  int *begin();
+  int *end();
+};
+
+void modelIterators() {
+  int *it = IntVector{}.begin(); // TODO ?
+  (void)it;
+}