Inline allocator call when c++-allocator-inlining is specified as true

[karthikthecool]

Summary

Hi Jordan,
As suggessted by you i have broken down the patch discussed at http://llvm-reviews.chandlerc.com/D2616.
As our first step in this patch we inline cxx allocator call corresponding to a call to new.
By default this feature is disabled as we still need to model deallocator call.
Analyzer config flag c++-allocator-inlining enables inlining of allocator call when specified as true.
Please let me know if this patch looks good to commit. Thanks alot for your input and time.
Regards
Karthik Bhat

[jordan_rose]

This looks good to me, except that I wouldn't bother turning on allocators in NewDelete-custom.cpp until we're using the new-region for construction. (Otherwise we'd get a leak whether or not there actually is one, wouldn't we?) Do you have any opinion on that, though?

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

340–342

I would move this up before the CallEvent creation, just in case that crashes. We can use the CXXNewExpr's location instead.

[karthikthecool]

Hi Jordan,
Yah it seems reasonable to turn off allocator in NewDelete-custom.cpp as it is not implemented completly yet.
Removed changes done in NewDelete-custom.cpp and implemented review comments. Also i'm calling VisitCXXNewAllocatorCall only in case we inline allocators. Once we have completly modelled allocator we can call VisitCXXNewAllocatorCall for all cases.
Please let me know if this looks good.
Thanks for the inputs and time.
Regards
Karthik Bhat

[jordan_rose]

Uh, we should always be evaluating the call even if we can't inline it. In fact, if we are evaluating it, we can probably drop the part of VisitCXXNewExpr that calls invalidateRegions for the allocator, even when inlining is off.

[karthikthecool]

Hi Jordan,
As discussed over the mail last week we are not evaluating allocators in all cases just yet as we're not handling the return value correctly in few cases when alpha.cplusplus.NewDeleteLeaks is turned on resulting in false alarms.
Added a TODO and a comment in code as suggested. Please let me know if it is ok for commit?
Thanks for suggessions and inputs.
Regards
Karthik Bhat

[jordan_rose]

Looks good to me! Will commit soon.

lib/StaticAnalyzer/Core/ExprEngine.cpp

559–561

I would rephrase this as "we're not handling the return value correctly, which causes false positives when the alpha.cplusplus.NewDeleteLeaks check is on". The return value isn't ever handled correctly; that's just the easiest way to get it to turn into a bug.

[jordan_rose]

Committed in r201122, with the comment change.

[karthikthecool]

Thanks Jordan.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

   /// \sa mayInlineTemplateFunctions
   Optional<bool> InlineTemplateFunctions;
 
+  /// \sa mayInlineCXXAllocator
+  Optional<bool> InlineCXXAllocator;
+
   /// \sa mayInlineCXXContainerCtorsAndDtors
   Optional<bool> InlineCXXContainerCtorsAndDtors;
 

   /// accepts the values "true" and "false".
   bool mayInlineTemplateFunctions();
 
+  /// Returns whether or not allocator call may be considered for inlining.
+  ///
+  /// This is controlled by the 'c++-allocator-inlining' config option, which
+  /// accepts the values "true" and "false".
+  bool mayInlineCXXAllocator();
+
   /// Returns whether or not constructors and destructors of C++ container
   /// objects may be considered for inlining.
   ///

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

                           const Stmt *S, bool IsBaseDtor,
                           ExplodedNode *Pred, ExplodedNodeSet &Dst);
 
+  void VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
+                                ExplodedNode *Pred,
+                                ExplodedNodeSet &Dst);
+
   void VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                        ExplodedNodeSet &Dst);
 

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp

                           /*Default=*/true);
 }
 
+bool AnalyzerOptions::mayInlineCXXAllocator() {
+  return getBooleanOption(InlineCXXAllocator,
+                          "c++-allocator-inlining",
+                          /*Default=*/false);
+}
+
 bool AnalyzerOptions::mayInlineCXXContainerCtorsAndDtors() {
   return getBooleanOption(InlineCXXContainerCtorsAndDtors,
                           "c++-container-inlining",

lib/StaticAnalyzer/Core/CoreEngine.cpp

     return;
   }
 
+  if ((*Block)[Idx].getKind() == CFGElement::NewAllocator) {
+    WList->enqueue(N, Block, Idx+1);
+    return;
+  }
+
   // At this point, we know we're processing a normal statement.
   CFGStmt CS = (*Block)[Idx].castAs<CFGStmt>();
   PostStmt Loc(CS.getStmt(), N->getLocationContext());

lib/StaticAnalyzer/Core/ExprEngine.cpp

 
 void ExprEngine::ProcessNewAllocator(const CXXNewExpr *NE,
                                      ExplodedNode *Pred) {
-  //TODO: Implement VisitCXXNewAllocatorCall
   ExplodedNodeSet Dst;
-  NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
-  const LocationContext *LCtx = Pred->getLocationContext();
-  PostImplicitCall PP(NE->getOperatorNew(), NE->getLocStart(), LCtx);
-  Bldr.generateNode(PP, Pred->getState(), Pred);
+  AnalysisManager &AMgr = getAnalysisManager();
+  AnalyzerOptions &Opts = AMgr.options;
+  // TODO: We're not evaluating allocators for all cases just yet as
+  // we're not handling the return value correctly when
+  // alpha.cplusplus.NewDeleteLeaks is check on.
+  if (Opts.mayInlineCXXAllocator())
+    VisitCXXNewAllocatorCall(NE, Pred, Dst);
+  else {
+    NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+    const LocationContext *LCtx = Pred->getLocationContext();
+    PostImplicitCall PP(NE->getOperatorNew(), NE->getLocStart(), LCtx);
+    Bldr.generateNode(PP, Pred->getState(), Pred);
+  }
   Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
 }
 

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

                                              *Call, *this);
 }
 
+void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
+                                          ExplodedNode *Pred,
+                                          ExplodedNodeSet &Dst) {
+  ProgramStateRef State = Pred->getState();
+  const LocationContext *LCtx = Pred->getLocationContext();
+  PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(),
+                                CNE->getStartLoc(),
+                                "Error evaluating New Allocator Call");
+  CallEventManager &CEMgr = getStateManager().getCallEventManager();
+  CallEventRef<CXXAllocatorCall> Call =
+    CEMgr.getCXXAllocatorCall(CNE, State, LCtx);
+
+  ExplodedNodeSet DstPreCall;
+  getCheckerManager().runCheckersForPreCall(DstPreCall, Pred,
+                                            *Call, *this);
+
+  ExplodedNodeSet DstInvalidated;
+  StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx);
+  for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
+       I != E; ++I)
+    defaultEvalCall(Bldr, *I, *Call);
+  getCheckerManager().runCheckersForPostCall(Dst, DstInvalidated,
+                                             *Call, *this);
+}
+
+
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
   // FIXME: Much of this should eventually migrate to CXXAllocatorCall.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

     break;
   }
   case CE_CXXAllocator:
+    if (Opts.mayInlineCXXAllocator())
+      break;
     // Do not inline allocators until we model deallocators.
     // This is unfortunate, but basically necessary for smart pointers and such.
     return CIP_DisallowedAlways;

test/Analysis/inline.cpp

-// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config ipa=inlining -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config ipa=inlining -analyzer-config c++-allocator-inlining=true -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_checkInlined(bool);

 // This is the standard placement new.
 inline void* operator new(size_t, void* __p) throw()
 {
+  clang_analyzer_checkInlined(true);// expected-warning{{TRUE}}
   return __p;
 }