https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/143735
From 2f0abfe5e306072fc56ca93a570c58f2b9a2c967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.n...@ericsson.com> Date: Wed, 11 Jun 2025 17:31:43 +0200 Subject: [PATCH 1/2] [analyzer] Conversion to CheckerFamily: NullabilityChecker This commit converts NullabilityChecker to the new checker family framework that was introduced in the recent commit 6833076a5d9f5719539a24e900037da5a3979289 This commit removes the dummy checker `nullability.NullabilityBase` because it was hidden from the users and didn't have any useful role except for helping the registration of the checker parts in the old ad-hoc system (which is replaced by the new standardized framework). Except for the removal of this dummy checker, no functional changes intended. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 56 +++--- .../Checkers/NullabilityChecker.cpp | 177 +++++++++--------- .../test/Analysis/analyzer-enabled-checkers.c | 1 - clang/test/Analysis/bugfix-124477.m | 2 +- ...c-library-functions-arg-enabled-checkers.c | 1 - 5 files changed, 112 insertions(+), 125 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 2a96df80d1001..d6a1b9bbc1fdd 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -326,39 +326,37 @@ def StdVariantChecker : Checker<"StdVariant">, let ParentPackage = Nullability in { -def NullabilityBase : Checker<"NullabilityBase">, - HelpText<"Stores information during the analysis about nullability.">, - Documentation<NotDocumented>, - Hidden; - -def NullPassedToNonnullChecker : Checker<"NullPassedToNonnull">, - HelpText<"Warns when a null pointer is passed to a pointer which has a " - "_Nonnull type.">, - Dependencies<[NullabilityBase]>, - Documentation<HasDocumentation>; + def NullPassedToNonnullChecker + : Checker<"NullPassedToNonnull">, + HelpText<"Warns when a null pointer is passed to a pointer which has a " + "_Nonnull type.">, + Documentation<HasDocumentation>; -def NullReturnedFromNonnullChecker : Checker<"NullReturnedFromNonnull">, - HelpText<"Warns when a null pointer is returned from a function that has " - "_Nonnull return type.">, - Dependencies<[NullabilityBase]>, - Documentation<HasDocumentation>; + def NullReturnedFromNonnullChecker + : Checker<"NullReturnedFromNonnull">, + HelpText< + "Warns when a null pointer is returned from a function that has " + "_Nonnull return type.">, + Documentation<HasDocumentation>; -def NullableDereferencedChecker : Checker<"NullableDereferenced">, - HelpText<"Warns when a nullable pointer is dereferenced.">, - Dependencies<[NullabilityBase]>, - Documentation<HasDocumentation>; + def NullableDereferencedChecker + : Checker<"NullableDereferenced">, + HelpText<"Warns when a nullable pointer is dereferenced.">, + Documentation<HasDocumentation>; -def NullablePassedToNonnullChecker : Checker<"NullablePassedToNonnull">, - HelpText<"Warns when a nullable pointer is passed to a pointer which has a " - "_Nonnull type.">, - Dependencies<[NullabilityBase]>, - Documentation<HasDocumentation>; + def NullablePassedToNonnullChecker + : Checker<"NullablePassedToNonnull">, + HelpText< + "Warns when a nullable pointer is passed to a pointer which has a " + "_Nonnull type.">, + Documentation<HasDocumentation>; -def NullableReturnedFromNonnullChecker : Checker<"NullableReturnedFromNonnull">, - HelpText<"Warns when a nullable pointer is returned from a function that has " - "_Nonnull return type.">, - Dependencies<[NullabilityBase]>, - Documentation<NotDocumented>; + def NullableReturnedFromNonnullChecker + : Checker<"NullableReturnedFromNonnull">, + HelpText<"Warns when a nullable pointer is returned from a function " + "that has " + "_Nonnull return type.">, + Documentation<NotDocumented>; } // end "nullability" diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index 461d01b452fd0..9744d1abf7790 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -81,11 +81,12 @@ enum class ErrorKind : int { }; class NullabilityChecker - : public Checker<check::Bind, check::PreCall, check::PreStmt<ReturnStmt>, - check::PostCall, check::PostStmt<ExplicitCastExpr>, - check::PostObjCMessage, check::DeadSymbols, eval::Assume, - check::Location, check::Event<ImplicitNullDerefEvent>, - check::BeginFunction> { + : public CheckerFamily< + check::Bind, check::PreCall, check::PreStmt<ReturnStmt>, + check::PostCall, check::PostStmt<ExplicitCastExpr>, + check::PostObjCMessage, check::DeadSymbols, eval::Assume, + check::Location, check::Event<ImplicitNullDerefEvent>, + check::BeginFunction> { public: // If true, the checker will not diagnose nullabilility issues for calls @@ -113,25 +114,21 @@ class NullabilityChecker void printState(raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep) const override; - enum CheckKind { - CK_NullPassedToNonnull, - CK_NullReturnedFromNonnull, - CK_NullableDereferenced, - CK_NullablePassedToNonnull, - CK_NullableReturnedFromNonnull, - CK_NumCheckKinds - }; - - bool ChecksEnabled[CK_NumCheckKinds] = {false}; - CheckerNameRef CheckNames[CK_NumCheckKinds]; - mutable std::unique_ptr<BugType> BTs[CK_NumCheckKinds]; - - const std::unique_ptr<BugType> &getBugType(CheckKind Kind) const { - if (!BTs[Kind]) - BTs[Kind].reset(new BugType(CheckNames[Kind], "Nullability", - categories::MemoryError)); - return BTs[Kind]; - } + StringRef getDebugTag() const override { return "NullabilityChecker"; } + + // FIXME: All bug types share the same Description ("Nullability") since the + // creation of this checker. We should write more descriptive descriptions... + // or just eliminate the Description field if it is meaningless? + CheckerFrontendWithBugType NullPassedToNonnull{"Nullability", + categories::MemoryError}; + CheckerFrontendWithBugType NullReturnedFromNonnull{"Nullability", + categories::MemoryError}; + CheckerFrontendWithBugType NullableDereferenced{"Nullability", + categories::MemoryError}; + CheckerFrontendWithBugType NullablePassedToNonnull{"Nullability", + categories::MemoryError}; + CheckerFrontendWithBugType NullableReturnedFromNonnull{ + "Nullability", categories::MemoryError}; // When set to false no nullability information will be tracked in // NullabilityMap. It is possible to catch errors like passing a null pointer @@ -164,17 +161,16 @@ class NullabilityChecker /// /// When \p SuppressPath is set to true, no more bugs will be reported on this /// path by this checker. - void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, CheckKind CK, - ExplodedNode *N, const MemRegion *Region, - CheckerContext &C, + void reportBugIfInvariantHolds(StringRef Msg, ErrorKind Error, + const BugType &BT, ExplodedNode *N, + const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr = nullptr, bool SuppressPath = false) const; - void reportBug(StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N, - const MemRegion *Region, BugReporter &BR, + void reportBug(StringRef Msg, ErrorKind Error, const BugType &BT, + ExplodedNode *N, const MemRegion *Region, BugReporter &BR, const Stmt *ValueExpr = nullptr) const { - const std::unique_ptr<BugType> &BT = getBugType(CK); - auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N); + auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N); if (Region) { R->markInteresting(Region); R->addVisitor<NullabilityBugVisitor>(Region); @@ -480,7 +476,7 @@ static bool checkInvariantViolation(ProgramStateRef State, ExplodedNode *N, } void NullabilityChecker::reportBugIfInvariantHolds( - StringRef Msg, ErrorKind Error, CheckKind CK, ExplodedNode *N, + StringRef Msg, ErrorKind Error, const BugType &BT, ExplodedNode *N, const MemRegion *Region, CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const { ProgramStateRef OriginalState = N->getState(); @@ -492,7 +488,7 @@ void NullabilityChecker::reportBugIfInvariantHolds( N = C.addTransition(OriginalState, N); } - reportBug(Msg, Error, CK, N, Region, C.getBugReporter(), ValueExpr); + reportBug(Msg, Error, BT, N, Region, C.getBugReporter(), ValueExpr); } /// Cleaning up the program state. @@ -546,19 +542,19 @@ void NullabilityChecker::checkEvent(ImplicitNullDerefEvent Event) const { if (!TrackedNullability) return; - if (ChecksEnabled[CK_NullableDereferenced] && + if (NullableDereferenced.isEnabled() && TrackedNullability->getValue() == Nullability::Nullable) { BugReporter &BR = *Event.BR; // Do not suppress errors on defensive code paths, because dereferencing // a nullable pointer is always an error. if (Event.IsDirectDereference) reportBug("Nullable pointer is dereferenced", - ErrorKind::NullableDereferenced, CK_NullableDereferenced, + ErrorKind::NullableDereferenced, NullableDereferenced, Event.SinkNode, Region, BR); else { reportBug("Nullable pointer is passed to a callee that requires a " "non-null", - ErrorKind::NullablePassedToNonnull, CK_NullableDereferenced, + ErrorKind::NullablePassedToNonnull, NullableDereferenced, Event.SinkNode, Region, BR); } } @@ -710,29 +706,28 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, Nullability RetExprTypeLevelNullability = getNullabilityAnnotation(lookThroughImplicitCasts(RetExpr)->getType()); - bool NullReturnedFromNonNull = (RequiredNullability == Nullability::Nonnull && - Nullness == NullConstraint::IsNull); - if (ChecksEnabled[CK_NullReturnedFromNonnull] && NullReturnedFromNonNull && - RetExprTypeLevelNullability != Nullability::Nonnull && - !InSuppressedMethodFamily) { - ExplodedNode *N = C.generateErrorNode(State); - if (!N) - return; + if (RequiredNullability == Nullability::Nonnull && + Nullness == NullConstraint::IsNull) { + if (NullReturnedFromNonnull.isEnabled() && + RetExprTypeLevelNullability != Nullability::Nonnull && + !InSuppressedMethodFamily) { + ExplodedNode *N = C.generateErrorNode(State); + if (!N) + return; - SmallString<256> SBuf; - llvm::raw_svector_ostream OS(SBuf); - OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null"); - OS << " returned from a " << C.getDeclDescription(D) << - " that is expected to return a non-null value"; - reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull, - CK_NullReturnedFromNonnull, N, nullptr, C, - RetExpr); - return; - } + SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null"); + OS << " returned from a " << C.getDeclDescription(D) + << " that is expected to return a non-null value"; + reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull, + NullReturnedFromNonnull, N, nullptr, C, + RetExpr); + return; + } - // If null was returned from a non-null function, mark the nullability - // invariant as violated even if the diagnostic was suppressed. - if (NullReturnedFromNonNull) { + // If null was returned from a non-null function, mark the nullability + // invariant as violated even if the diagnostic was suppressed. State = State->set<InvariantViolated>(true); C.addTransition(State); return; @@ -746,7 +741,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, State->get<NullabilityMap>(Region); if (TrackedNullability) { Nullability TrackedNullabValue = TrackedNullability->getValue(); - if (ChecksEnabled[CK_NullableReturnedFromNonnull] && + if (NullableReturnedFromNonnull.isEnabled() && Nullness != NullConstraint::IsNotNull && TrackedNullabValue == Nullability::Nullable && RequiredNullability == Nullability::Nonnull) { @@ -758,7 +753,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, " that is expected to return a non-null value"; reportBugIfInvariantHolds(OS.str(), ErrorKind::NullableReturnedToNonnull, - CK_NullableReturnedFromNonnull, N, Region, C); + NullableReturnedFromNonnull, N, Region, C); } return; } @@ -809,8 +804,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, unsigned ParamIdx = Param->getFunctionScopeIndex() + 1; - if (ChecksEnabled[CK_NullPassedToNonnull] && - Nullness == NullConstraint::IsNull && + if (NullPassedToNonnull.isEnabled() && Nullness == NullConstraint::IsNull && ArgExprTypeLevelNullability != Nullability::Nonnull && RequiredNullability == Nullability::Nonnull && isDiagnosableCall(Call)) { @@ -824,7 +818,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, OS << " passed to a callee that requires a non-null " << ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter"; reportBugIfInvariantHolds(OS.str(), ErrorKind::NilPassedToNonnull, - CK_NullPassedToNonnull, N, nullptr, C, ArgExpr, + NullPassedToNonnull, N, nullptr, C, ArgExpr, /*SuppressPath=*/false); return; } @@ -841,7 +835,7 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, TrackedNullability->getValue() != Nullability::Nullable) continue; - if (ChecksEnabled[CK_NullablePassedToNonnull] && + if (NullablePassedToNonnull.isEnabled() && RequiredNullability == Nullability::Nonnull && isDiagnosableCall(Call)) { ExplodedNode *N = C.addTransition(State); @@ -850,17 +844,16 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, OS << "Nullable pointer is passed to a callee that requires a non-null " << ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter"; reportBugIfInvariantHolds(OS.str(), ErrorKind::NullablePassedToNonnull, - CK_NullablePassedToNonnull, N, Region, C, + NullablePassedToNonnull, N, Region, C, ArgExpr, /*SuppressPath=*/true); return; } - if (ChecksEnabled[CK_NullableDereferenced] && + if (NullableDereferenced.isEnabled() && Param->getType()->isReferenceType()) { ExplodedNode *N = C.addTransition(State); - reportBugIfInvariantHolds("Nullable pointer is dereferenced", - ErrorKind::NullableDereferenced, - CK_NullableDereferenced, N, Region, C, - ArgExpr, /*SuppressPath=*/true); + reportBugIfInvariantHolds( + "Nullable pointer is dereferenced", ErrorKind::NullableDereferenced, + NullableDereferenced, N, Region, C, ArgExpr, /*SuppressPath=*/true); return; } continue; @@ -1294,7 +1287,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S, bool NullAssignedToNonNull = (LocNullability == Nullability::Nonnull && RhsNullness == NullConstraint::IsNull); - if (ChecksEnabled[CK_NullPassedToNonnull] && NullAssignedToNonNull && + if (NullPassedToNonnull.isEnabled() && NullAssignedToNonNull && ValNullability != Nullability::Nonnull && ValueExprTypeLevelNullability != Nullability::Nonnull && !isARCNilInitializedLocal(C, S)) { @@ -1312,7 +1305,7 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S, OS << (LocType->isObjCObjectPointerType() ? "nil" : "Null"); OS << " assigned to a pointer which is expected to have non-null value"; reportBugIfInvariantHolds(OS.str(), ErrorKind::NilAssignedToNonnull, - CK_NullPassedToNonnull, N, nullptr, C, ValueStmt); + NullPassedToNonnull, N, nullptr, C, ValueStmt); return; } @@ -1338,13 +1331,13 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S, if (RhsNullness == NullConstraint::IsNotNull || TrackedNullability->getValue() != Nullability::Nullable) return; - if (ChecksEnabled[CK_NullablePassedToNonnull] && + if (NullablePassedToNonnull.isEnabled() && LocNullability == Nullability::Nonnull) { ExplodedNode *N = C.addTransition(State, C.getPredecessor()); reportBugIfInvariantHolds("Nullable pointer is assigned to a pointer " "which is expected to have non-null value", ErrorKind::NullableAssignedToNonnull, - CK_NullablePassedToNonnull, N, ValueRegion, C); + NullablePassedToNonnull, N, ValueRegion, C); } return; } @@ -1391,28 +1384,26 @@ void NullabilityChecker::printState(raw_ostream &Out, ProgramStateRef State, } } -void ento::registerNullabilityBase(CheckerManager &mgr) { - mgr.registerChecker<NullabilityChecker>(); -} - -bool ento::shouldRegisterNullabilityBase(const CheckerManager &mgr) { - return true; -} - -#define REGISTER_CHECKER(name, trackingRequired) \ - void ento::register##name##Checker(CheckerManager &mgr) { \ - NullabilityChecker *checker = mgr.getChecker<NullabilityChecker>(); \ - checker->ChecksEnabled[NullabilityChecker::CK_##name] = true; \ - checker->CheckNames[NullabilityChecker::CK_##name] = \ - mgr.getCurrentCheckerName(); \ - checker->NeedTracking = checker->NeedTracking || trackingRequired; \ - checker->NoDiagnoseCallsToSystemHeaders = \ - checker->NoDiagnoseCallsToSystemHeaders || \ - mgr.getAnalyzerOptions().getCheckerBooleanOption( \ - checker, "NoDiagnoseCallsToSystemHeaders", true); \ +// The checker group "nullability" (which consists of the checkers that are +// implemented in this file) has a group-level configuration option which +// affects all the checkers in the group. As this is a completely unique +// remnant of old design (this is the only group option in the analyzer), there +// is no machinery to inject the group name from `Checkers.td`, so it is simply +// hardcoded here: +constexpr llvm::StringLiteral GroupName = "nullability"; +constexpr llvm::StringLiteral GroupOptName = "NoDiagnoseCallsToSystemHeaders"; + +#define REGISTER_CHECKER(NAME, TRACKING_REQUIRED) \ + void ento::register##NAME##Checker(CheckerManager &Mgr) { \ + NullabilityChecker *Chk = Mgr.getChecker<NullabilityChecker>(); \ + Chk->NAME.enable(Mgr); \ + Chk->NeedTracking = Chk->NeedTracking || TRACKING_REQUIRED; \ + Chk->NoDiagnoseCallsToSystemHeaders = \ + Mgr.getAnalyzerOptions().getCheckerBooleanOption(GroupName, \ + GroupOptName, true); \ } \ \ - bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \ + bool ento::shouldRegister##NAME##Checker(const CheckerManager &) { \ return true; \ } diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c index 66b9be9795f12..78ee00deea18d 100644 --- a/clang/test/Analysis/analyzer-enabled-checkers.c +++ b/clang/test/Analysis/analyzer-enabled-checkers.c @@ -34,7 +34,6 @@ // CHECK-NEXT: core.uninitialized.CapturedBlockVariable // CHECK-NEXT: core.uninitialized.UndefReturn // CHECK-NEXT: deadcode.DeadStores -// CHECK-NEXT: nullability.NullabilityBase // CHECK-NEXT: nullability.NullPassedToNonnull // CHECK-NEXT: nullability.NullReturnedFromNonnull // CHECK-NEXT: security.insecureAPI.SecuritySyntaxChecker diff --git a/clang/test/Analysis/bugfix-124477.m b/clang/test/Analysis/bugfix-124477.m index 80820f4c93444..8bb0196b2f9b8 100644 --- a/clang/test/Analysis/bugfix-124477.m +++ b/clang/test/Analysis/bugfix-124477.m @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling,nullability.NullableDereferenced,nullability.NullabilityBase -x objective-c %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling,nullability.NullableDereferenced -x objective-c %s /* This test is reduced from a static analyzer crash. The bug causing the crash is explained in #124477. It can only be triggered in some diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c index 8c6078a49c231..7f9c9ff4c9fd7 100644 --- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c +++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c @@ -42,7 +42,6 @@ // CHECK-NEXT: core.uninitialized.CapturedBlockVariable // CHECK-NEXT: core.uninitialized.UndefReturn // CHECK-NEXT: deadcode.DeadStores -// CHECK-NEXT: nullability.NullabilityBase // CHECK-NEXT: nullability.NullPassedToNonnull // CHECK-NEXT: nullability.NullReturnedFromNonnull // CHECK-NEXT: security.insecureAPI.SecuritySyntaxChecker From 4aa581695cc6e960c1254f6f225fbba695be701e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.n...@ericsson.com> Date: Mon, 16 Jun 2025 16:25:49 +0200 Subject: [PATCH 2/2] Fix wrapping of multiline strings Was broken by git-clang-format which is not aware of string concatenation. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index d6a1b9bbc1fdd..211ce585fbac8 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -334,9 +334,8 @@ let ParentPackage = Nullability in { def NullReturnedFromNonnullChecker : Checker<"NullReturnedFromNonnull">, - HelpText< - "Warns when a null pointer is returned from a function that has " - "_Nonnull return type.">, + HelpText<"Warns when a null pointer is returned from a function that " + "has _Nonnull return type.">, Documentation<HasDocumentation>; def NullableDereferencedChecker @@ -346,16 +345,14 @@ let ParentPackage = Nullability in { def NullablePassedToNonnullChecker : Checker<"NullablePassedToNonnull">, - HelpText< - "Warns when a nullable pointer is passed to a pointer which has a " - "_Nonnull type.">, + HelpText<"Warns when a nullable pointer is passed to a pointer which " + "has a _Nonnull type.">, Documentation<HasDocumentation>; def NullableReturnedFromNonnullChecker : Checker<"NullableReturnedFromNonnull">, HelpText<"Warns when a nullable pointer is returned from a function " - "that has " - "_Nonnull return type.">, + "that has _Nonnull return type.">, Documentation<NotDocumented>; } // end "nullability" _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits