[analyzer] Suppress more MallocChecker positives in reference counting pointer destructors.

[NoQ]

Summary

D43791 wasn't quite enough because we often run out of inlining stack depth limit and for that reason fail to see the atomics we're looking for.

Add a more straightforward false positive suppression that is based on the name of the class. I.e. if we're releasing a pointer in a destructor of a "something shared/intrusive/reference/counting something ptr/pointer something", then any use-after-free or double-free that occurs later would likely be a false positive.

[george.karpenkov]

we often run out of inlining stack depth limit

Can we consider increasing that limit? I'd much rather have a limit on maximum path *length* (which we currently don't have), as longer paths are more likely to be false positives.
On the other hand, I don't see that many issues with paths which perform too many inlinings.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2836

There's lib/Support/Regex.cpp?

2903–2924

auto

[NoQ]

• fix auto.
• don't use regexs yet because it's clean enough anyway; maybe later.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

for (ReallocPairsTy::iterator I = prevMap.begin(), E = prevMap.end();
     SymbolRef sym = I.getKey();
     if (!currMap.lookup(sym))
       return sym;
   }
 
   return nullptr;
 }
 
+static bool isReferenceCountingPointerDestructor(const CXXDestructorDecl *DD) {
+  if (const IdentifierInfo *II = DD->getParent()->getIdentifier()) {
+    StringRef N = II->getName();
+    if (N.contains_lower("ptr") || N.contains_lower("pointer")) {
+      if (N.contains_lower("ref") || N.contains_lower("cnt") ||
+          N.contains_lower("intrusive") || N.contains_lower("shared")) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode(
     const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
     BugReport &BR) {
   const Stmt *S = PathDiagnosticLocation::getStmt(N);
   if (!S)
     return nullptr;
 
   const LocationContext *CurrentLC = N->getLocationContext();
if (isAllocated(RS, RSPrev, S)) {
       Msg = "Memory is allocated";
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                                   "Returned allocated memory");
     } else if (isReleased(RS, RSPrev, S)) {
       Msg = "Memory is released";
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                              "Returning; memory was released");
 
-      // See if we're releasing memory while inlining a destructor (or one of
-      // its callees). If so, enable the atomic-related suppression within that
-      // destructor (and all of its callees), which would kick in while visiting
-      // other nodes (the visit order is from the bug to the graph root).
+      // See if we're releasing memory while inlining a destructor
+      // (or one of its callees). This turns on various common
+      // false positive suppressions.
+      bool FoundAnyDestructor = false;
       for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-        if (isa<CXXDestructorDecl>(LC->getDecl())) {
+        if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
+          if (isReferenceCountingPointerDestructor(DD)) {
+            // This immediately looks like a reference-counting destructor.
+            // We're bad at guessing the original reference count of the object,
+            // so suppress the report for now.
+            BR.markInvalid(getTag(), DD);
+          } else if (!FoundAnyDestructor) {
             assert(!ReleaseDestructorLC &&
                    "There can be only one release point!");
+            // Suspect that it's a reference counting pointer destructor.
+            // On one of the next nodes might find out that it has atomic
+            // reference counting operations within it (see the code above),
+            // and if so, we'd conclude that it likely is a reference counting
+            // pointer destructor.
             ReleaseDestructorLC = LC->getCurrentStackFrame();
             // It is unlikely that releasing memory is delegated to a destructor
             // inside a destructor of a shared pointer, because it's fairly hard
             // to pass the information that the pointer indeed needs to be
             // released into it. So we're only interested in the innermost
             // destructor.
-          break;
+            FoundAnyDestructor = true;
+          }
         }
       }
     } else if (isRelinquished(RS, RSPrev, S)) {
       Msg = "Memory ownership is transferred";
       StackHint = new StackHintGeneratorForSymbol(Sym, "");
     } else if (isReallocFailedCheck(RS, RSPrev, S)) {
       Mode = ReallocationFailed;
       Msg = "Reallocation failed";

test/Analysis/NewDelete-atomics.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
 
 // expected-no-diagnostics
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 typedef enum memory_order {
   memory_order_relaxed = __ATOMIC_RELAXED,
   memory_order_consume = __ATOMIC_CONSUME,
public:
   }
 
   ~IntrusivePtr() {
   // We should not take the path on which the object is deleted.
     if (Ptr->decRef() == 1)
       delete Ptr;
   }
 
-  Obj *getPtr() const { return Ptr; }
+  Obj *getPtr() const { return Ptr; } // no-warning
 };
 
 void testDestroyLocalRefPtr() {
   IntrusivePtr p1(new Obj());
   {
     IntrusivePtr p2(p1);
   }