[analyzer] Refine suppression mechanism to better support AST checks

[vsavchenko]

Summary

This commit slightly changes the way suppressions were checked before.
Instead of going up from statements and expressions, where bugs were
found, and looking for statements and declarations with attributes,
we traverse function bodies once and map locations with suppressions.

This approach can help with the majority of AST checks that report
directly on source locations. It is not trivial to map locations
back to nodes, so its much easier to map both nodes and suppressions
to locations and reason only on that level.

[vsavchenko]

This change was squashed into https://reviews.llvm.org/D93110

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

 #ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTER_H
 #define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTER_H
 
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
private:
   void FlushReport(BugReportEquivClass& EQ);
 
   /// The set of bug reports tracked by the BugReporter.
   llvm::FoldingSet<BugReportEquivClass> EQClasses;
 
   /// A vector of BugReports for tracking the allocated pointers and cleanup.
   std::vector<BugReportEquivClass *> EQClassesVector;
 
+  /// User-provided in-code suppressions.
+  BugSuppression UserSuppressions;
+
 public:
   BugReporter(BugReporterData &d);
   virtual ~BugReporter();
 
   /// Generate and flush diagnostics for all bug reports.
   void FlushReports();
 
   ArrayRef<PathDiagnosticConsumer*> getPathDiagnosticConsumers() {

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h

+//===- BugSuppression.h - Suppression interface -----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines BugSuppression, a simple interface class incapsulating
+//  all user provided in-code suppressions.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H
+#define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H
+
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallVector.h"
+
+namespace clang {
+class Decl;
+
+namespace ento {
+class BugReport;
+
+class BugSuppression {
+public:
+  /// Return true if the given bug report was explicitly suppressed by the user.
+  bool isSuppressed(const BugReport &);
+
+private:
+  // Overly pessimistic number, to be honest.
+  static constexpr unsigned EXPECTED_NUMBER_OF_SUPPRESSIONS = 8;
+  using CachedRanges =
+      llvm::SmallVector<SourceRange, EXPECTED_NUMBER_OF_SUPPRESSIONS>;
+
+  llvm::DenseMap<const Decl *, CachedRanges> CachedSuppressionLocations;
+};
+
+} // end namespace ento
+} // end namespace clang
+
+#endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

for (PathDiagnosticConsumer *PC : consumers) {
         (*Out)[PC] = std::move(PD);
       }
     }
   }
 
   return Out;
 }
 
