Skip to content

[analyzer][NFC] Introduce framework for checker families #139256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
May 26, 2025
Merged

[analyzer][NFC] Introduce framework for checker families #139256

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3bead14
[analyzer][NFC] Introduce framework for checker families
NagyDonat May 8, 2025
23d128c
Fix abbreviation of 'CheckerFrontend'
NagyDonat May 9, 2025
8459b2f
Use class name as tag description of checker family
NagyDonat May 13, 2025
76950c1
Make CheckerFrontendWithBugType a subclass of BugType
NagyDonat May 13, 2025
1cbbc02
Rename `CheckerFrontend *CF` to `Checker`
NagyDonat May 13, 2025
c7a96f2
Declare data members in separate declarations
NagyDonat May 13, 2025
4e8035f
Use fold expression over comma
NagyDonat May 13, 2025
7da3862
NFC Rename getTagDescription to getDebugName
steakhal May 15, 2025
4becdeb
HACK: Smuggle in the checker name into the CheckerFamily class
steakhal May 15, 2025
13f4a30
Use registration function name fragment as checker family debug name
NagyDonat May 19, 2025
ffe6ac4
s/FamilyChecker/CheckerFamily/ in comment
NagyDonat May 19, 2025
84eb5af
Refactor use of CheckerRegistry::addChecker()
NagyDonat May 20, 2025
04bfbf1
Provide a simplified interface for plugins defining checkers
NagyDonat May 20, 2025
58821ba
Revert complex implementation of `getTagDescription()`
NagyDonat May 26, 2025
84192a7
Update the doc-comment of CheckerFamily
NagyDonat May 26, 2025
9c01aab
Satisfy git-clang-format
NagyDonat May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/include/clang/StaticAnalyzer/Core/Checker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,8 @@ class CheckerFamily : public CheckerBackend, public CHECKs... {
public:
template <typename CHECKER>
static void _register(CHECKER *Chk, CheckerManager &Mgr) {
Chk->CheckerBackendName = Mgr.getCurrentCheckerName();
if (Chk->getDebugName().empty())
Chk->CheckerBackendName = Mgr.getCurrentCheckerDebugName();
(CHECKs::_register(Chk, Mgr), ...);
}

Expand Down
10 changes: 9 additions & 1 deletion clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class CheckerManager {
const AnalyzerOptions &AOptions;
const Preprocessor *PP = nullptr;
CheckerNameRef CurrentCheckerName;
CheckerNameRef CurrentCheckerDebugName;
DiagnosticsEngine &Diags;
std::unique_ptr<CheckerRegistryData> RegistryData;

Expand Down Expand Up @@ -159,9 +160,16 @@ class CheckerManager {

~CheckerManager();

void setCurrentCheckerName(CheckerNameRef name) { CurrentCheckerName = name; }
void setCurrentCheckerName(CheckerNameRef Name) { CurrentCheckerName = Name; }
CheckerNameRef getCurrentCheckerName() const { return CurrentCheckerName; }

void setCurrentCheckerDebugName(CheckerNameRef Name) {
CurrentCheckerDebugName = Name;
}
CheckerNameRef getCurrentCheckerDebugName() const {
return CurrentCheckerDebugName;
}

bool hasPathSensitiveCheckers() const;

const LangOptions &getLangOpts() const { return LangOpts; }
Expand Down
7 changes: 4 additions & 3 deletions clang/include/clang/StaticAnalyzer/Core/CheckerRegistryData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ struct CheckerInfo {
RegisterCheckerFn Initialize = nullptr;
ShouldRegisterFunction ShouldRegister = nullptr;
StringRef FullName;
StringRef DebugName;
StringRef Desc;
StringRef DocumentationUri;
CmdLineOptionList CmdLineOptions;
Expand All @@ -128,9 +129,9 @@ struct CheckerInfo {
}

CheckerInfo(RegisterCheckerFn Fn, ShouldRegisterFunction sfn, StringRef Name,
StringRef Desc, StringRef DocsUri, bool IsHidden)
: Initialize(Fn), ShouldRegister(sfn), FullName(Name), Desc(Desc),
DocumentationUri(DocsUri), IsHidden(IsHidden) {}
StringRef DName, StringRef Desc, StringRef DocsUri, bool IsHidden)
: Initialize(Fn), ShouldRegister(sfn), FullName(Name), DebugName(DName),
Desc(Desc), DocumentationUri(DocsUri), IsHidden(IsHidden) {}

// Used for lower_bound.
explicit CheckerInfo(StringRef FullName) : FullName(FullName) {}
Expand Down
21 changes: 17 additions & 4 deletions clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@
// checker.
//
// The clang_registerCheckers function may add any number of checkers to the
// registry. If any checkers require additional initialization, use the three-
// argument form of CheckerRegistry::addChecker.
// registry. If any checkers require additional initialization, use the
// non-templated form of CheckerRegistry::addChecker.
//
// To load a checker plugin, specify the full path to the dynamic library as
// the argument to the -load option in the cc1 frontend. You can then enable
Expand Down Expand Up @@ -115,9 +115,22 @@ class CheckerRegistry {
public:
/// Adds a checker to the registry. Use this non-templated overload when your
/// checker requires custom initialization.
void addChecker(RegisterCheckerFn Fn, ShouldRegisterFunction sfn,
void addChecker(RegisterCheckerFn Fn, ShouldRegisterFunction Sfn,
StringRef FullName, StringRef DebugName, StringRef Desc,
StringRef DocsUri, bool IsHidden);

/// Adds a checker to the registry. This overload doesn't take a `DebugName`
/// (which usually looks like `DivZeroChecker`), so it uses the user-facing
/// `FullName` (which usually looks like ``core.DivideZero`) as a debug name.
/// THIS IS DEPRECATED and is only provided to preserve compatibility with
/// legacy plugins.
/// TODO: Eventually remove this from the codebase.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can break compatibility. In fact, a clang plugin is only compatible with the same major version of clang already.
There is no such thing in Clang as API or ABI compatibility across major releases.
I'd recommend dropping this compatibility overload.

Copy link
Contributor Author

@NagyDonat NagyDonat May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will remove this method variant from the codebase (I'll need to update a unit test).

Unfortunately this "no DebugName, we need to pass the FullName issue" also affects the templated variant of the method which is below this one, and that one is used by a dozen unit tests, so modifying it would be bothersome (I would like to skip updating it, but even if I do it perhaps it should be in a separate commit).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point me to some examples? That way we could use collective intelligence and maybe I could figure out a way forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be straightforward to update the templated variant of addChecker by adding an extra DebugName parameter and updating all the code that calls it – but it is called from 20+ locations, so I felt that it would be bothersome to update them all:

clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h |141 Col 8|   void addChecker(StringRef FullName, StringRef Desc, StringRef DocsUri,                           
clang/lib/Analysis/plugins/CheckerDependencyHandling/CheckerDependencyHandling.cpp |20 Col 12|   registry.addChecker<Dependency>("example.Dependency", "", "");              
clang/lib/Analysis/plugins/CheckerDependencyHandling/CheckerDependencyHandling.cpp |21 Col 12|   registry.addChecker<DependendentChecker>("example.DependendentChecker", "", 
clang/lib/Analysis/plugins/SampleAnalyzer/MainCallChecker.cpp |48 Col 12|   registry.addChecker<MainCallChecker>(                                                            
clang/unittests/StaticAnalyzer/BugReportInterestingnessTest.cpp |122 Col 16|       Registry.addChecker<InterestingnessTestChecker>("test.Interestingness",                   
clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp |623 Col 14|     Registry.addChecker<CallDescChecker>("test.CallDescChecker", "Description",                          
clang/unittests/StaticAnalyzer/CallEventTest.cpp |58 Col 14|     Registry.addChecker<CXXDeallocatorChecker>("test.CXXDeallocator",                                           
clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp |80 Col 14|     Registry.addChecker<ExprEngineVisitPreChecker>("ExprEngineVisitPreChecker",                           
clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp |89 Col 14|     Registry.addChecker<ExprEngineVisitPostChecker>(                                                      
clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp |98 Col 14|     Registry.addChecker<MemAccessChecker>("MemAccessChecker", "Desc",                                     
clang/unittests/StaticAnalyzer/ObjcBug-124477.cpp |39 Col 14|     Registry.addChecker<FlipFlagOnCheckLocation>("test.FlipFlagOnCheckLocation",                               
clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp |47 Col 14|     Registry.addChecker<CustomChecker>("test.CustomChecker", "Description", "");                   
clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp |76 Col 14|     Registry.addChecker<CustomChecker>("test.LocIncDecChecker", "Description",                     
clang/unittests/StaticAnalyzer/SValSimplifyerTest.cpp |71 Col 14|     Registry.addChecker<SimplifyChecker>("SimplifyChecker", "EmptyDescription",                            
clang/unittests/StaticAnalyzer/SValTest.cpp |160 Col 1| SVAL_TEST(GetConstType, R"(                                                                                          
clang/unittests/StaticAnalyzer/SValTest.cpp |180 Col 1| SVAL_TEST(GetLocAsIntType, R"(                                                                                       
clang/unittests/StaticAnalyzer/SValTest.cpp |202 Col 1| SVAL_TEST(GetSymExprType, R"(                                                                                        
clang/unittests/StaticAnalyzer/SValTest.cpp |224 Col 1| SVAL_TEST(GetPointerType, R"(                                                                                        
clang/unittests/StaticAnalyzer/SValTest.cpp |278 Col 1| SVAL_TEST(GetCompoundType, R"(                                                                                       
clang/unittests/StaticAnalyzer/SValTest.cpp |330 Col 1| SVAL_TEST(GetStringType, R"(                                                                                         
clang/unittests/StaticAnalyzer/SValTest.cpp |342 Col 1| SVAL_TEST(GetThisType, R"(                                                                                           
clang/unittests/StaticAnalyzer/SValTest.cpp |359 Col 1| SVAL_TEST(GetFunctionPtrType, R"(                                                                                    
clang/unittests/StaticAnalyzer/SValTest.cpp |372 Col 1| SVAL_TEST(GetLabelType, R"(                                                                                          
clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp |52 Col 16|       Registry.addChecker<TestReturnValueUnderConstructionChecker>(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Maybe we could expose a macro to fill the debug name with the stringified token of the class?
I'm not a fan of macros but this could be a good use of them if it makes it more ergonomic.
And btw, could we assert that when adding a checker the debug names are unique?

Copy link
Contributor Author

@NagyDonat NagyDonat May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Maybe we could expose a macro to fill the debug name with the stringified token of the class?
I'm not a fan of macros but this could be a good use of them if it makes it more ergonomic.

Most of these are mock checkers where the debug name is completely irrelevant, they won't be used for actual analysis. In fact I created a method called addMockChecker(StringRef FullName) which is sufficient for these simple situations.

And btw, could we assert that when adding a checker the debug names are unique?

Perhaps, although providing unique debug names for the mock checkers in the unit tests is pointless. Overall, the whole "debug name" concept is an extremely unimportant feature that is only used during manual investigation of bugs, so IMO it doesn't deserve complex assertions and we should stop making it the centerpiece of our architecture that demands reorganizations and constrains otherwise good plans.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Maybe we could expose a macro to fill the debug name with the stringified token of the class?
I'm not a fan of macros but this could be a good use of them if it makes it more ergonomic.

Most of these are mock checkers where the debug name is completely irrelevant, they won't be used for actual analysis. In fact I created a method called addMockChecker(StringRef FullName) which is sufficient for these simple situations.

And btw, could we assert that when adding a checker the debug names are unique?

Perhaps, although providing unique debug names for the mock checkers in the unit tests is pointless. Overall, the whole "debug name" concept is an extremely unimportant feature that is only used during manual investigation of bugs, so IMO it doesn't deserve complex assertions and we should stop making it the centerpiece of our architecture that demands reorganizations and constrains otherwise good plans.

Makes sense. Lets not enforce this.

void addChecker(RegisterCheckerFn Fn, ShouldRegisterFunction Sfn,
StringRef FullName, StringRef Desc, StringRef DocsUri,
bool IsHidden);
bool IsHidden) {
addChecker(Fn, Sfn, FullName, /*DebugName =*/FullName, Desc, DocsUri,
IsHidden);
}

/// Adds a checker to the registry. Use this templated overload when your
/// checker does not require any custom initialization.
Expand Down
12 changes: 7 additions & 5 deletions clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ CheckerRegistry::CheckerRegistry(
// Register builtin checkers.
#define GET_CHECKERS
#define CHECKER(FULLNAME, CLASS, HELPTEXT, DOC_URI, IS_HIDDEN) \
addChecker(register##CLASS, shouldRegister##CLASS, FULLNAME, HELPTEXT, \
DOC_URI, IS_HIDDEN);
addChecker(register##CLASS, shouldRegister##CLASS, FULLNAME, #CLASS, \
HELPTEXT, DOC_URI, IS_HIDDEN);

#define GET_PACKAGES
#define PACKAGE(FULLNAME) addPackage(FULLNAME);
Expand Down Expand Up @@ -433,9 +433,10 @@ void CheckerRegistry::addPackageOption(StringRef OptionType,

void CheckerRegistry::addChecker(RegisterCheckerFn Rfn,
ShouldRegisterFunction Sfn, StringRef Name,
StringRef Desc, StringRef DocsUri,
bool IsHidden) {
Data.Checkers.emplace_back(Rfn, Sfn, Name, Desc, DocsUri, IsHidden);
StringRef DebugName, StringRef Desc,
StringRef DocsUri, bool IsHidden) {
Data.Checkers.emplace_back(Rfn, Sfn, Name, DebugName, Desc, DocsUri,
IsHidden);

// Record the presence of the checker in its packages.
StringRef PackageName, LeafName;
Expand All @@ -462,6 +463,7 @@ void CheckerRegistry::initializeManager(CheckerManager &CheckerMgr) const {
// Initialize the CheckerManager with all enabled checkers.
for (const auto *Checker : Data.EnabledCheckers) {
CheckerMgr.setCurrentCheckerName(CheckerNameRef(Checker->FullName));
CheckerMgr.setCurrentCheckerDebugName(CheckerNameRef(Checker->DebugName));
Checker->Initialize(CheckerMgr);
}
}
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/ftime-trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@

// Finally, each checker call back is also present:
//
// CHECK: "name": "Total Stmt:core.DivideZero",
// CHECK: "name": "Total Stmt:DivZeroChecker",
// CHECK-NEXT: "args": {
// CHECK-NEXT: "count": {{[0-9]+}},
// CHECK-NEXT: "avg ms": {{[0-9]+}}
Expand Down