[analyzer] pr39348: MallocChecker: Realize that sized delete isn't custom delete.

MallocChecker no longer thinks that operator delete() that accepts the size of
the object to delete (available since C++14 or under -fsized-deallocation)
is some weird user-defined operator. Instead, it handles it like normal delete.

Additionally, it exposes a regression in NewDelete-intersections.mm's
testStandardPlacementNewAfterDelete() test, where the diagnostic is delayed
from before the call of placement new into the code of placement new
in the header. This happens because the check for pass-into-function-after-free
for placement arguments is located in checkNewAllocator(), which happens after
the allocator is inlined, which is too late. Move this use-after-free check
into checkPreCall instead, where it works automagically because the guard
that prevents it from working is useless and can be removed as well.

This commit causes regressions under -analyzer-config
c++-allocator-inlining=false but this option is essentially unsupported
because the respective feature has been enabled by default quite a while ago.

Differential Revision: https://reviews.llvm.org/D53543

llvm-svn: 345802

3 files changed, 53 insertions, 55 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index ba1663838cf..fbd8a9d4d87 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -712,10 +712,8 @@ bool MallocChecker::isCMemFunction(const FunctionDecl *FD,
   return false;
 }
 
-// Tells if the callee is one of the following:
-// 1) A global non-placement new/delete operator function.
-// 2) A global placement operator function with the single placement argument
-//    of type std::nothrow_t.
+// Tells if the callee is one of the builtin new/delete operators, including
+// placement operators and other standard overloads.
 bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD,
                                         ASTContext &C) const {
   if (!FD)
@@ -726,23 +724,11 @@ bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD,
       Kind != OO_Delete && Kind != OO_Array_Delete)
     return false;
 
-  // Skip all operator new/delete methods.
-  if (isa<CXXMethodDecl>(FD))
-    return false;
-
-  // Return true if tested operator is a standard placement nothrow operator.
-  if (FD->getNumParams() == 2) {
-    QualType T = FD->getParamDecl(1)->getType();
-    if (const IdentifierInfo *II = T.getBaseTypeIdentifier())
-      return II->getName().equals("nothrow_t");
-  }
-
-  // Skip placement operators.
-  if (FD->getNumParams() != 1 || FD->isVariadic())
-    return false;
-
-  // One of the standard new/new[]/delete/delete[] non-placement operators.
-  return true;
+  // This is standard if and only if it's not defined in a user file.
+  SourceLocation L = FD->getLocation();
+  // If the header for operator delete is not included, it's still defined
+  // in an invalid source location. Check to make sure we don't crash.
+  return !L.isValid() || C.getSourceManager().isInSystemHeader(L);
 }
 
 llvm::Optional<ProgramStateRef> MallocChecker::performKernelMalloc(
@@ -1087,12 +1073,6 @@ static bool treatUnusedNewEscaped(const CXXNewExpr *NE) {
 void MallocChecker::processNewAllocation(const CXXNewExpr *NE,
                                          CheckerContext &C,
                                          SVal Target) const {
-  if (NE->getNumPlacementArgs())
-    for (CXXNewExpr::const_arg_iterator I = NE->placement_arg_begin(),
-         E = NE->placement_arg_end(); I != E; ++I)
-      if (SymbolRef Sym = C.getSVal(*I).getAsSymbol())
-        checkUseAfterFree(Sym, C, *I);
-
   if (!isStandardNewDelete(NE->getOperatorNew(), C.getASTContext()))
     return;
 
@@ -2438,10 +2418,6 @@ void MallocChecker::checkPreCall(const CallEvent &Call,
          isCMemFunction(FD, Ctx, AF_IfNameIndex,
                         MemoryOperationKind::MOK_Free)))
       return;
-
-    if (ChecksEnabled[CK_NewDeleteChecker] &&
-        isStandardNewDelete(FD, Ctx))
-      return;
   }
 
   // Check if the callee of a method is deleted.
diff --git a/clang/test/Analysis/NewDelete-custom.cpp b/clang/test/Analysis/NewDelete-custom.cpp
index f5a2952699f..8c4d9a663a3 100644
--- a/clang/test/Analysis/NewDelete-custom.cpp
+++ b/clang/test/Analysis/NewDelete-custom.cpp
@@ -1,12 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=false -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=false -DLEAKS=1 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -DALLOCATOR_INLINING=1 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS=1 -DALLOCATOR_INLINING=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -fblocks -verify %s -analyzer-config c++-allocator-inlining=false
 #include "Inputs/system-header-simulator-cxx.h"
 
-#if !(LEAKS && !ALLOCATOR_INLINING)
 // expected-no-diagnostics
-#endif
 
 
 void *allocator(std::size_t size);
@@ -24,24 +20,18 @@ public:
 void testNewMethod() {
   void *p1 = C::operator new(0); // no warn
 
-  C *p2 = new C; // no warn
+  C *p2 = new C; // no-warning
 
-  C *c3 = ::new C;
+  C *c3 = ::new C; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'c3'}}
-#endif
 
 void testOpNewArray() {
   void *p = operator new[](0); // call is inlined, no warn
 }
 
 void testNewExprArray() {
-  int *p = new int[0];
+  int *p = new int[0]; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
-#endif
 
 
 //----- Custom non-placement operators
@@ -50,12 +40,8 @@ void testOpNew() {
 }
 
 void testNewExpr() {
-  int *p = new int;
+  int *p = new int; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
-#endif
-
 
 //----- Custom NoThrow placement operators
 void testOpNewNoThrow() {
@@ -63,11 +49,8 @@ void testOpNewNoThrow() {
 }
 
 void testNewExprNoThrow() {
-  int *p = new(std::nothrow) int;
+  int *p = new(std::nothrow) int; // no-warning
 }
-#if LEAKS && !ALLOCATOR_INLINING
-// expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
-#endif
 
 //----- Custom placement operators
 void testOpNewPlacement() {
diff --git a/clang/test/Analysis/NewDelete-sized-deallocation.cpp b/clang/test/Analysis/NewDelete-sized-deallocation.cpp
new file mode 100644
index 00000000000..b0f2cfb765c
--- /dev/null
+++ b/clang/test/Analysis/NewDelete-sized-deallocation.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS -fsized-deallocation
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++14 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS -fsized-deallocation
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES -fsized-deallocation
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus -verify -analyzer-output=text %s -std=c++17 -DINCLUDE_INCLUDES -DTEST_INLINABLE_ALLOCATORS -fsized-deallocation
+
+// Test all three: undeclared operator delete, operator delete forward-declared
+// in the system header, operator delete defined in system header.
+#ifdef INCLUDE_INCLUDES
+// TEST_INLINABLE_ALLOCATORS is used within this include.
+#include "Inputs/system-header-simulator-cxx.h"
+#endif
+
+void leak() {
+  int *x = new int; // expected-note{{Memory is allocated}}
+} // expected-warning{{Potential leak of memory pointed to by 'x'}}
+  // expected-note@-1{{Potential leak of memory pointed to by 'x'}}
+
+// This function was incorrectly diagnosed as leak under -fsized-deallocation
+// because the sized operator delete was mistaken for a custom delete.
+void no_leak() {
+  int *x = new int; // no-note
+  delete x;
+} // no-warning