-//===----------------------------------------------------------------------===//
-//                             Manual suppressions
-//===----------------------------------------------------------------------===//
-
-namespace {
-llvm::Optional<const SuppressAttr *>
-findSuppressAttribute(ArrayRef<const Attr *> Attrs) {
-  const auto *It =
-      llvm::find_if(Attrs, [](const Attr *A) { return isa<SuppressAttr>(A); });
-
-  if (It != Attrs.end()) {
-    return cast<SuppressAttr>(*It);
-  }
-
-  return llvm::None;
-}
-
-// TODO: In the future, when we come up with good stable IDs for checkers
-//       we can return a list of kinds to ignore, or all if no arguments were
-//       provided.
-llvm::Optional<const SuppressAttr *> findSuppressAttribute(const BugReport &BR,
-                                                           ASTContext &AC) {
-  PathDiagnosticLocation Location = BR.getLocation();
-
-  if (const Stmt *BugStmt = Location.asStmt()) {
-    // If location is statement, it can actually be an expression buried
-    // deeply in other expressions, so the attribute can be few levels above
-    // in the AST.  For this reason, we traverse this statement's parents
-    // and check if they have suppressions.
-    //
-    // The statement itself can have suppression attributes, so let's
-    // start with it.
-    llvm::SmallVector<DynTypedNode, 8> Stack{DynTypedNode::create(*BugStmt)};
-
-    while (!Stack.empty()) {
-      const auto &Current = Stack.back();
-      Stack.pop_back();
-
-      if (const auto *AS = Current.get<AttributedStmt>()) {
-        // Only AttributedStmt out of all statements can have attributes.
-        if (auto Suppress = findSuppressAttribute(AS->getAttrs())) {
-          return Suppress;
-        }
-        // Suppressions can still be further up, so let's continue.
-      } else if (const auto *VD = Current.get<VarDecl>()) {
-        // Bug location could've been somewhere in the init value of
-        // a freshly declared variable.  Even though it looks like the
-        // user applied attribute to a statement, it will apply to a
-        // variable declaration, and this is where we check for it.
-        if (VD->hasAttr<SuppressAttr>()) {
-          return VD->getAttr<SuppressAttr>();
-        }
-      } else if (const auto *D = Current.get<Decl>()) {
-        // If we got to some other form of declaration, we don't want
-        // to go further.  Declaration attributes (in cases other than for
-        // variables) should be used ONLY to suppress warnings reported directly
-        // on that declaration.
-        continue;
-      }
-
-      // Let's continue with the current node's parent(s).
-      for (const auto &Parent : AC.getParents(Current)) {
-        Stack.push_back(Parent);
-      }
-    }
-  } else if (const Decl *BugDecl = Location.asDecl()) {
-    // If we want to report on a declaration, we don't want to check parents
-    // or stuff like that, we simply want to check if that declaration has
-    // a supression attribute.
-    if (BugDecl->hasAttr<SuppressAttr>()) {
-      return BugDecl->getAttr<SuppressAttr>();
-    }
-  } else if (Location.hasValidLocation()) {
-    // This is the trickiest part.  Bug location is simply a SourceLocation
-    // object.  We don't know a node to attach it to.
-    //
-    // At the moment, we check the declaration containing the bug for
-    // suppression attributes.  It is indeed to crude, probably a better
-    // solution would be to suppress issues reported outside of the body
-    // of this declaration in this manner (e.g. reported on curly braces
-    // or on one of the function arguments, or on class parent).
-    if (const Decl *DeclWithIssue = BR.getDeclWithIssue()) {
-      if (DeclWithIssue->hasAttr<SuppressAttr>()) {
-        return DeclWithIssue->getAttr<SuppressAttr>();
-      }
-    }
-  }
-
-  return llvm::None;
-}
-
-bool isSuppressed(const BugReport &R, ASTContext &AC) {
-  // TODO: Introduce stable IDs for checkers and check for those here
-  //       to be more specific.  Attribute without arguments should still
-  //       be considered as "suppress all".
-  //       It is already much finer granularity than what we have now
-  //       (i.e. removing the whole function from the analysis).
-  return findSuppressAttribute(R, AC).hasValue();
-}
-} // end anonymous namespace
-
 void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
   bool ValidSourceLoc = R->getLocation().isValid();
   assert(ValidSourceLoc);
   // If we mess up in a release build, we'd still prefer to just drop the bug
   // instead of trying to go on.
   if (!ValidSourceLoc)
     return;
 
   // If the user asked to suppress this report, we should skip it.
-  if (isSuppressed(*R, getContext()))
+  if (UserSuppressions.isSuppressed(*R))
     return;
 
   // Compute the bug report's hash to determine its equivalence class.
   llvm::FoldingSetNodeID ID;
   R->Profile(ID);
 
   // Lookup the equivance class.  If there isn't one, create it.
   void *InsertPos;

clang/lib/StaticAnalyzer/Core/BugSuppression.cpp

