Fixes and features for Consumed analysis

[chriswailes]

Summary

This patch provides fixes to the code based on feedback, as well as a couple of new features:

• The same functionality is now supported for both CXXOperatorCallExprs and CXXMemberCallExprs.
• Factored out some code in the StmtVisitor.
• Removed unnecessary asserts from SemaDeclAttr.cpp
• Remove variables from the state map when their destructors are encountered.
• Re-use an existing warning for misplaced attributes.
• Started adding documentation for the consumed analysis attributes.

[delesley]

Other than whitespace issues, LGTM.

include/clang/Analysis/Analyses/Consumed.h

27

See if you can avoid making whitespace changes in your edits. Not so important here, but it is important when editing files originally written by others.

[chriswailes]

Removed whitespace edits.

[dblaikie]

Committed in 189059.

docs/LanguageExtensions.rst

 locks.  Arguments must be lockable type, and there must be at least one
 argument.
 
+Consumed Annotation Checking
+============================
+
+Clang supports additional attributes for checking basic resource management
+properties, specifically for unique objects that have a single owning reference.
+The following attributes are currently supported, although **the implementation
+for these annotations is currently in development and are subject to change.**
+
+``consumes``
+------------
+
+Use ``__attribute__((consumes))`` on a method that transitions an object into
+the consumed state.
+
+``callable_when_unconsumed``
+----------------------------
+
+Use ``__attribute__((callable_when_unconsumed))`` to indicate that a method may
+only be called when the object is not in the consumed state.
+
+``tests_unconsumed``
+--------------------
+
+Use `__attribute__((tests_unconsumed))`` to indicate that a method returns true
+if the object is in the unconsumed state.
+
+
 Type Safety Checking
 ====================
 

include/clang/Analysis/Analyses/Consumed.h

 
 namespace clang {
 namespace consumed {
   
   typedef SmallVector<PartialDiagnosticAt, 1> OptionalNotes;
   typedef std::pair<PartialDiagnosticAt, OptionalNotes> DelayedDiag;
   typedef std::list<DelayedDiag> DiagList;

 
     /// \brief Emit the warnings and notes left by the analysis.
     virtual void emitDiagnostics() {}
     
     /// Warn about unnecessary-test errors.
     /// \param VariableName -- The name of the variable that holds the unique
     /// value.

     /// \param Loc -- The SourceLocation of the method invocation.
     virtual void warnUseOfTempWhileConsumed(StringRef MethodName,
                                             SourceLocation Loc) {}
   
     /// Warn about use-in-unknown-state errors.
     /// \param MethodName -- The name of the method that was incorrectly
     /// invoked.

     /// \param Loc -- The SourceLocation of the method invocation.
     virtual void warnUseOfTempInUnknownState(StringRef MethodName,
                                              SourceLocation Loc) {}
     
     /// Warn about use-while-consumed errors.
     /// \param MethodName -- The name of the method that was incorrectly
     /// invoked.

     virtual void warnUseWhileConsumed(StringRef MethodName,
                                       StringRef VariableName,
                                       SourceLocation Loc) {}
     
     /// Warn about use-in-unknown-state errors.
     /// \param MethodName -- The name of the method that was incorrectly
     /// invoked.

                                        StringRef VariableName,
                                        SourceLocation Loc) {}
   };
   
   enum ConsumedState {
     // No state information for the given variable.
     CS_None,

     
     /// \brief Set the consumed state of a given variable.
     void setState(const VarDecl *Var, ConsumedState State);
+    
+    /// \brief Remove the variable from our state map.
+    void remove(const VarDecl *Var);
   };
   
   class ConsumedBlockInfo {

   public:
     
     ConsumedWarningsHandlerBase &WarningsHandler;
     
     ConsumedAnalyzer(ConsumedWarningsHandlerBase &WarningsHandler)
         : WarningsHandler(WarningsHandler) {}
     
     /// \brief Check to see if the type is a consumable type.
     bool isConsumableType(QualType Type);
     

   };
   
   /// \brief Check to see if a function tests an object's validity.
-  bool isTestingFunction(const CXXMethodDecl *MethodDecl);
+  bool isTestingFunction(const FunctionDecl *FunDecl);
   
 }} // end namespace clang::consumed
 

include/clang/Basic/Attr.td

 
 def Consumes : InheritableAttr {
   let Spellings = [GNU<"consumes">];
-  let Subjects = [CXXMethod];
+  let Subjects = [CXXMethod, CXXConstructor];
 }
 
 def TestsConsumed : InheritableAttr {

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_use_of_temp_while_consumed : Warning<
   "invocation of method '%0' on a temporary object while it is in the "
   "'consumed' state">, InGroup<Consumed>, DefaultIgnore;
-def warn_uniqueness_attribute_wrong_decl_type : Warning<
-  "%0 attribute only applies to methods">,
-  InGroup<Consumed>, DefaultIgnore;
 
 // ConsumedStrict warnings
 def warn_use_in_unknown_state : Warning<

lib/Analysis/Consumed.cpp

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/raw_ostream.h"
 
-// TODO: Add support for methods with CallableWhenUnconsumed.
 // TODO: Mark variables as Unknown going into while- or for-loops only if they
 //       are referenced inside that block. (Deferred)
 // TODO: Add a method(s) to identify which method calls perform what state

       StateOrVar.Var = Var;
     }
     
-    ConsumedState getState() { return StateOrVar.State; };
+    ConsumedState getState() const { return StateOrVar.State; };
     
-    const VarDecl * getVar() { return IsVar ? StateOrVar.Var : NULL; };
+    const VarDecl * getVar() const { return IsVar ? StateOrVar.Var : NULL; };
   };
   
   typedef llvm::DenseMap<const Stmt *, PropagationInfo> MapType;
   typedef std::pair<const Stmt *, PropagationInfo> PairType;
   typedef MapType::iterator InfoEntry;
   
   AnalysisDeclContext &AC;
   ConsumedAnalyzer &Analyzer;
   ConsumedStateMap *StateMap;
   MapType PropagationMap;
   
+  void checkCallability(const PropagationInfo &PState,
+                        const FunctionDecl *FunDecl,
+                        const CallExpr *Call);
   void forwardInfo(const Stmt *From, const Stmt *To);
   bool isLikeMoveAssignment(const CXXMethodDecl *MethodDecl);
   

   ConsumedStmtVisitor(AnalysisDeclContext &AC, ConsumedAnalyzer &Analyzer,
                       ConsumedStateMap *StateMap)
       : AC(AC), Analyzer(Analyzer), StateMap(StateMap) {}
   
   void reset() {
     PropagationMap.clear();
   }
 };
 
+// TODO: When we support CallableWhenConsumed this will have to check for
+//       the different attributes and change the behavior bellow. (Deferred)
+void ConsumedStmtVisitor::checkCallability(const PropagationInfo &PState,
+                                           const FunctionDecl *FunDecl,
+                                           const CallExpr *Call) {
+  
+  if (!FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) return;
+  
+  if (PState.IsVar) {
+    const VarDecl *Var = PState.getVar();
+    
+    switch (StateMap->getState(Var)) {
+    case CS_Consumed:
+      Analyzer.WarningsHandler.warnUseWhileConsumed(
+        FunDecl->getNameAsString(), Var->getNameAsString(),
+        Call->getExprLoc());
+      break;
+    
+    case CS_Unknown:
+      Analyzer.WarningsHandler.warnUseInUnknownState(
+        FunDecl->getNameAsString(), Var->getNameAsString(),
+        Call->getExprLoc());
+      break;
+      
+    default:
+      break;
+    }
+    
+  } else {
+    switch (PState.getState()) {
+    case CS_Consumed:
+      Analyzer.WarningsHandler.warnUseOfTempWhileConsumed(
+        FunDecl->getNameAsString(), Call->getExprLoc());
+      break;
+    
+    case CS_Unknown:
+      Analyzer.WarningsHandler.warnUseOfTempInUnknownState(
+        FunDecl->getNameAsString(), Call->getExprLoc());
+      break;
+      
+    default:
+      break;
+    }
+  }
+}
+
 void ConsumedStmtVisitor::forwardInfo(const Stmt *From, const Stmt *To) {
   InfoEntry Entry = PropagationMap.find(From);
   

 
 void ConsumedStmtVisitor::VisitCXXConstructExpr(const CXXConstructExpr *Call) {
   CXXConstructorDecl *Constructor = Call->getConstructor();
   
   ASTContext &CurrContext = AC.getASTContext();
   QualType ThisType = Constructor->getThisType(CurrContext)->getPointeeType();
   

   
   if (Entry != PropagationMap.end()) {
     PropagationInfo PState = Entry->second;
-    if (!PState.IsVar) return;
+    const CXXMethodDecl *MethodDecl = Call->getMethodDecl();
     
-    const CXXMethodDecl *Method = Call->getMethodDecl();
+    checkCallability(PState, MethodDecl, Call);
     
-    if (Method->hasAttr<ConsumesAttr>())
-      StateMap->setState(PState.getVar(), consumed::CS_Consumed);
-    else if (!Method->isConst())
-      StateMap->setState(PState.getVar(), consumed::CS_Unknown);
+    if (PState.IsVar) {
+      if (MethodDecl->hasAttr<ConsumesAttr>())
+        StateMap->setState(PState.getVar(), consumed::CS_Consumed);
+      else if (!MethodDecl->isConst())
+        StateMap->setState(PState.getVar(), consumed::CS_Unknown);
+    }
   }
 }
 

     InfoEntry Entry = PropagationMap.find(Call->getArg(0));
     
     if (Entry != PropagationMap.end()) {
-      
       PropagationInfo PState = Entry->second;
       
-      // TODO: When we support CallableWhenConsumed this will have to check for
-      //       the different attributes and change the behavior bellow.
-      //       (Deferred)
-      if (FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) {
-        if (PState.IsVar) {
-          const VarDecl *Var = PState.getVar();
-          
-          switch (StateMap->getState(Var)) {
-          case CS_Consumed:
-            Analyzer.WarningsHandler.warnUseWhileConsumed(
-              FunDecl->getNameAsString(), Var->getNameAsString(),
-              Call->getExprLoc());
-            break;
-          
-          case CS_Unknown:
-            Analyzer.WarningsHandler.warnUseInUnknownState(
-              FunDecl->getNameAsString(), Var->getNameAsString(),
-              Call->getExprLoc());
-            break;
-            
-          default:
-            break;
-          }
-          
-        } else {
-          switch (PState.getState()) {
-          case CS_Consumed:
-            Analyzer.WarningsHandler.warnUseOfTempWhileConsumed(
-              FunDecl->getNameAsString(), Call->getExprLoc());
-            break;
+      checkCallability(PState, FunDecl, Call);
+      
+      if (PState.IsVar) {
+        if (FunDecl->hasAttr<ConsumesAttr>()) {
+          // Handle consuming operators.
+          StateMap->setState(PState.getVar(), consumed::CS_Consumed);
+        } else if (const CXXMethodDecl *MethodDecl =
+          dyn_cast_or_null<CXXMethodDecl>(FunDecl)) {
           
-          case CS_Unknown:
-            Analyzer.WarningsHandler.warnUseOfTempInUnknownState(
-              FunDecl->getNameAsString(), Call->getExprLoc());
-            break;
-            
-          default:
-            break;
-          }
+          // Handle non-constant member operators.
+          if (!MethodDecl->isConst())
+            StateMap->setState(PState.getVar(), consumed::CS_Unknown);
         }
       }
-      
-      // Handle non-constant member operators.
-      if (const CXXMethodDecl *MethodDecl =
-        dyn_cast_or_null<CXXMethodDecl>(FunDecl)) {
-        
-        if (!MethodDecl->isConst() && PState.IsVar)
-          StateMap->setState(PState.getVar(), consumed::CS_Unknown);
-      }
     }
   }
 }

 };
 
 bool TestedVarsVisitor::VisitCallExpr(CallExpr *Call) {
-  if (const CXXMethodDecl *Method =
-    dyn_cast_or_null<CXXMethodDecl>(Call->getDirectCallee())) {
+  if (const FunctionDecl *FunDecl =
+    dyn_cast_or_null<FunctionDecl>(Call->getDirectCallee())) {
     
-    if (isTestingFunction(Method)) {
+    if (isTestingFunction(FunDecl)) {
       CurrTestLoc = Call->getExprLoc();
       IsUsefulConditional = true;
       return true;

 }
 
 bool TestedVarsVisitor::VisitDeclRefExpr(DeclRefExpr *DeclRef) {
-  if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(DeclRef->getDecl())) {
-    if (StateMap->getState(Var) != consumed::CS_None) {
+  if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(DeclRef->getDecl()))
+    if (StateMap->getState(Var) != consumed::CS_None)
       Test = VarTestResult(Var, CurrTestLoc, !Invert);
-    }
-    
-  } else {
-    IsUsefulConditional = false;
-  }
   
-  return IsUsefulConditional;
+  return true;
 }
 
 bool TestedVarsVisitor::VisitUnaryOperator(UnaryOperator *UnaryOp) {

   Map[Var] = State;
 }
 
+void ConsumedStateMap::remove(const VarDecl *Var) {
+  Map.erase(Var);
+}
 
 bool ConsumedAnalyzer::isConsumableType(QualType Type) {
   const CXXRecordDecl *RD =

     
     if (CurrStates == NULL)
       CurrStates = BlockInfo.getInfo(CurrBlock);
     
     ConsumedStmtVisitor Visitor(AC, *this, CurrStates);
     
     // Visit all of the basic block's statements.
     for (CFGBlock::const_iterator BI = CurrBlock->begin(),
          BE = CurrBlock->end(); BI != BE; ++BI) {
       
-      if (BI->getKind() == CFGElement::Statement)
+      switch (BI->getKind()) {
+      case CFGElement::Statement:
         Visitor.Visit(BI->castAs<CFGStmt>().getStmt());
+        break;
+      case CFGElement::AutomaticObjectDtor:
+        CurrStates->remove(BI->castAs<CFGAutomaticObjDtor>().getVarDecl());
+      default:
+        break;
+      }
     }
     
-    // TODO: Remove any variables that have reached the end of their
-    //       lifetimes from the state map. (Deferred)
-    
     if (const IfStmt *Terminator =
       dyn_cast_or_null<IfStmt>(CurrBlock->getTerminator().getStmt())) {
       

   WarningsHandler.emitDiagnostics();
 }
 
-bool isTestingFunction(const CXXMethodDecl *Method) {
-  return Method->hasAttr<TestsUnconsumedAttr>();
+bool isTestingFunction(const FunctionDecl *FunDecl) {
+  return FunDecl->hasAttr<TestsUnconsumedAttr>();
 }
 
 }} // end namespace clang::consumed

lib/Sema/AnalysisBasedWarnings.cpp

   DefaultPolicy.enableThreadSafetyAnalysis = (unsigned)
     (D.getDiagnosticLevel(diag::warn_double_lock, SourceLocation()) !=
      DiagnosticsEngine::Ignored);
-  DefaultPolicy.enableConsumedAnalysis =
-      (unsigned)(D.getDiagnosticLevel(diag::warn_use_while_consumed,
-                                      SourceLocation()) !=
-                 DiagnosticsEngine::Ignored);
+  DefaultPolicy.enableConsumedAnalysis = (unsigned)
+    (D.getDiagnosticLevel(diag::warn_use_while_consumed, SourceLocation()) !=
+     DiagnosticsEngine::Ignored);
 }
 
 static void flushDiagnostics(Sema &S, sema::FunctionScopeInfo *fscope) {

lib/Sema/SemaDeclAttr.cpp

 
 static bool checkGuardedVarAttrCommon(Sema &S, Decl *D,
                                       const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 0))
     return false;
 

 static bool checkGuardedByAttrCommon(Sema &S, Decl *D,
                                      const AttributeList &Attr,
                                      Expr* &Arg) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 1))
     return false;
 

 
 static bool checkLockableAttrCommon(Sema &S, Decl *D,
                                     const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 0))
     return false;
 

 
 static void handleNoThreadSafetyAnalysis(Sema &S, Decl *D,
                                          const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 0))
     return;
 

 
 static void handleNoSanitizeAddressAttr(Sema &S, Decl *D,
                                       const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 0))
     return;
 

 
 static void handleNoSanitizeMemory(Sema &S, Decl *D,
                                    const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 0))
     return;
 

 
 static void handleNoSanitizeThread(Sema &S, Decl *D,
                                    const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 0))
     return;
 

 static bool checkAcquireOrderAttrCommon(Sema &S, Decl *D,
                                         const AttributeList &Attr,
                                         SmallVectorImpl<Expr *> &Args) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return false;
 

 static bool checkLockFunAttrCommon(Sema &S, Decl *D,
                                    const AttributeList &Attr,
                                    SmallVectorImpl<Expr *> &Args) {
-  assert(!Attr.isInvalid());
-
   // zero or more arguments ok
 
   // check that the attribute is applied to a function

 static bool checkTryLockFunAttrCommon(Sema &S, Decl *D,
                                       const AttributeList &Attr,
                                       SmallVectorImpl<Expr *> &Args) {
-  assert(!Attr.isInvalid());
-
+  
   if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return false;
 

 static bool checkLocksRequiredCommon(Sema &S, Decl *D,
                                      const AttributeList &Attr,
                                      SmallVectorImpl<Expr *> &Args) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return false;
 

 
 static void handleUnlockFunAttr(Sema &S, Decl *D,
                                 const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   // zero or more arguments ok
 
   if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {

 
 static void handleLockReturnedAttr(Sema &S, Decl *D,
                                    const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeNumArgs(S, Attr, 1))
     return;
 

 
 static void handleLocksExcludedAttr(Sema &S, Decl *D,
                                     const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
-
   if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return;
 

 
 static void handleConsumesAttr(Sema &S, Decl *D,
                                const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
   if (!checkAttributeNumArgs(S, Attr, 0)) return;
 
   if (!(isa<CXXMethodDecl>(D) || isa<CXXConstructorDecl>(D))) {
-    S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<
-      Attr.getName();
+    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<
+      Attr.getName() << "methods";
     return;
   }
   

 
 static void handleCallableWhenUnconsumedAttr(Sema &S, Decl *D,
                                              const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
   if (!checkAttributeNumArgs(S, Attr, 0)) return;
 
   if (!isa<CXXMethodDecl>(D)) {
-    S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<
-      Attr.getName();
+    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<
+      Attr.getName() << "methods";
     return;
   }
   

 
 static void handleTestsConsumedAttr(Sema &S, Decl *D,
                                     const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
   if (!checkAttributeNumArgs(S, Attr, 0)) return;
 
   if (!isa<CXXMethodDecl>(D)) {
-    S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<
-      Attr.getName();
+    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<
+      Attr.getName() << "methods";
     return;
   }
   

 
 static void handleTestsUnconsumedAttr(Sema &S, Decl *D,
                                       const AttributeList &Attr) {
-  assert(!Attr.isInvalid());
   if (!checkAttributeNumArgs(S, Attr, 0)) return;
 
   if (!isa<CXXMethodDecl>(D)) {
-    S.Diag(Attr.getLoc(), diag::warn_uniqueness_attribute_wrong_decl_type) <<
-      Attr.getName();
+    S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type) <<
+      Attr.getName() << "methods";
     return;
   }
   

test/SemaCXX/warn-consumed-analysis.cpp

   template <typename U>
   Bar<T>& operator=(Bar<U> &&other);
   
+  void operator()(int a) CONSUMES;
   void operator*(void) const CALLABLE_WHEN_UNCONSUMED;
+  void unconsumedCall(void) const CALLABLE_WHEN_UNCONSUMED;
   
   bool isValid(void) const TESTS_UNCONSUMED;
   operator bool() const TESTS_UNCONSUMED;
+  bool operator!=(nullptr_t) const TESTS_UNCONSUMED;
   
   void constCall(void) const;
   void nonconstCall(void);

   } else {
     *var; // expected-warning {{invocation of method 'operator*' on object 'var' while it is in the 'consumed' state}}
   }
+  
+  if (var != nullptr) {
+    // Empty
+    
+  } else {
+    *var; // expected-warning {{invocation of method 'operator*' on object 'var' while it is in the 'consumed' state}}
+  }
 }
 
 void testCallingConventions(void) {

   *var; // expected-warning {{invocation of method 'operator*' on object 'var' while it is in the 'consumed' state}}
 }
 
+void testConsumes2(void) {
+  Bar<int> var(42);
+  
+  var.unconsumedCall();
+  var(6);
+  
+  var.unconsumedCall(); // expected-warning {{invocation of method 'unconsumedCall' on object 'var' while it is in the 'consumed' state}}
+}
+
 void testSimpleForLoop(void) {
   Bar<int> var;