StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

[frederic-tingaud-sonarsource]

Summary

Currently, when encountering an overridden new operator, the StaticAnalyzer will inline it when possible, while an overridden delete operator will never be, which leads us to false positives like the following:

struct CustomOperators {
  void *operator new(size_t count) {
    return malloc(count);
  }

  void operator delete(void *addr) {
    free(addr);
  }
};

void compliant() {
  auto *a = new CustomOperators();
  delete a; // warning{{Potential leak of memory pointed to by 'a'}}
}

This patch restores the symmetry between how operator new and operator delete are handled by also inlining the content of operator delete when possible.

[martong]

Looks promising, I am close to accept this, thanks!

clang/include/clang/Analysis/ConstructionContext.h

122–125

I think we could use the variadic isa template .

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

950–958

Could you please also update the config option's documentation at AnalyzerOptions.def?

clang/test/Analysis/cxxnewexpr-callback-inline.cpp

0

It is hard to see how the two test files are different.
Ideally, the two test files cxxnewexpr-callback-inline.cpp and cxxnewexpr-callback-noinline.cpp could be merged into the very same test file with two RUN lines and with different --check-prefix.

However, I see that this is kind of unrelated to this particular change, so, that could be done in a separate, follow-up patch, if you have the time and mood for that.

[frederic-tingaud-sonarsource]

Applying recommendations

[frederic-tingaud-sonarsource]

clang/include/clang/Analysis/ConstructionContext.h

122–125

Because assert is a macro, the commas of variadic templates break it.

[martong]

Thanks for the update, LGTM!

[frederic-tingaud-sonarsource]

As I don't have merge rights, would it be possible for you to merge it?

[frederic-tingaud-sonarsource]

Fix formatting

clang/include/clang/Analysis/ConstructionContext.h

