Skip to content

Commit 3aa9397

Browse files
NagyDonattomtor
authored andcommitted
[analyzer][NFCI] Remove ad-hoc program point tagging (llvm#142980)
Previously some checkers attached explicitly created program point tags to some of the exploded graph nodes that they created. In most of the checkers this ad-hoc tagging only affected the debug dump of the exploded graph (and they weren't too relevant for debugging) so this commit removes them. There were two checkers where the tagging _did_ have a functional role: - In `RetainCountChecker` the presence of tags were checked by `RefCountReportVisitor`. - In `DynamicTypePropagation` the checker sometimes wanted to create two identical nodes and had to apply an explicit tag on the second one to avoid "caching out". In these two situations I preserved the tags but switched to using `SimpleProgramPointTag` instead of `CheckerProgramPointTag` because `CheckerProgramPointTag` didn't provide enough benefits to justify its existence. Note that this commit depends on the earlier commit "[analyzer] Fix tagging of PostAllocatorCall" ec96c0c and would introduce crashes when cherry-picked onto a branch that doesn't contain that commit. For more details about the background see the discourse thread https://discourse.llvm.org/t/role-of-programpointtag-in-the-static-analyzer/ As a tangentially related changes, this commit also adds some comments to document the surprising behavior of `CheckerContext::addTransition` and an assertion in the constructor of `PathSensitiveBugReport` to get a more readable crash dump in the case when the report is constructed with `nullptr` as the `ErrorNode`. (This can happen due to "caching out".)
1 parent c1b62f1 commit 3aa9397

File tree

14 files changed

+56
-89
lines changed

14 files changed

+56
-89
lines changed

clang/include/clang/StaticAnalyzer/Core/Checker.h

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -608,20 +608,6 @@ class EventDispatcher {
608608
}
609609
};
610610

611-
/// Tag that can use a checker name as a message provider
612-
/// (see SimpleProgramPointTag).
613-
/// FIXME: This is a cargo cult class which is copied into several checkers but
614-
/// does not provide anything useful.
615-
/// The only added functionality provided by this class (compared to
616-
/// SimpleProgramPointTag) is that it composes the tag description string from
617-
/// two arguments -- but tag descriptions only appear in debug output so there
618-
/// is no reason to bother with this.
619-
class CheckerProgramPointTag : public SimpleProgramPointTag {
620-
public:
621-
CheckerProgramPointTag(StringRef CheckerName, StringRef Msg);
622-
CheckerProgramPointTag(const CheckerBase *Checker, StringRef Msg);
623-
};
624-
625611
/// We dereferenced a location that may be null.
626612
struct ImplicitNullDerefEvent {
627613
SVal Location;

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,9 @@ class CheckerContext {
171171
/// tag is specified, a default tag, unique to the given checker,
172172
/// will be used. Tags are used to prevent states generated at
173173
/// different sites from caching out.
174+
/// NOTE: If the State is unchanged and the Tag is nullptr, this may return a
175+
/// node which is not tagged (instead of using the default tag corresponding
176+
/// to the active checker). This is arguably a bug and should be fixed.
174177
ExplodedNode *addTransition(ProgramStateRef State = nullptr,
175178
const ProgramPointTag *Tag = nullptr) {
176179
return addTransitionImpl(State ? State : getState(), false, nullptr, Tag);
@@ -183,6 +186,9 @@ class CheckerContext {
183186
/// @param Pred The transition will be generated from the specified Pred node
184187
/// to the newly generated node.
185188
/// @param Tag The tag to uniquely identify the creation site.
189+
/// NOTE: If the State is unchanged and the Tag is nullptr, this may return a
190+
/// node which is not tagged (instead of using the default tag corresponding
191+
/// to the active checker). This is arguably a bug and should be fixed.
186192
ExplodedNode *addTransition(ProgramStateRef State, ExplodedNode *Pred,
187193
const ProgramPointTag *Tag = nullptr) {
188194
return addTransitionImpl(State, false, Pred, Tag);

clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -671,7 +671,6 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
671671
ProgramStateRef state,
672672
const ObjCMethodCall &Msg) const {
673673
ASTContext &Ctx = C.getASTContext();
674-
static CheckerProgramPointTag Tag(this, "NilReceiver");
675674

676675
// Check the return type of the message expression. A message to nil will
677676
// return different values depending on the return type and the architecture.
@@ -682,7 +681,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
682681
if (CanRetTy->isStructureOrClassType()) {
683682
// Structure returns are safe since the compiler zeroes them out.
684683
SVal V = C.getSValBuilder().makeZeroVal(RetTy);
685-
C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V), &Tag);
684+
C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V));
686685
return;
687686
}
688687

@@ -701,7 +700,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
701700
Ctx.LongDoubleTy == CanRetTy ||
702701
Ctx.LongLongTy == CanRetTy ||
703702
Ctx.UnsignedLongLongTy == CanRetTy)))) {
704-
if (ExplodedNode *N = C.generateErrorNode(state, &Tag))
703+
if (ExplodedNode *N = C.generateErrorNode(state))
705704
emitNilReceiverBug(C, Msg, N);
706705
return;
707706
}
@@ -720,7 +719,7 @@ void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C,
720719
// of this case unless we have *a lot* more knowledge.
721720
//
722721
SVal V = C.getSValBuilder().makeZeroVal(RetTy);
723-
C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V), &Tag);
722+
C.addTransition(state->BindExpr(Msg.getOriginExpr(), LCtx, V));
724723
return;
725724
}
726725

clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -674,9 +674,16 @@ void DynamicTypePropagation::checkPostStmt(const CastExpr *CE,
674674
if (TrackedType &&
675675
!ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, *TrackedType) &&
676676
!ASTCtxt.canAssignObjCInterfaces(*TrackedType, DestObjectPtrType)) {
677-
static CheckerProgramPointTag IllegalConv(this, "IllegalConversion");
678-
ExplodedNode *N = C.addTransition(State, AfterTypeProp, &IllegalConv);
679-
reportGenericsBug(*TrackedType, DestObjectPtrType, N, Sym, C);
677+
// This distinct program point tag is needed because `State` can be
678+
// identical to the state of the node `AfterTypeProp`, and in that case
679+
// `generateNonFatalErrorNode` would "cache out" and return nullptr
680+
// (instead of re-creating an already existing node).
681+
static SimpleProgramPointTag IllegalConv("DynamicTypePropagation",
682+
"IllegalConversion");
683+
ExplodedNode *N =
684+
C.generateNonFatalErrorNode(State, AfterTypeProp, &IllegalConv);
685+
if (N)
686+
reportGenericsBug(*TrackedType, DestObjectPtrType, N, Sym, C);
680687
return;
681688
}
682689

@@ -885,8 +892,7 @@ void DynamicTypePropagation::checkPreObjCMessage(const ObjCMethodCall &M,
885892
// Warn when argument is incompatible with the parameter.
886893
if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType,
887894
ArgObjectPtrType)) {
888-
static CheckerProgramPointTag Tag(this, "ArgTypeMismatch");
889-
ExplodedNode *N = C.addTransition(State, &Tag);
895+
ExplodedNode *N = C.generateNonFatalErrorNode(State);
890896
reportGenericsBug(ArgObjectPtrType, ParamObjectPtrType, N, Sym, C, Arg);
891897
return;
892898
}

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1043,8 +1043,7 @@ bool GenericTaintChecker::generateReportIfTainted(const Expr *E, StringRef Msg,
10431043

10441044
// Generate diagnostic.
10451045
assert(BT);
1046-
static CheckerProgramPointTag Tag(BT->getCheckerName(), Msg);
1047-
if (ExplodedNode *N = C.generateNonFatalErrorNode(C.getState(), &Tag)) {
1046+
if (ExplodedNode *N = C.generateNonFatalErrorNode(C.getState())) {
10481047
auto report = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
10491048
report->addRange(E->getSourceRange());
10501049
for (auto TaintedSym : getTaintedSymbols(C.getState(), *TaintedSVal)) {

clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -747,9 +747,7 @@ void NonLocalizedStringChecker::reportLocalizationError(
747747
if (isDebuggingContext(C))
748748
return;
749749

750-
static CheckerProgramPointTag Tag("NonLocalizedStringChecker",
751-
"UnlocalizedString");
752-
ExplodedNode *ErrNode = C.addTransition(C.getState(), C.getPredecessor(), &Tag);
750+
ExplodedNode *ErrNode = C.generateNonFatalErrorNode();
753751

754752
if (!ErrNode)
755753
return;

clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,6 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent,
7373
return;
7474

7575
ProgramStateRef State = Ctx.getState();
76-
static CheckerProgramPointTag Tag("MPI-Checker", "UnmatchedWait");
7776
ExplodedNode *ErrorNode{nullptr};
7877

7978
// Check all request regions used by the wait function.
@@ -82,7 +81,7 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent,
8281
State = State->set<RequestMap>(ReqRegion, Request::State::Wait);
8382
if (!Req) {
8483
if (!ErrorNode) {
85-
ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
84+
ErrorNode = Ctx.generateNonFatalErrorNode(State);
8685
State = ErrorNode->getState();
8786
}
8887
// A wait has no matching nonblocking call.
@@ -105,7 +104,6 @@ void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper,
105104
if (Requests.isEmpty())
106105
return;
107106

108-
static CheckerProgramPointTag Tag("MPI-Checker", "MissingWait");
109107
ExplodedNode *ErrorNode{nullptr};
110108

111109
auto ReqMap = State->get<RequestMap>();
@@ -114,7 +112,7 @@ void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper,
114112
if (Req.second.CurrentState == Request::State::Nonblocking) {
115113

116114
if (!ErrorNode) {
117-
ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
115+
ErrorNode = Ctx.generateNonFatalErrorNode(State);
118116
State = ErrorNode->getState();
119117
}
120118
BReporter.reportMissingWait(Req.second, Req.first, ErrorNode,

clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -556,8 +556,7 @@ void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR,
556556
return;
557557
}
558558

559-
static CheckerProgramPointTag Tag(this, "DeadSymbolsLeak");
560-
ExplodedNode *N = C.generateNonFatalErrorNode(C.getState(), &Tag);
559+
ExplodedNode *N = C.generateNonFatalErrorNode(C.getState());
561560
if (!N)
562561
return;
563562

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3098,8 +3098,7 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
30983098
// Generate leak node.
30993099
ExplodedNode *N = C.getPredecessor();
31003100
if (!Errors.empty()) {
3101-
static CheckerProgramPointTag Tag("MallocChecker", "DeadSymbolsLeak");
3102-
N = C.generateNonFatalErrorNode(C.getState(), &Tag);
3101+
N = C.generateNonFatalErrorNode(C.getState());
31033102
if (N) {
31043103
for (SymbolRef Sym : Errors) {
31053104
HandleLeak(Sym, N, C);

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ const char *getNullabilityString(Nullability Nullab) {
6969
}
7070

7171
// These enums are used as an index to ErrorMessages array.
72+
// FIXME: ErrorMessages no longer exists, perhaps remove this as well?
7273
enum class ErrorKind : int {
7374
NilAssignedToNonnull,
7475
NilPassedToNonnull,
@@ -714,8 +715,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
714715
if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull &&
715716
RetExprTypeLevelNullability != Nullability::Nonnull &&
716717
!InSuppressedMethodFamily) {
717-
static CheckerProgramPointTag Tag(this, "NullReturnedFromNonnull");
718-
ExplodedNode *N = C.generateErrorNode(State, &Tag);
718+
ExplodedNode *N = C.generateErrorNode(State);
719719
if (!N)
720720
return;
721721

@@ -750,8 +750,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
750750
Nullness != NullConstraint::IsNotNull &&
751751
TrackedNullabValue == Nullability::Nullable &&
752752
RequiredNullability == Nullability::Nonnull) {
753-
static CheckerProgramPointTag Tag(this, "NullableReturnedFromNonnull");
754-
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
753+
ExplodedNode *N = C.addTransition(State, C.getPredecessor());
755754

756755
SmallString<256> SBuf;
757756
llvm::raw_svector_ostream OS(SBuf);
@@ -1299,8 +1298,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
12991298
ValNullability != Nullability::Nonnull &&
13001299
ValueExprTypeLevelNullability != Nullability::Nonnull &&
13011300
!isARCNilInitializedLocal(C, S)) {
1302-
static CheckerProgramPointTag Tag(this, "NullPassedToNonnull");
1303-
ExplodedNode *N = C.generateErrorNode(State, &Tag);
1301+
ExplodedNode *N = C.generateErrorNode(State);
13041302
if (!N)
13051303
return;
13061304

@@ -1342,8 +1340,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
13421340
return;
13431341
if (ChecksEnabled[CK_NullablePassedToNonnull] &&
13441342
LocNullability == Nullability::Nonnull) {
1345-
static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
1346-
ExplodedNode *N = C.addTransition(State, C.getPredecessor(), &Tag);
1343+
ExplodedNode *N = C.addTransition(State, C.getPredecessor());
13471344
reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer "
13481345
"which is expected to have non-null value",
13491346
ErrorKind::NullableAssignedToNonnull,

0 commit comments

Comments
 (0)