Skip to content

Commit 8690321

Browse files
committed
[analyzer][NFCI] Remove pointless program point tagging
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 d7c7c46 commit 8690321

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)