[analyzer][NFC] Move PrettyStackTraceLocationContext into dispatchWorkItem

[balazs-benics-sonarsource]

[analyzer][NFC] Move PrettyStackTraceLocationContext into dispatchWorkItem

This change helps with ensuring that the abstract machine call stack is only dumped exactly once no matter what checker callback we have the crash in.

Note that check::EndAnalysis callbacks are resolved outside of dispatchWorkItem, but that's the only checker callback that is outside of dispatchWorkItem.

CPP-6476

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Benics (balazs-benics-sonarsource)

Changes

This change helps with ensuring that the abstract machine call stack is only dumped exactly once no matter what checker callback we have the crash in.

Note that some checker callbacks happen outside of dispatchWorkItem, thus they need special attention.
This was the case in the past and that is not changed in this patch. Maybe in the future we could improve that too.

This patch is motivated by a new downstream checker callback, that is invoked for transitioning CFG edges, thus acting on BlockEdge program points. If it makes sense, I'd be happy to contribute that too.

---

Full diff: https://github.com/llvm/llvm-project/pull/140035.diff

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/CoreEngine.cpp (+2)
• (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+3-14)
• (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (+1-4)

diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
index 2e6631f2f620c..8cc086a12ad70 100644
--- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h"
+#include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Stmt.h"
@@ -216,6 +217,7 @@ void CoreEngine::dispatchWorkItem(ExplodedNode *Pred, ProgramPoint Loc,
   llvm::TimeTraceScope tcs{timeTraceScopeName(Loc), [Loc, Pred]() {
                              return timeTraceMetadata(Pred, Loc);
                            }};
+  PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
   // Dispatch on the location type.
   switch (Loc.getKind()) {
     case ProgramPoint::BlockEdgeKind:
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index ebad83dad0c8f..1afd4b52eb354 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -968,7 +968,6 @@ void ExprEngine::processEndWorklist() {
 
 void ExprEngine::processCFGElement(const CFGElement E, ExplodedNode *Pred,
                                    unsigned StmtIdx, NodeBuilderContext *Ctx) {
-  PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
   currStmtIdx = StmtIdx;
   currBldrCtx = Ctx;
 
@@ -2541,7 +2540,6 @@ static const LocationContext *getInlinedLocationContext(ExplodedNode *Node,
 void ExprEngine::processCFGBlockEntrance(const BlockEdge &L,
                                          NodeBuilderWithSinks &nodeBuilder,
                                          ExplodedNode *Pred) {
-  PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
   // If we reach a loop which has a known bound (and meets
   // other constraints) then consider completely unrolling it.
   if(AMgr.options.ShouldUnrollLoops) {
@@ -2808,8 +2806,6 @@ void ExprEngine::processBranch(
     std::optional<unsigned> IterationsCompletedInLoop) {
   assert((!Condition || !isa<CXXBindTemporaryExpr>(Condition)) &&
          "CXXBindTemporaryExprs are handled by processBindTemporary.");
-  const LocationContext *LCtx = Pred->getLocationContext();
-  PrettyStackTraceLocationContext StackCrashInfo(LCtx);
   currBldrCtx = &BldCtx;
 
   // Check for NULL conditions; e.g. "for(;;)"
@@ -2935,13 +2931,9 @@ void ExprEngine::processBranch(
 REGISTER_TRAIT_WITH_PROGRAMSTATE(InitializedGlobalsSet,
                                  llvm::ImmutableSet<const VarDecl *>)
 
-void ExprEngine::processStaticInitializer(const DeclStmt *DS,
-                                          NodeBuilderContext &BuilderCtx,
-                                          ExplodedNode *Pred,
-                                          ExplodedNodeSet &Dst,
-                                          const CFGBlock *DstT,
-                                          const CFGBlock *DstF) {
-  PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
+void ExprEngine::processStaticInitializer(
+    const DeclStmt *DS, NodeBuilderContext &BuilderCtx, ExplodedNode *Pred,
+    ExplodedNodeSet &Dst, const CFGBlock *DstT, const CFGBlock *DstF) {
   currBldrCtx = &BuilderCtx;
 
   const auto *VD = cast<VarDecl>(DS->getSingleDecl());
@@ -3064,9 +3056,6 @@ void ExprEngine::processEndOfFunction(NodeBuilderContext& BC,
   assert(areAllObjectsFullyConstructed(Pred->getState(),
                                        Pred->getLocationContext(),
                                        Pred->getStackFrame()->getParent()));
-
-  PrettyStackTraceLocationContext CrashInfo(Pred->getLocationContext());
-
   ExplodedNodeSet Dst;
   if (Pred->getLocationContext()->inTopFrame()) {
     // Remove dead symbols.
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
index 90625a96e9059..63bdc58030187 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -44,8 +44,6 @@ STAT_COUNTER(NumReachedInlineCountMax,
 void ExprEngine::processCallEnter(NodeBuilderContext& BC, CallEnter CE,
                                   ExplodedNode *Pred) {
   // Get the entry block in the CFG of the callee.
-  const StackFrameContext *calleeCtx = CE.getCalleeContext();
-  PrettyStackTraceLocationContext CrashInfo(calleeCtx);
   const CFGBlock *Entry = CE.getEntry();
 
   // Validate the CFG.
@@ -56,7 +54,7 @@ void ExprEngine::processCallEnter(NodeBuilderContext& BC, CallEnter CE,
   const CFGBlock *Succ = *(Entry->succ_begin());
 
   // Construct an edge representing the starting location in the callee.
-  BlockEdge Loc(Entry, Succ, calleeCtx);
+  BlockEdge Loc(Entry, Succ, CE.getCalleeContext());
 
   ProgramStateRef state = Pred->getState();
 
@@ -253,7 +251,6 @@ ProgramStateRef ExprEngine::removeStateTraitsUsedForArrayEvaluation(
 /// 5. PostStmt<CallExpr>
 void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
   // Step 1 CEBNode was generated before the call.
-  PrettyStackTraceLocationContext CrashInfo(CEBNode->getLocationContext());
   const StackFrameContext *calleeCtx = CEBNode->getStackFrame();
 
   // The parent context might not be a stack frame, so make sure we

[balazs-benics-sonarsource]

/CC @pdschbrt

[balazs-benics-sonarsource]

I've updated the PR. I noticed some mistakes in the original submission.
Could you please have a look? @Xazax-hun

[NagyDonat]

LGTM. I'm happy to see that this change simplifies the logic and moves several scattered stacktrace helpers to a common location.