+//===- BugSuppression.cpp - Suppression interface -------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+using Ranges = llvm::SmallVectorImpl<SourceRange>;
+
+inline bool hasSuppression(const Decl *D) { return D->hasAttr<SuppressAttr>(); }
+inline bool hasSuppression(const AttributedStmt *S) {
+  return llvm::any_of(S->getAttrs(),
+                      [](const Attr *A) { return isa<SuppressAttr>(A); });
+}
+
+template <class NodeType> inline SourceRange getRange(const NodeType *Node) {
+  return Node->getSourceRange();
+}
+template <> inline SourceRange getRange(const AttributedStmt *S) {
+  // Begin location for attributed statement node seems to be ALWAYS invalid.
+  //
+  // It is unlikely that we ever report any warnings on suppression
+  // attribute itself, but even if we do, we wouldn't want that warning
+  // to be suppressed by that same attribute.
+  //
+  // Long story short, we can use inner statement and it's not going to break
+  // anything.
+  return getRange(S->getSubStmt());
+}
+
+inline bool isLessOrEqual(SourceLocation LHS, SourceLocation RHS,
+                          const SourceManager &SM) {
+  // SourceManager::isBeforeInTranslationUnit tests for strict
+  // inequality, when we need a non-strict comparison (bug
+  // can be reported directly on the annotated note).
+  // For this reason, we use the following equivalence:
+  //
+  //   A <= B <==> !(B < A)
+  //
+  return !SM.isBeforeInTranslationUnit(RHS, LHS);
+}
+
+inline bool fullyContains(SourceRange Larger, SourceRange Smaller,
+                          const SourceManager &SM) {
+  // Essentially this means:
+  //
+  //   Larger.fullyContains(Smaller)
+  //
+  // However, that method has a very trivial implementation and couldn't
+  // compare regular locations and locations from macro expansions.
+  // We could've converted everything into regular locations as a solution,
+  // but the following solution seems to be the most bulletproof.
+  return isLessOrEqual(Larger.getBegin(), Smaller.getBegin(), SM) &&
+         isLessOrEqual(Smaller.getEnd(), Larger.getEnd(), SM);
+}
+
+class CacheInitializer : public RecursiveASTVisitor<CacheInitializer> {
+public:
+  static void initialize(const Decl *D, Ranges &ToInit) {
+    CacheInitializer(ToInit).TraverseDecl(const_cast<Decl *>(D));
+  }
+
+  bool VisitVarDecl(VarDecl *VD) {
+    // Bug location could be somewhere in the init value of
+    // a freshly declared variable.  Even though it looks like the
+    // user applied attribute to a statement, it will apply to a
+    // variable declaration, and this is where we check for it.
+    return VisitAttributedNode(VD);
+  }
+
+  bool VisitAttributedStmt(AttributedStmt *AS) {
+    // When we apply attributes to statements, it actually creates
+    // a wrapper statement that only contains attributes and the wrapped
+    // statement.
+    return VisitAttributedNode(AS);
+  }
+
+private:
+  template <class NodeType> bool VisitAttributedNode(NodeType *Node) {
+    if (hasSuppression(Node)) {
+      // TODO: In the future, when we come up with good stable IDs for checkers
+      //       we can return a list of kinds to ignore, or all if no arguments
+      //       were provided.
+      addRange(getRange(Node));
+    }
+    // We should keep traversing AST.
+    return true;
+  }
+
+  void addRange(SourceRange R) {
+    if (R.isValid()) {
+      Result.push_back(R);
+    }
+  }
+
+  CacheInitializer(Ranges &R) : Result(R) {}
+  Ranges &Result;
+};
+
+inline bool isInDeclarationBody(PathDiagnosticLocation Location,
+                                const Decl *D) {
+  // Couldn't be inside of body if there is no body.
+  if (!D->hasBody())
+    return false;
+
+  SourceRange BugRange = Location.asRange();
+  // If the warning is reported on the closing brace,
+  // the user should suppress it on the declaration level.
+  if (BugRange.getBegin() == D->getBodyRBrace())
+    return false;
+
+  return fullyContains(D->getBody()->getSourceRange(), BugRange,
+                       Location.getManager());
+}
+
+} // end anonymous namespace
+
+// TODO: Introduce stable IDs for checkers and check for those here
+//       to be more specific.  Attribute without arguments should still
+//       be considered as "suppress all".
+//       It is already much finer granularity than what we have now
+//       (i.e. removing the whole function from the analysis).
+bool BugSuppression::isSuppressed(const BugReport &R) {
+  PathDiagnosticLocation Location = R.getLocation();
+  const Decl *DeclWithIssue = R.getDeclWithIssue();
+
+  if (!Location.isValid() || DeclWithIssue == nullptr)
+    return false;
+
+  // There are two scenarios for suppressions:
+  //
+  //    1. warning is in the declaration's body
+  //    2. warning is somewhere else
+  //
+  // In the first case, we assume that the declaration is some sort
+  // of function and suppressions should be attached to statements
+  // within that body, for finer granularity.
+  //
+  // In the second case, even though it is not necessarily reported
+  // on the declaration itself (e.g. class' base specification), we
+  // still expect suppressions to be attached to the declaration.
+  //
+  // Case #2.
+  if (!isInDeclarationBody(Location, DeclWithIssue))
+    return hasSuppression(DeclWithIssue);
+
+  // Case #1.
+  //
+  // While some warnings are attached to AST nodes (mostly path-sensitive
+  // checks), others are simply associated with a plain source location
+  // or range.  Figuring out the node based on locations can be tricky,
+  // so instead, we traverse the whole body of the declaration and gather
+  // information on ALL suppressions.  After that we can simply check if
+  // any of those suppressions affect the warning in question.
+  //
+  // Traversing AST of a function is not a heavy operation, but for
+  // large functions with a lot of bugs it can make a dent in performance.
+  // In order to avoid this scenario, we cache traversal results.
+  auto InsertionResult = CachedSuppressionLocations.insert(
+      std::make_pair(DeclWithIssue, CachedRanges{}));
+  Ranges &SuppressionRanges = InsertionResult.first->second;
+  if (InsertionResult.second) {
+    // We haven't checked this declaration for suppressions yet!
+    CacheInitializer::initialize(DeclWithIssue, SuppressionRanges);
+  }
+
+  SourceRange BugRange = Location.asRange();
+  const SourceManager &SM = Location.getManager();
+
+  return llvm::any_of(SuppressionRanges,
+                      [BugRange, &SM](SourceRange Suppression) {
+                        return fullyContains(Suppression, BugRange, SM);
+                      });
+}

