[PATCH] D70150: [analyzer] Don't clean up dead symbols from constraints twice.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet.
Herald added a project: clang.
NoQ marked an inline comment as done.
NoQ added inline comments.
NoQ marked an inline comment as done.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:750-752
     // The constraint manager has not been cleaned up yet, so clean up now.
     CheckerState =
         getConstraintManager().removeDeadBindings(CheckerState, SymReaper);
----------------
Note: the second invocation is here.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:763-764
     // generate a transition to that state.
     ProgramStateRef CleanedCheckerSt =
         StateMgr.getPersistentStateWithGDM(CleanedState, CheckerState);
     Bldr.generateNode(DiagnosticStmt, I, CleanedCheckerSt, &cleanupTag, K);
----------------
Note: The results of the first invocation are discarded here, as the updated state is getting frankensteined by attaching `ExprEngine`'s environment and store to checker's GDM, while range constraints also reside in the GDM.


A solid 2% analysis speed improvement! Not really - such amounts are barely measurable.

But, i mean, everybody knows that dead symbol elimination is the biggest performance bottleneck, so many people tried to improve it, how come nobody noticed this bug before?


Repository:
  rC Clang

https://reviews.llvm.org/D70150

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ProgramState.cpp


Index: clang/lib/StaticAnalyzer/Core/ProgramState.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ProgramState.cpp
+++ clang/lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -91,10 +91,9 @@
     I->second.second(I->second.first);
 }
 
-ProgramStateRef
-ProgramStateManager::removeDeadBindings(ProgramStateRef state,
-                                   const StackFrameContext *LCtx,
-                                   SymbolReaper& SymReaper) {
+ProgramStateRef ProgramStateManager::removeDeadBindingsFromEnvironmentAndStore(
+    ProgramStateRef state, const StackFrameContext *LCtx,
+    SymbolReaper &SymReaper) {
 
   // This code essentially performs a "mark-and-sweep" of the VariableBindings.
   // The roots are any Block-level exprs and Decls that our liveness algorithm
@@ -112,8 +111,7 @@
   NewState.setStore(newStore);
   SymReaper.setReapedStore(newStore);
 
-  ProgramStateRef Result = getPersistentState(NewState);
-  return ConstraintMgr->removeDeadBindings(Result, SymReaper);
+  return getPersistentState(NewState);
 }
 
 ProgramStateRef ProgramState::bindLoc(Loc LV,
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -728,7 +728,8 @@
   // Create a state in which dead bindings are removed from the environment
   // and the store. TODO: The function should just return new env and store,
   // not a new state.
-  CleanedState = StateMgr.removeDeadBindings(CleanedState, SFC, SymReaper);
+  CleanedState = StateMgr.removeDeadBindingsFromEnvironmentAndStore(
+      CleanedState, SFC, SymReaper);
 
   // Process any special transfer function for dead symbols.
   // A tag to track convenience transitions, which can be removed at cleanup.
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -533,9 +533,10 @@
   ConstraintManager &getConstraintManager() { return *ConstraintMgr; }
   SubEngine &getOwningEngine() { return *Eng; }
 
-  ProgramStateRef removeDeadBindings(ProgramStateRef St,
-                                    const StackFrameContext *LCtx,
-                                    SymbolReaper& SymReaper);
+  ProgramStateRef
+  removeDeadBindingsFromEnvironmentAndStore(ProgramStateRef St,
+                                            const StackFrameContext *LCtx,
+                                            SymbolReaper &SymReaper);
 
 public:
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70150.228980.patch
Type: text/x-patch
Size: 2752 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191113/e8df01c0/attachment.bin>