[analyzer] Fix tagging of PostAllocatorCall

[NagyDonat]

By design the Location data member of a CheckerContext is always a ProgramPoint which is tagged with the currently active checker (note that all checker classes are subclasses of ProgramPointTag). This ensures that exploded nodes created by the checker are by default tagged by the checker object unless the checker specifies some other tag (e.g. a note tag) when it calls the addTransition-like method that creates the node.

This was followed by all the CheckerManager::runCheckersForXXX methods, except for runCheckerForNewAllocator, where the implementation constructed the PostAllocatorCall program point which was used to create the CheckerContext without passing checkFn.Checker as the tag of the program point.

This commit elimintates this inconsistency and adds an assertion to the constructor of CheckerContext to ensure that this invariant will be upheld even if we e.g. add a new program point kind.

I strongly suspect that this is a non-functional change because program point tags are a vestigial feature in the codebase that barely affect anything -- but e.g. their presence affects the infamous node reclamation process, so I'm not marking this as NFC.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Donát Nagy (NagyDonat)

Changes

By design the Location data member of a CheckerContext is always a ProgramPoint which is tagged with the currently active checker (note that all checker classes are subclasses of ProgramPointTag). This ensures that exploded nodes created by the checker are by default tagged by the checker object unless the checker specifies some other tag (e.g. a note tag) to the addTransition-like method that creates the node.

This was followed by all the CheckerManager::runCheckersForXXX methods, except for runCheckerForNewAllocator, where the implementation constructed the PostAllocatorCall program point without passing checkFn.Checker as the tag of the program point.

This commit elimintates this inconsistency and adds an assertion to the constructor of CheckerContext to ensure that this invariant will be upheld even if we e.g. add a new program point kind.

I strongly suspect that this is a non-functional change because program point tags are a vestigial feature in the codebase that barely affect anything -- but e.g. their presence affects the infamous node reclamation process, so I'm not marking this as NFC.

---

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

2 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h (+2)
• (modified) clang/lib/StaticAnalyzer/Core/CheckerManager.cpp (+2-2)

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
index 63ca3efc6d228..aad71299ccdc1 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -51,6 +51,8 @@ class CheckerContext {
       wasInlined(wasInlined) {
     assert(Pred->getState() &&
            "We should not call the checkers on an empty state.");
+    assert(loc.getTag() && "The ProgramPoint associated with CheckerContext "
+                           "must be tagged with the active checker.");
   }
 
   AnalysisManager &getAnalysisManager() {
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index d2b7b2bfbb019..0fe677e4ee435 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -585,8 +585,8 @@ namespace {
                     NodeBuilder &Bldr, ExplodedNode *Pred) {
       llvm::TimeTraceScope TimeScope(
           checkerScopeName("Allocator", checkFn.Checker));
-      ProgramPoint L =
-          PostAllocatorCall(Call.getOriginExpr(), Pred->getLocationContext());
+      ProgramPoint L = PostAllocatorCall(
+          Call.getOriginExpr(), Pred->getLocationContext(), checkFn.Checker);
       CheckerContext C(Bldr, Eng, Pred, L, WasInlined);
       checkFn(cast<CXXAllocatorCall>(*Call.cloneWithState(Pred->getState())),
               C);

[steakhal]

LGTM, thanks.

[NagyDonat]

I tested this commit on our usual collection of 10+ open source projects and as expected, there were no new crashes or result changes.