Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[xazax.hun]

Summary

This patch introduces even more warnings some of them are very compelling. Look at the loop example, it supposed to catch a lot of errors early.

These warnings has been run on more than 300 open source projects. They found a few true positives and no false positives so far.

[xazax.hun]

I have run this successfully on ~192 open source projects.

The results so far:

1.
https://github.com/ANTsX/ANTs/blob/master/Examples/sccan.cxx#L2899
/home/tgahor/data/projects/ANTs/Examples/sccan.cxx:2899:34: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling]
        const char *longName = ( ( *it )->GetLongName() ).c_str();
                                 ^~~~~~~~~~~~~~~~~~~~~~
This seems to be a true positive given how LongName is defined with this macro: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/include/itkMacro.h#L944
Here: https://github.com/ANTsX/ANTs/blob/15f4e4013ed33b9d226c8d1b7d9509b2a8b19ba2/Utilities/antsCommandLineOption.h#L179
This problem has also other instances in this project. True positives, yay!

2.
https://github.com/mgbellemare/Arcade-Learning-Environment/blob/master/ale_python_interface/ale_c_wrapper.h#L10
In file included from /home/tgahor/data/projects/Arcade-Learning-Environment/ale_python_interface/ale_c_wrapper.cpp:1:
/home/tgahor/data/projects/Arcade-Learning-Environment/ale_python_interface/ale_c_wrapper.h:10:68: warning: returning address of local temporary object [-Wreturn-stack-address]
  const char *getString(ALEInterface *ale, const char *key){return ale->getString(key).c_str();}
                                                                   ^~~~~~~~~~~~~~~~~~
This also is a true positive!

3.
https://github.com/assimp/assimp/blob/master/code/Step/STEPFile.h#L910
/home/tgahor/data/projects/assimp/./code/Step/STEPFile.h:910:26: warning: returning address of local temporary object [-Wreturn-stack-address]
                return (*it).second;
                         ^~
/home/tgahor/data/projects/assimp/./code/Step/STEPFile.h:908:45: note: via initialization of variable 'it' here
            const ObjectMap::const_iterator it = objects.find(id);

This one seems to be a false positive, will look into what is the root cause. Very similar false positives appear on jsoncpp, bamtools, gtest, cppcheck, Urho3D projects.

4.
https://github.com/openscenegraph/OpenSceneGraph/blob/master/include/osg/io_utils#L69
/home/tgahor/data/projects/OpenSceneGraph/include/osg/io_utils:69:51: warning: returning address of local temporary object [-Wreturn-stack-address]
        inline const char* c_str() const { return sstream.str().c_str(); }
                                                  ^~~~~~~~~~~~~
This also is a true positive!

Conclusion: it does find true positives! All of the false positives should have the same root cause and I will look into fixing that soon. After the fix I will rerun this on even more projects, but I think this already starts to show the value of these warnings. I believe we will be able to find all the errors I reported and have 0 false positives.

[xazax.hun]

• Rebase

[xazax.hun]

• Rebase, add the results of testing on real world projects to the description.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

6582

If we want to relax the warnings to give more results we could extend the checking of these overloaded operators for annotated types. But this would imply that the user need to have the expected semantics for those types and can only suppress false positives by removing some gsl:::owner/poinnter annotations.

[mgehre]

clang/lib/Sema/SemaInit.cpp

6582

I see those options:

• Either gsl::Owner implies some specific form of those operators (and if that does not hold for a class, then one should not annotate it with gsl::Owner)
• or gsl::Owner only implies some specific behavior for the "gsl::Pointer constructed from gsl::Owner" case and everything else requires additional annotation

I expect that we will experiment a bit in the future to see what works well for real-world code.

[gribozavr]

clang/lib/Sema/SemaInit.cpp

6582

I understand the difficulty, but I don't think it is appropriate to experiment by ourselves -- these attributes are defined in a spec, and if something is not clear, the spec should be clarified.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

6582

This is exactly what is going to happen but I think it would be unfortunate to stall the progress until the new version of the spec materializes. The idea is to keep the implementations and the specs in sync, but as Herb has other projects too, it takes some time to channel the experience back into the spec. As the current version of the warnings found true positives in real world projects and we have yet to see any false positives I would prefer to move forward to maximize utility.

[gribozavr]

clang/lib/Sema/SemaInit.cpp

6582

I don't understand how different implementations can ever converge in that case.

If this language extension is not sufficiently designed yet, maybe it is not ready for inclusion in Clang?

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

6582

The MSVC implementation does not support user defined annotations yet, so we are the first one to ask such questions like is it valid for an user to annotate a type as gsl::Pointer and have an overloaded deref operator with functionality other than accessing the pointee. We already forwarded these concerns to Herb, and he promised to clarify these things in the paper. Once it is clarified, MSVC will also follow it. Since this code will not reach the wider audience until Clang 10 is released and it is pretty easy to change this detail I do not see the justification to postpone the inclusion.

