More warnings regarding gsl::Pointer and gsl::Owner attributes

[xazax.hun]

Summary

This patch extends the warnings for additional cases: when we call some well-known methods on a temporary object.

This is using a hard coded list of those well-known functions. Later revisions might replace string matching with checking function annotations from the lifetime papers. There are two reasons to hard code the list this way for now:

1. The spelling and semantics of function annotations might change in the near future after incorporating some implementation related experience so we are not rushing adding those annotations to clang just yet (as opposed to class annotations which are considered quite stable now.)
2. Checking for those well known functions is a huge usability boost for the check.

[xazax.hun]

clang/test/Analysis/inner-pointer.cpp

2 ↗

(On Diff #211214)

I had to disable the compiler warnings for this test file as some of the warnings overlapped with the warnings from the clang static analyzer (which is not bad IMO!). I think this is cleaner than adding expected-warining lines that are unrelated to the analyzer.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

6564 ↗

(On Diff #211214)

The second param is unused, will fix in the next revision after the first round of reviews.

[xazax.hun]

• Remove unused parameter.

[gribozavr]

clang/lib/Sema/SemaInit.cpp

6563 ↗

(On Diff #211581)

This looks like an ad-hoc rule. Is there a way to express it with attributes instead?

6572 ↗

(On Diff #211581)

Should we also check that Callee->getParent is an owner?

Otherwise the check would complain about begin() on, for example, a std::string_view.

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

170 ↗

(On Diff #211581)

Please add a newline.

[xazax.hun]

• Add new line to end of file.
• Rebase, calls no longer need special handling.

clang/lib/Sema/SemaInit.cpp

6563 ↗

(On Diff #211581)

It is indeed an ad-hoc rule. The point is that we do know that std methods behave this way and function attributes will be able to express the same in the future. But we are still experimenting with what is the best spelling, representation etc for those function attributes. So given the value of these checks I do not want to wait until function annotations are upstreamed since we can achieve the same with relatively little code. I run these checks on ~300 open source projects and not a single false positive was found in the latest run.

6572 ↗

(On Diff #211581)

This is intentional. We only warn if the initialization chain can be tracked back to a temporary (or local in some cases) owner.
So we warn for the following code:

const char *trackThroughMultiplePointer() {
  return std::basic_string_view<char>(std::basic_string<char>()).begin(); // expected-warning {{returning address of local temporary object}}
}

[gribozavr]

clang/lib/Sema/SemaInit.cpp

6572 ↗

(On Diff #211581)

Makes sense, but then we should still check that Callee->getParent is an owner or a pointer.

[xazax.hun]

• Fix review comments.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

6572 ↗

(On Diff #211581)

Good point, done.

cfe/trunk/lib/Sema/SemaInit.cpp

static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
                                                   LocalVisitor Visit);
 
 template <typename T> static bool isRecordWithAttr(QualType Type) {
   if (auto *RD = Type->getAsCXXRecordDecl())
     return RD->getCanonicalDecl()->hasAttr<T>();
   return false;
 }
 
+static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
+  if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
+    if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()))
+      return true;
+  if (!Callee->getParent()->isInStdNamespace() || !Callee->getIdentifier())
+    return false;
+  if (!isRecordWithAttr<PointerAttr>(Callee->getThisObjectType()) &&
+      !isRecordWithAttr<OwnerAttr>(Callee->getThisObjectType()))
+    return false;
+  if (!isRecordWithAttr<PointerAttr>(Callee->getReturnType()) &&
+      !Callee->getReturnType()->isPointerType())
+    return false;
+  return llvm::StringSwitch<bool>(Callee->getName())
+      .Cases("begin", "rbegin", "cbegin", "crbegin", true)
+      .Cases("end", "rend", "cend", "crend", true)
+      .Cases("c_str", "data", "get", true)
+      .Default(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());
+    const auto *MD = cast_or_null<CXXMethodDecl>(MCE->getDirectCallee());
+    if (MD && shouldTrackImplicitObjectArg(MD))
+      VisitPointerArg(MD, 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]);

cfe/trunk/test/Analysis/inner-pointer.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer \
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer   \
+// RUN:   -Wno-dangling -Wno-dangling-field -Wno-return-stack-address \
 // RUN:   %s -analyzer-output=text -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 namespace std {
 
 template <typename T>
 void func_ref(T &a);
 

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 &);
MyIntPointer ownershipTransferToMyPointer() {
   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;
 }
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}}
 }
 
+MyIntOwner makeTempOwner();
+
+MyIntPointer danglingGslPtrFromTemporary2() {
+  return makeTempOwner(); // 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();
+namespace std {
+template <typename T>
+struct basic_iterator {};
+
+template <typename T>
+struct vector {
+  typedef basic_iterator<T> iterator;
+  iterator begin();
+  T *data();
+};
+
+template<typename T>
+struct basic_string {
+  const T *c_str() const;
+};
+
+template<typename T>
+struct unique_ptr {
+  T *get() const;
 };
+}
 
 void modelIterators() {
-  int *it = IntVector{}.begin(); // TODO ?
+  std::vector<int>::iterator it = std::vector<int>().begin(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
   (void)it;
 }
+
+std::vector<int>::iterator modelIteratorReturn() {
+  return std::vector<int>().begin(); // expected-warning {{returning address of local temporary object}}
+}
+
+const char *danglingRawPtrFromLocal() {
+  std::basic_string<char> s;
+  return s.c_str(); // expected-warning {{address of stack memory associated with local variable 's' returned}}
+}
+
+const char *danglingRawPtrFromTemp() {
+  return std::basic_string<char>().c_str(); // expected-warning {{returning address of local temporary object}}
+}
+
+std::unique_ptr<int> getUniquePtr();
+
+int *danglingUniquePtrFromTemp() {
+  return getUniquePtr().get(); // expected-warning {{returning address of local temporary object}}
+}
+
+int *danglingUniquePtrFromTemp2() {
+  return std::unique_ptr<int>().get(); // expected-warning {{returning address of local temporary object}}
+}