clang/lib/StaticAnalyzer/Core/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
 )
 
 add_clang_library(clangStaticAnalyzerCore
   APSIntType.cpp
   AnalysisManager.cpp
   AnalyzerOptions.cpp
   BasicValueFactory.cpp
   BlockCounter.cpp
   BugReporter.cpp
   BugReporterVisitors.cpp
+  BugSuppression.cpp
   CallEvent.cpp
   Checker.cpp
   CheckerContext.cpp
   CheckerHelpers.cpp
   CheckerManager.cpp
   CheckerRegistryData.cpp
   CommonBugCategories.cpp
   ConstraintManager.cpp

clang/test/Analysis/suppression-attr.m

 // RUN: %clang_analyze_cc1 -fblocks \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=osx.cocoa.MissingSuperCall \
 // RUN:   -analyzer-checker=osx.cocoa.NSError \
+// RUN:   -analyzer-checker=osx.ObjCProperty \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=alpha.core.CastToStruct \
 // RUN:   -Wno-unused-value -Wno-objc-root-class -verify %s
 
 #define SUPPRESS __attribute__((suppress))
 
 @protocol NSObject
 - (id)retain;
 - (oneway void)release;
 @end

 - (void)encodeWithCoder:(NSCoder *)aCoder;
 @end
 @class NSDictionary;
 @interface NSError : NSObject <NSCopying, NSCoding> {
 }
 + (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictionary *)dict;
 @end
 
+@interface NSMutableString : NSObject
+@end
+
 void dereference_1() {
   int *x = 0;
   *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
 }
 
 void dereference_suppression_1() {
   int *x = 0;
   SUPPRESS { *x; } // no-warning
void dereference_4() {
   int y = *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}}
 }
 
 void dereference_suppression_4() {
   int *x = 0;
   SUPPRESS int y = *x; // no-warning
 }
 
+void retain_release_leak_1() {
+  [[NSMutableString alloc] init]; // expected-warning{{Potential leak of an object of type 'NSMutableString *'}}
+}
+
+void retain_release_leak_suppression_1() {
+  SUPPRESS { [[NSMutableString alloc] init]; }
+}
+
+void retain_release_leak_2(int cond) {
+  id obj = [[NSMutableString alloc] init]; // expected-warning{{Potential leak of an object stored into 'obj'}}
+  if (cond) {
+    [obj release];
+  }
+}
+
+void retain_release_leak__suppression_2(int cond) {
+  SUPPRESS id obj = [[NSMutableString alloc] init];
+  if (cond) {
+    [obj release];
+  }
+}
+
 @interface UIResponder : NSObject {
 }
 - (char)resignFirstResponder;
 @end
 
 @interface Test : UIResponder {
 }
+@property(copy) NSMutableString *mutableStr; // expected-warning{{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}}
 @end
 @implementation Test
 
 - (BOOL)resignFirstResponder {
   return 0;
 } // expected-warning {{The 'resignFirstResponder' instance method in UIResponder subclass 'Test' is missing a [super resignFirstResponder] call}}
 
 - (void)methodWhichMayFail:(NSError **)error {
   // expected-warning@-1 {{Method accepting NSError** should have a non-void return value to indicate whether or not an error occurred}}
 }
 @end
 
 @interface TestSuppress : UIResponder {
 }
+@property(copy) SUPPRESS NSMutableString *mutableStr;
 @end
 @implementation TestSuppress
 
 - (BOOL)resignFirstResponder SUPPRESS {
   return 0;
 } // no-warning
 
 - (void)methodWhichMayFail:(NSError **)error SUPPRESS { // no-warning
 }
 @end
+
+struct AB {
+  int A, B;
+};
+
+struct ABC {
+  int A, B, C;
+};
+
+void ast_checker_1() {
+  struct AB Ab;
+  struct ABC *Abc;
+  Abc = (struct ABC *)&Ab; // expected-warning {{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}}
+}
+
+void ast_checker_suppress_1() {
+  struct AB Ab;
+  struct ABC *Abc;
+  SUPPRESS { Abc = (struct ABC *)&Ab; }
+}