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

[xazax.hun]

Summary

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

The implicit assumption of this patch is that if a class annotated with gsl::Pointer is created from a gsl::Owner (through conversion operator or a constructor taking the owner as the first argument) the Pointer will point to the buffer of the Owner, so it will dangle after the lifetime of the Owner ends.

[gribozavr]

clang/lib/Sema/SemaInit.cpp

6510 ↗

(On Diff #208206)

What is this name supposed to mean? Initialization of a "lifetime pointer"? What's a "lifetime pointer"?

If you were imitating "LifetimeBoundCall", it is a reference to the attribute: https://clang.llvm.org/docs/AttributeReference.html#lifetimebound .

6587 ↗

(On Diff #208206)

I feel like the code would be better off if we defined another function for handling gsl::Pointer semantics.

This function is for clang::lifetimebound, and mixing the logic is confusing IMO.

7049 ↗

(On Diff #208206)

Sorry, I can't parse this: "... initializing lifetime Pointer ..."

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

10 ↗

(On Diff #208206)

= nullptr

16 ↗

(On Diff #208206)

Can T and MyOwner be the same type?

It is confusing to have two -- for example, toOwner() returning T instead of MyOwner is confusing.

20 ↗

(On Diff #208206)

"ReleaseAsMyPointer'

21 ↗

(On Diff #208206)

"ReleaseAsRawPointer"

22 ↗

(On Diff #208206)

This method is confusing -- is it a name that the warning is supposed to know about?

25 ↗

(On Diff #208206)

It would be helpful to rename this test function and other functions below according to what they test.

f, g, g2, i, i2 -- unclear what the test is supposed to be about, and why tests are grouped like that.

33 ↗

(On Diff #208206)

Why is the last line OK? Looks like a false negative to me -- the pointer on the heap is pointing to a local variable, just like in f, which produces a warning.

If it is an intentional false negative, please annotate it as such in comments.

38 ↗

(On Diff #208206)

Is "release" a magic name that the warning understands?

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

6510 ↗

(On Diff #208206)

When we were working on the lifetime analysis we needed to distinguish raw pointers from the user defined types that are categorized as pointers. We adopted this notion of Lifetime Pointer which stands for the user defined type that is annotated with gsl::Pointer.

In case this is confusing I could rename it GslPointerInit and GslTempOwner. What do you think?

7049 ↗

(On Diff #208206)

We called gsl::Pointer annotated types lifetime pointers (to distinguish from raw pointers). Should we use gsl::Pointer in the comments too? Or do you have an alternative in mind?

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

16 ↗

(On Diff #208206)

I agree that T might not be a great name. The reason why we have two types because one can be converted to a Pointer using a conversion operator the other can be converted using a one argument non-explicit constructor. These two are generating different ASTs, thus I wanted to test both ways.

33 ↗

(On Diff #208206)

Yeah, this is an intentional false negative as we do not have enough information in a statement local analysis to warn about this case. Will fix the comment thanks!

38 ↗

(On Diff #208206)

Method calls are not handled yet, but the idea is to understand some STL specific methods, like the std::unique_ptr::release.

[xazax.hun]

• Address review comments.

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

22 ↗

(On Diff #208206)

Not yet, see my other answer below.

[xazax.hun]

• Fix a typo.

[gribozavr]

clang/lib/Sema/SemaInit.cpp

6549 ↗

(On Diff #208441)

Move closer to the point of usage?

6549 ↗

(On Diff #208441)

re: name, pathOnlyContainsGslPointerInit or pathOnlyInitializesGslPointer

6564 ↗

(On Diff #208441)

"isRecordWithAttr" ?

7072 ↗

(On Diff #208441)

Please adjust the name.

7076 ↗

(On Diff #208441)

Is it important that the whole path only contains gsl::Pointer nodes?

6510 ↗

(On Diff #208206)

My suggestion is to refer to a type that was annotated with gsl::Pointer as a "gsl::Pointer type" in prose, or "GslPointer" where you need an identifier.

So I agree about GslPointerInit.

As for the second one, it seems like GslOwnerTemporaryInit would be more fitting -- WDYT? It is an initialization of a temporary that has a gsl::Owner type.

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

16 ↗

(On Diff #208206)

Please explain this distinction in the comments in the test. If you didn't tell me just now, I would never know.

Maybe it is also a good idea to split the MyPointer type then, let each owner have its own pointer. Right now having one pointer with two owners is confusing, it suggests that owners are somehow related, but they aren't.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

7076 ↗

(On Diff #208441)

We are trying to figure out what do we initialize a gsl::Pointer annotated class. The reason why we have multiple GslPointerInits in the path is that we usually can see a lot of intermediate temporary objects. Looking at the AST of the examples I have so far the other kind of paths does not seem to be relevant. I can imagine refining this condition later after examining a false negative but I think starting with the most conservative approach should be reasonable for warnings.

[xazax.hun]

• Address review comments.

[gribozavr]

I'm not an expert in SemaInit code, but this change LGTM.

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

8 ↗

(On Diff #208709)

Not needed anymore.

25 ↗

(On Diff #208709)

Would be nice if the second pair of types had clearly related names, like the first pair (MyOwner/MyPointer).

[xazax.hun]

Thanks for the review! I will update the patch soon. As there is a dependency that is not accepted yet Richard (who authored the code I extended) might have some chance to take a look at this patch.

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

25 ↗

(On Diff #208709)

Would renaming MyPtrFromConv to PointerFromConv be enough to suggest the relationship?

[gribozavr]

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

25 ↗

(On Diff #208709)

Yes, that would be sufficient. However my best suggestion would be:

MyOwner => MyIntOwner
MyPointer => MyIntPointer

OwnerWithConv => MyLongOwnerWithConversion
MyPtrFromConv => MyLongPointerFromConversion

Also makes it clear which two types are related together.

[xazax.hun]

• Address the rest of the review comments.

[xazax.hun]

• Fix a TODO and add some more tests.

[xazax.hun]

• Fix a false positive from previous change.

[xazax.hun]

• Fix a false positive case found by running over ~200 open source projects

[gribozavr]

clang/lib/Sema/SemaInit.cpp

7095 ↗

(On Diff #211580)

It is unclear to me why if (IsGslPtrInitWithGslTempOwner) is before if (!MTE), seems like that exclusion should apply to our case, too.

[xazax.hun]

• A small refactoring based on an observation from a review comment.

[xazax.hun]

• Actually move the code snippet in question.

clang/lib/Sema/SemaInit.cpp

7095 ↗

(On Diff #211580)

The reason was that I always skipped MTEs and was looking for the expr that actually initialized MTEs. Relying on MTEs, however, turned out to simplify the code a bit. Updating the diff, thanks!

[xazax.hun]

Thanks for the review!
Since I did some refactoring I will wait for an additional accept before committing.

[gribozavr]

clang/lib/Sema/SemaInit.cpp

7031 ↗

(On Diff #212205)

This change would be best committed separately (preferably with a test).

7046 ↗

(On Diff #212205)

I think you can go back to llvm::c_find now?

7076 ↗

(On Diff #212205)

"Owner"

7077 ↗

(On Diff #212205)

Why is it a false positive? std::move left memory owned by localOwner in unspecified state.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

7077 ↗

(On Diff #212205)

I saw user code relying on the semantics of certain classes. E.g. they assume if a std::unique_ptr is moved the pointee is still in place, so it is safe to return a reference to the pointee. Do you think those cases should be diagnosed too?

[gribozavr]

clang/lib/Sema/SemaInit.cpp

7077 ↗

(On Diff #212205)

It is... debatable. It is not obvious whether the lifetime of the pointed-to memory has ended or not without more detailed lifetime annotations. I think it is fair to silence it, however, I think the comment should be updated to explain the situation in a more detailed way, since without context it looks like a use-after-move.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

7077 ↗

(On Diff #212205)

Do you think renaming localOwner to uniquePtr would be sufficient or do you want me to extend the text too?

[leonardchan]

Hi. I noticed in our builders that both of the warnings introduced in this patch are being diagnosed for pointers that don't use GSL at all. I'm attempting to make a small reproducer now.

[xazax.hun]

In D64256#1625998, @leonardchan wrote:

Hi. I noticed in our builders that both of the warnings introduced in this patch are being diagnosed for pointers that don't use GSL at all. I'm attempting to make a small reproducer now.

Hi!

Clang now apply GSL annotations for some STL types automatically. So this is the expected behavior. Are any of those warnings unwanted/false positive with the newest Clang version? If so, please share the reproducers and I will either fix them ASAP or revert/turn off the warnings.

[leonardchan]

In D64256#1626025, @xazax.hun wrote:

In D64256#1625998, @leonardchan wrote:

Hi. I noticed in our builders that both of the warnings introduced in this patch are being diagnosed for pointers that don't use GSL at all. I'm attempting to make a small reproducer now.

Hi!

Clang now apply GSL annotations for some STL types automatically. So this is the expected behavior. Are any of those warnings unwanted/false positive with the newest Clang version? If so, please share the reproducers and I will either fix them ASAP or revert/turn off the warnings.

Thanks! I didn't know that they were applied to stl types. This could explain why the warning is raised in my case (std::string), but I don't think there's anything wrong in this example that would lead to a warning:

leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat ~/misc/test.cpp
#include <string>

std::string MakeStr();
void other_func(const char *s);

void func(std::string s) {
  const char *x = MakeStr().c_str();
  other_func(x);
}
leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
/usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling]
  const char *x = MakeStr().c_str();
                  ^~~~~~~~~
1 warning generated.

This was made using a release build from ToT clang.

[xazax.hun]

In D64256#1626279, @leonardchan wrote:

In D64256#1626025, @xazax.hun wrote:

In D64256#1625998, @leonardchan wrote:

Hi. I noticed in our builders that both of the warnings introduced in this patch are being diagnosed for pointers that don't use GSL at all. I'm attempting to make a small reproducer now.

Hi!

Clang now apply GSL annotations for some STL types automatically. So this is the expected behavior. Are any of those warnings unwanted/false positive with the newest Clang version? If so, please share the reproducers and I will either fix them ASAP or revert/turn off the warnings.

Thanks! I didn't know that they were applied to stl types. This could explain why the warning is raised in my case (std::string), but I don't think there's anything wrong in this example that would lead to a warning:

leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat ~/misc/test.cpp
#include <string>

std::string MakeStr();
void other_func(const char *s);

void func(std::string s) {
  const char *x = MakeStr().c_str();
  other_func(x);
}
leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
/usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling]
  const char *x = MakeStr().c_str();
                  ^~~~~~~~~
1 warning generated.

This was made using a release build from ToT clang.

Thanks for the repro! I think this is a true positive. MakeStr will create a temporary object, and c_str() will create a pointer that points into the buffer owned by the temporary object. At the end of the full expression the temporary object is destroyed and the result of c_str will dangle.

[leonardchan]

In D64256#1626329, @xazax.hun wrote:

In D64256#1626279, @leonardchan wrote:

In D64256#1626025, @xazax.hun wrote:

In D64256#1625998, @leonardchan wrote:

Hi. I noticed in our builders that both of the warnings introduced in this patch are being diagnosed for pointers that don't use GSL at all. I'm attempting to make a small reproducer now.

Hi!

Clang now apply GSL annotations for some STL types automatically. So this is the expected behavior. Are any of those warnings unwanted/false positive with the newest Clang version? If so, please share the reproducers and I will either fix them ASAP or revert/turn off the warnings.

Thanks! I didn't know that they were applied to stl types. This could explain why the warning is raised in my case (std::string), but I don't think there's anything wrong in this example that would lead to a warning:

leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat ~/misc/test.cpp
#include <string>

std::string MakeStr();
void other_func(const char *s);

void func(std::string s) {
  const char *x = MakeStr().c_str();
  other_func(x);
}
leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
/usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling]
  const char *x = MakeStr().c_str();
                  ^~~~~~~~~
1 warning generated.

This was made using a release build from ToT clang.

Thanks for the repro! I think this is a true positive. MakeStr will create a temporary object, and c_str() will create a pointer that points into the buffer owned by the temporary object. At the end of the full expression the temporary object is destroyed and the result of c_str will dangle.

Ah you're right! I accidentally glossed over that. This means your patch is working as expected then. Thanks!

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def err_dangling_member : Error<
   "whose lifetime would be shorter than the lifetime of "
   "the constructed object">;
 def warn_dangling_member : Warning<
   "%select{reference|backing array for 'std::initializer_list'}2 "
   "%select{|subobject of }1member %0 "
   "%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 "
   "declared here">;
 def warn_dangling_variable : Warning<
   "%select{temporary %select{whose address is used as value of|"
   "%select{|implicitly }2bound to}4 "
   "%select{%select{|reference }4member of local variable|"
   "local %select{variable|reference}4}1|"
   "array backing "
   "%select{initializer list subobject of local variable|"
   "local initializer list}1}0 "
   "%select{%3 |}2will be destroyed at the end of the full-expression">,
   InGroup<Dangling>;
 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|"
   "the allocated initializer list}0 "
   "will be destroyed at the end of the full-expression">,
   InGroup<DanglingInitializerList>;
 def warn_unsupported_lifetime_extension : Warning<
   "sorry, lifetime extension of "

cfe/trunk/lib/Sema/SemaInit.cpp

 /// the initializer are included.
 struct IndirectLocalPathEntry {
   enum EntryKind {
     DefaultInit,
     AddressOf,
     VarInit,
     LValToRVal,
     LifetimeBoundCall,
+    GslPointerInit
   } Kind;
   Expr *E;
   const Decl *D = nullptr;
   IndirectLocalPathEntry() {}
   IndirectLocalPathEntry(EntryKind K, Expr *E) : Kind(K), E(E) {}
   IndirectLocalPathEntry(EntryKind K, Expr *E, const Decl *D)
       : Kind(K), E(E), D(D) {}
 };

 static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
                                              Expr *Init, LocalVisitor Visit,
                                              bool RevisitSubinits);
 
 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)
     return false;
   // Don't declare this variable in the second operand of the for-statement;
   // GCC miscompiles that by ending its lifetime before evaluating the
   // third operand. See gcc.gnu.org/PR86769.
   AttributedTypeLoc ATL;
static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
   } while (Init != Old);
 
   if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(Init)) {
     if (Visit(Path, Local(MTE), RK))
       visitLocalsRetainedByInitializer(Path, MTE->GetTemporaryExpr(), Visit,
                                        true);
   }
 
-  if (isa<CallExpr>(Init))
+  if (isa<CallExpr>(Init)) {
+    handleGslAnnotatedTypes(Path, Init, Visit);
     return visitLifetimeBoundArguments(Path, Init, Visit);
+  }
 
   switch (Init->getStmtClass()) {
   case Stmt::DeclRefExprClass: {
     // If we find the name of a local non-reference parameter, we could have a
     // lifetime problem.
     auto *DRE = cast<DeclRefExpr>(Init);
     auto *VD = dyn_cast<VarDecl>(DRE->getDecl());
     if (VD && VD->hasLocalStorage() &&
for (Expr *E : LE->capture_inits()) {
       if (E->isGLValue())
         visitLocalsRetainedByReferenceBinding(Path, E, RK_ReferenceBinding,
                                               Visit);
       else
         visitLocalsRetainedByInitializer(Path, E, Visit, true);
     }
   }
 
-  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: {
     auto *UO = cast<UnaryOperator>(Init);
     // If the initializer is the address of a local, we could have a lifetime
     // problem.
     if (UO->getOpcode() == UO_AddrOf) {
       // If this is &rvalue, then it's ill-formed and we have already diagnosed

 /// Find the range for the first interesting entry in the path at or after I.
 static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
                                       Expr *E) {
   for (unsigned N = Path.size(); I != N; ++I) {
     switch (Path[I].Kind) {
     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;
 
     case IndirectLocalPathEntry::DefaultInit:
     case IndirectLocalPathEntry::VarInit:
       return Path[I].E->getSourceRange();
     }
   }
   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);
   LifetimeKind LK = LR.getInt();
   const InitializedEntity *ExtendingEntity = LR.getPointer();
 
   // If this entity doesn't have an interesting lifetime, don't bother looking
   // for temporaries within its initializer.
   if (LK == LK_FullExpression)
     return;
 
   auto TemporaryVisitor = [&](IndirectLocalPath &Path, Local L,
                               ReferenceKind RK) -> bool {
     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.
         //
         // FIXME: We should consider warning if a static / thread storage
         // duration variable retains an automatic storage duration local.
         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.
         // FIXME: Rebuild the expression instead of mutating it.
         MTE->setExtendingDecl(ExtendingEntity->getDecl(),
                               ExtendingEntity->allocateManglingNumber());
         // Also visit the temporaries lifetime-extended by this initializer.
         return true;
auto TemporaryVisitor = [&](IndirectLocalPath &Path, Local L,
 
     case LK_MemInitializer: {
       if (isa<MaterializeTemporaryExpr>(L)) {
         // Under C++ DR1696, if a mem-initializer (or a default member
         // initializer used by the absence of one) would lifetime-extend a
         // 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
                             : diag::warn_dangling_member)
               << ExtendingDecl << IsSubobjectMember << RK << DiagRange;
           // Don't bother adding a note pointing to the field if we're inside
           // its default member initializer; our primary diagnostic points to
           // the same place in that case.
case LK_MemInitializer: {
         if (!VD) {
           // A member was initialized to a local block.
           // FIXME: Warn on this.
           return false;
         }
 
         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;
           Diag(Member->getLocation(),
                diag::note_ref_or_ptr_member_declared_here)
               << (unsigned)IsPointer;
         }
       }
       break;
     }
 
     case LK_New:
       if (isa<MaterializeTemporaryExpr>(L)) {
+        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;
       }
       break;
 
     case LK_Return:
     case LK_StmtExprResult:
for (unsigned I = 0; I != Path.size(); ++I) {
       switch (Elem.Kind) {
       case IndirectLocalPathEntry::AddressOf:
       case IndirectLocalPathEntry::LValToRVal:
         // These exist primarily to mark the path as not permitting or
         // supporting lifetime extension.
         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: {
         auto *FD = cast<FieldDecl>(Elem.D);
         Diag(FD->getLocation(), diag::note_init_with_default_member_initalizer)
             << FD << nextPathEntryRange(Path, I + 1, L);
         break;
       }

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

+// 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;
+}