ConstructionContextItem(const CXXInheritedCtorInitExpr *CE, unsigned Index)
       : Data(CE), Kind(ArgumentKind), Index(Index) {}
 
   ConstructionContextItem(const ObjCMessageExpr *ME, unsigned Index)
       : Data(ME), Kind(ArgumentKind), Index(Index) {}
 
   // A polymorphic version of the previous calls with dynamic type check.
   ConstructionContextItem(const Expr *E, unsigned Index)
       : Data(E), Kind(ArgumentKind), Index(Index) {
     assert(isa<CallExpr>(E) || isa<CXXConstructExpr>(E) ||
-           isa<CXXInheritedCtorInitExpr>(E) || isa<ObjCMessageExpr>(E));
+           isa<CXXDeleteExpr>(E) || isa<CXXInheritedCtorInitExpr>(E) ||
+           isa<ObjCMessageExpr>(E));
   }
 
   ConstructionContextItem(const CXXCtorInitializer *Init)
       : Data(Init), Kind(InitializerKind), Index(0) {}
 
   ItemKind getKind() const { return Kind; }
 
   LLVM_DUMP_METHOD StringRef getKindAsString() const {
     return getKindAsString(getKind());

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

ANALYZER_OPTION(
     "Whether or not templated functions may be considered for inlining.", true)
 
 ANALYZER_OPTION(bool, MayInlineCXXStandardLibrary, "c++-stdlib-inlining",
                 "Whether or not C++ standard library functions may be "
                 "considered for inlining.",
                 true)
 
 ANALYZER_OPTION(bool, MayInlineCXXAllocator, "c++-allocator-inlining",
-                "Whether or not allocator call may be considered for inlining.",
+                "Whether or not allocator and deallocator calls may be "
+                "considered for inlining.",
                 true)
 
 ANALYZER_OPTION(
     bool, MayInlineCXXSharedPtrDtor, "c++-shared_ptr-inlining",
     "Whether or not the destructor of C++ 'shared_ptr' may be considered for "
     "inlining. This covers std::shared_ptr, std::tr1::shared_ptr, and "
     "boost::shared_ptr, and indeed any destructor named '~shared_ptr'.",
     false)

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

 public:
   CallEventManager(llvm::BumpPtrAllocator &alloc) : Alloc(alloc) {}
 
   /// Gets an outside caller given a callee context.
   CallEventRef<>
   getCaller(const StackFrameContext *CalleeCtx, ProgramStateRef State);
 
   /// Gets a call event for a function call, Objective-C method call,
-  /// or a 'new' call.
+  /// a 'new', or a 'delete' call.
   CallEventRef<>
   getCall(const Stmt *S, ProgramStateRef State,
           const LocationContext *LC);
 
   CallEventRef<>
   getSimpleCall(const CallExpr *E, ProgramStateRef State,
                 const LocationContext *LCtx);
 

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

 }
 
 CallEventRef<> CallEventManager::getCall(const Stmt *S, ProgramStateRef State,
                                          const LocationContext *LC) {
   if (const auto *CE = dyn_cast<CallExpr>(S)) {
     return getSimpleCall(CE, State, LC);
   } else if (const auto *NE = dyn_cast<CXXNewExpr>(S)) {
     return getCXXAllocatorCall(NE, State, LC);
+  } else if (const auto *DE = dyn_cast<CXXDeleteExpr>(S)) {
+    return getCXXDeallocatorCall(DE, State, LC);
   } else if (const auto *ME = dyn_cast<ObjCMessageExpr>(S)) {
     return getObjCMethodCall(ME, State, LC);
   } else {
     return nullptr;
   }
 }

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE,
                                     ExplodedNode *Pred, ExplodedNodeSet &Dst) {
 
   CallEventManager &CEMgr = getStateManager().getCallEventManager();
   CallEventRef<CXXDeallocatorCall> Call = CEMgr.getCXXDeallocatorCall(
       CDE, Pred->getState(), Pred->getLocationContext());
 
   ExplodedNodeSet DstPreCall;
   getCheckerManager().runCheckersForPreCall(DstPreCall, Pred, *Call, *this);
+  ExplodedNodeSet DstPostCall;
 
-  getCheckerManager().runCheckersForPostCall(Dst, DstPreCall, *Call, *this);
+  if (AMgr.getAnalyzerOptions().MayInlineCXXAllocator) {
+    StmtNodeBuilder Bldr(DstPreCall, DstPostCall, *currBldrCtx);
+    for (ExplodedNode *I : DstPreCall) {
+      defaultEvalCall(Bldr, I, *Call);
+    }
+  } else {
+    DstPostCall = DstPreCall;
+  }
+  getCheckerManager().runCheckersForPostCall(Dst, DstPostCall, *Call, *this);
 }
 
 void ExprEngine::VisitCXXCatchStmt(const CXXCatchStmt *CS, ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
   const VarDecl *VD = CS->getExceptionDecl();
   if (!VD) {
     Dst.Add(Pred);
     return;

clang/test/Analysis/cxxnewexpr-callback-inline.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=true,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s
-
-#include "Inputs/system-header-simulator-cxx.h"
-
-namespace std {
-  void *malloc(size_t);
-}
-
-void *operator new(size_t size) { return std::malloc(size); }
-
-struct S {
-  S() {}
-};
-
-void foo();
-
-void test() {
-  S *s = new S();
-  foo();
-}
-
-// CHECK:      PreCall (operator new)
-// CHECK-NEXT: PreCall (std::malloc)
-// CHECK-NEXT: PostCall (std::malloc)
-// CHECK-NEXT: PostCall (operator new)
-// CHECK-NEXT: NewAllocator
-// CHECK-NEXT: PreCall (S::S)
-// CHECK-NEXT: PostCall (S::S)
-// CHECK-NEXT: PreStmt<CXXNewExpr>
-// CHECK-NEXT: PostStmt<CXXNewExpr>
-// CHECK-NEXT: PreCall (foo)
-// CHECK-NEXT: PostCall (foo)

clang/test/Analysis/cxxnewexpr-callback-noinline.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=false,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s
-
-#include "Inputs/system-header-simulator-cxx.h"
-
-namespace std {
-  void *malloc(size_t);
-}
-
-void *operator new(size_t size) { return std::malloc(size); }
-
-struct S {
-  S() {}
-};
-
-void foo();
-
-void test() {
-  S *s = new S();
-  foo();
-}
-
-// CHECK:      PreCall (S::S)
-// CHECK-NEXT: PostCall (S::S)
-// CHECK-NEXT: PreStmt<CXXNewExpr>
-// CHECK-NEXT: PostStmt<CXXNewExpr>
-// CHECK-NEXT: PreCall (foo)
-// CHECK-NEXT: PostCall (foo)
-// CHECK-NEXT: PreCall (std::malloc)
-// CHECK-NEXT: PostCall (std::malloc)

clang/test/Analysis/cxxnewexpr-callback.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=true,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-INLINE
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config c++-allocator-inlining=false,debug.AnalysisOrder:PreStmtCXXNewExpr=true,debug.AnalysisOrder:PostStmtCXXNewExpr=true,debug.AnalysisOrder:PreCall=true,debug.AnalysisOrder:PostCall=true,debug.AnalysisOrder:NewAllocator=true %s 2>&1 | FileCheck %s  --check-prefixes=CHECK,CHECK-NO-INLINE
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+namespace std {
+void *malloc(size_t);
+void free(void *);
+} // namespace std
+
+void *operator new(size_t size) { return std::malloc(size); }
+void operator delete(void *ptr) { std::free(ptr); }
+
+struct S {
+  S() {}
+  ~S() {}
+};
+
+void foo();
+
+void test() {
+  S *s = new S();
+  foo();
+  delete s;
+}
+
+/*
+void test() {
+  S *s = new S();
+// CHECK-INLINE:      PreCall (operator new)
+// CHECK-INLINE-NEXT: PreCall (std::malloc)
+// CHECK-INLINE-NEXT: PostCall (std::malloc)
+// CHECK-INLINE-NEXT: PostCall (operator new)
+// CHECK-INLINE-NEXT: NewAllocator
+// CHECK-NO-INLINE: PreCall (S::S)
+// CHECK-INLINE-NEXT: PreCall (S::S)
+// CHECK-NEXT: PostCall (S::S)
+// CHECK-NEXT: PreStmt<CXXNewExpr>
+// CHECK-NEXT: PostStmt<CXXNewExpr>
+  foo();
+// CHECK-NEXT: PreCall (foo)
+// CHECK-NEXT: PostCall (foo)
+  delete s;
+// CHECK-NEXT: PreCall (S::~S)
+// CHECK-NEXT: PostCall (S::~S)
+// CHECK-NEXT: PreCall (operator delete)
+// CHECK-INLINE-NEXT: PreCall (std::free)
+// CHECK-INLINE-NEXT: PostCall (std::free)
+// CHECK-NEXT: PostCall (operator delete)
+}
+
+void operator delete(void *ptr) {
+  std::free(ptr);
+// CHECK-NO-INLINE-NEXT: PreCall (std::free)
+// CHECK-NO-INLINE-NEXT: PostCall (std::free)
+}
+
+void *operator new(size_t size) {
+  return std::malloc(size);
+// CHECK-NO-INLINE-NEXT: PreCall (std::malloc)
+// CHECK-NO-INLINE-NEXT: PostCall (std::malloc)
+}
+*/

clang/test/Analysis/dtor.cpp

void testC() {
   c->~C(); // no-crash
 }
 
 void testAutoDtor() {
   const A &a = *(A *)-1;
   // no-crash
 }
 } // namespace dtor_over_loc_concrete_int
+
+// Test overriden new/delete operators
+struct CustomOperators {
+  void *operator new(size_t count) {
+    return malloc(count);
+  }
+
+  void operator delete(void *addr) {
+    free(addr);
+  }
+
+private:
+  int i;
+};
+
+void compliant() {
+  auto *a = new CustomOperators();
+  delete a;
+}
+
+void overrideLeak() {
+  auto *a = new CustomOperators();
+} // expected-warning{{Potential leak of memory pointed to by 'a'}}
+
+void overrideDoubleDelete() {
+  auto *a = new CustomOperators();
+  delete a;
+  delete a; // expected-warning@581 {{Attempt to free released memory}}
+}