If we postpone the inclusion over and over we will never get enough experience from real world users to ever have enough confidence.

[xazax.hun]

clang/lib/Sema/SemaInit.cpp

6582

Ok, after discussing this with Herb the conclusion is the following. The paper does not have any requirements for any of the methods for Pointers or Owners. If a method has a semantics that is not a good match with the default rules the user can annotate the methods. As method annotations are coming later, the right approach is to only rely on the semantics of these operators for STL types for now, exactly what is implemented in this patch.

And again, just to make it clear, the reason why we want to add this first rather than the annotations because the latter is a rather large patch and we want to gain more real world experience first before committing to a specific approach. Yet, all of the true positives we found so far needs these assumptions about STL types, so it really is useful to have this until we add the support for annotations.

[gribozavr]

clang/lib/Sema/SemaInit.cpp

6582

Okay, since this code is introducing new behavior only for std and builtin types, I think we can do it even though it is not in the spec.

clang/lib/Sema/SemaInit.cpp

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())
+  if (!Callee->getParent()->isInStdNamespace())
     return false;
-  if (!isRecordWithAttr<PointerAttr>(Callee->getReturnType()) &&
-      !Callee->getReturnType()->isPointerType())
+  if (Callee->getReturnType()->isPointerType() ||
+      isRecordWithAttr<PointerAttr>(Callee->getReturnType())) {
+    if (!Callee->getIdentifier())
       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)
+        // Map and set types.
+        .Cases("find", "equal_range", "lower_bound", "upper_bound", true)
         .Default(false);
+  } else if (Callee->getReturnType()->isReferenceType()) {
+    if (!Callee->getIdentifier()) {
+      auto OO = Callee->getOverloadedOperator();
+      return OO == OverloadedOperatorKind::OO_Subscript ||
+             OO == OverloadedOperatorKind::OO_Star;
+    }
+    return llvm::StringSwitch<bool>(Callee->getName())
+        .Cases("front", "back", "at", true)
+        .Default(false);
+  }
+  return false;
 }
 
 static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
                                     LocalVisitor Visit) {
   const FunctionDecl *Callee;
   if (auto *CE = dyn_cast<CallExpr>(Call)) {
     Callee = CE->getDirectCallee();
     if (!Callee)
auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
     Path.pop_back();
   };
 
   if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
     const auto *MD = cast<CXXMethodDecl>(Callee);
     if (shouldTrackImplicitObjectArg(MD))
       VisitPointerArg(MD, MCE->getImplicitObjectArgument());
     return;
+  } else if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(Call)) {
+    if (Callee->isCXXInstanceMember() &&
+        shouldTrackImplicitObjectArg(cast<CXXMethodDecl>(Callee)))
+      VisitPointerArg(Callee, OCE->getArg(0));
+    return;
   }
 
   if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
     const auto *Ctor = CCE->getConstructor();
     const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl();
-    if (RD->hasAttr<OwnerAttr>() && isa<CXXTemporaryObjectExpr>(Call)) {
+    if (RD->hasAttr<OwnerAttr>() && isa<CXXConstructExpr>(Call) &&
+        !Ctor->isCopyOrMoveConstructor()) {
       Path.push_back({IndirectLocalPathEntry::GslOwnerTemporaryInit, Call, RD});
       Visit(Path, Call, RK_ReferenceBinding);
       Path.pop_back();
     } else {
       if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())
         VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]);
     }
   }
if (!Path.empty() &&
         return false;
     }
 
     switch (LK) {
     case LK_FullExpression:
       llvm_unreachable("already handled this");
 
     case LK_Extended: {
-      auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
       if (IsGslPtrInitWithGslTempOwner) {
         Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;
         return false;
       }
+      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;
       }

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

void initLocalGslPtrWithTempOwner() {
   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 ?
 }
 
 namespace std {
 template <typename T>
-struct basic_iterator {};
+struct basic_iterator {
+  basic_iterator operator++();
+  T& operator*();
+};
+
+template<typename T>
+bool operator!=(basic_iterator<T>, basic_iterator<T>);
 
 template <typename T>
 struct vector {
   typedef basic_iterator<T> iterator;
   iterator begin();
+  iterator end();
   T *data();
+  T &at(int n);
 };
 
 template<typename T>
 struct basic_string {
   const T *c_str() const;
 };
 
 template<typename T>
 struct unique_ptr {
   T *get() const;
 };
+
+template<typename T>
+struct optional {
+  optional();
+  optional(const T&);
+  T &operator*();
+};
 }
 
 void modelIterators() {
   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() {

 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}}
+}
+
+void danglingReferenceFromTempOwner() {
+  int &r = *std::optional<int>(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int &r2 = *std::optional<int>(5); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int &r3 = std::vector<int>().at(3); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+std::vector<int> getTempVec();
+std::optional<std::vector<int>> getTempOptVec();
+
+void testLoops() {
+  for (auto i : getTempVec()) // ok
+    ;
+  for (auto i : *getTempOptVec()) // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+    ;
 }
 No newline at end of file