[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
Szelethus added a comment. I personally find the registration process very error-prone, the current infrastructure around it makes it super easy to mess up in a way that won't be detected for months, so we probably need a better approach altogether. It shouldn't be possible to do any harm within a registry function, especially not in a way that affects other checkers. https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
Szelethus added a comment.
Herald added a subscriber: donat.nagy.
Bad news, this approach doesn't work too well, and here's why:
https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Core/CheckerRegistry.cpp#L115
void CheckerRegistry::initializeManager(CheckerManager ,
SmallVectorImpl )
const {
// Sort checkers for efficient collection.
llvm::sort(Checkers, checkerNameLT);
// Collect checkers enabled by the options.
CheckerInfoSet enabledCheckers;
for (auto : opts)
collectCheckers(Checkers, Packages, i, enabledCheckers);
// Initialize the CheckerManager with all enabled checkers.
for (const auto *i :enabledCheckers) {
checkerMgr.setCurrentCheckName(CheckName(i->FullName));
i->Initialize(checkerMgr);
}
}
Note that `Initialize` is a function pointer that points to
`register##CHECKERNAME`, so a single registry function should only register one
checker, because `setCurrentCheckName` is only called once, resulting in
`MallocChecker`'s checker object receiving the `cpluscplus.InnetPointer` name.
It's very not-obvious how to fix this gracefully :/ You could call
`CheckerManager::setCurrentCheckName` within the registry function, but you
have to supply a full name, which adds unnecessary maintenance cost (for
example, if someone moves a checker out of alpha, or puts one back).
I found this bug while trying to fix checker options, and noticed that the
`Optimistic` flag of `MallocChecker` is acquired as
`cplusplus.InnerPointer:Optimistic`, instead of `unix.Malloc:Optimistic`. Let's
keep this in for now, and I'll try to look for ways to express dependencies
while avoiding this issue.
https://reviews.llvm.org/D50211
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
rnkovacs added a comment. In https://reviews.llvm.org/D50211#1190146, @NoQ wrote: > Welcome to the club! :D Thanks, makes me feel better. https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
NoQ added a comment. Welcome to the club! https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
rnkovacs closed this revision. rnkovacs added a comment. Committed in r339067, I just messed up the revision-closing line in the commit message. https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
rnkovacs updated this revision to Diff 159244.
rnkovacs marked an inline comment as done.
rnkovacs added a comment.
Replace empty `Optional`s with `None`s.
https://reviews.llvm.org/D50211
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -194,6 +194,7 @@
CK_NewDeleteChecker,
CK_NewDeleteLeaksChecker,
CK_MismatchedDeallocatorChecker,
+CK_InnerPointerChecker,
CK_NumCheckKinds
};
@@ -1662,22 +1663,24 @@
case AF_IfNameIndex: {
if (ChecksEnabled[CK_MallocChecker])
return CK_MallocChecker;
-
-return Optional();
+return None;
}
case AF_CXXNew:
- case AF_CXXNewArray:
- // FIXME: Add new CheckKind for AF_InnerBuffer.
- case AF_InnerBuffer: {
+ case AF_CXXNewArray: {
if (IsALeakCheck) {
if (ChecksEnabled[CK_NewDeleteLeaksChecker])
return CK_NewDeleteLeaksChecker;
}
else {
if (ChecksEnabled[CK_NewDeleteChecker])
return CK_NewDeleteChecker;
}
-return Optional();
+return None;
+ }
+ case AF_InnerBuffer: {
+if (ChecksEnabled[CK_InnerPointerChecker])
+ return CK_InnerPointerChecker;
+return None;
}
case AF_None: {
llvm_unreachable("no family");
@@ -1980,7 +1983,8 @@
SymbolRef Sym) const {
if (!ChecksEnabled[CK_MallocChecker] &&
- !ChecksEnabled[CK_NewDeleteChecker])
+ !ChecksEnabled[CK_NewDeleteChecker] &&
+ !ChecksEnabled[CK_InnerPointerChecker])
return;
Optional CheckKind = getCheckIfTracked(C, Sym);
@@ -3109,6 +3113,18 @@
}
}
+// Intended to be used in InnerPointerChecker to register the part of
+// MallocChecker connected to it.
+void ento::registerInnerPointerCheckerAux(CheckerManager ) {
+registerCStringCheckerBasic(mgr);
+MallocChecker *checker = mgr.registerChecker();
+checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption(
+"Optimistic", false, checker);
+checker->ChecksEnabled[MallocChecker::CK_InnerPointerChecker] = true;
+checker->CheckNames[MallocChecker::CK_InnerPointerChecker] =
+mgr.getCurrentCheckName();
+}
+
#define REGISTER_CHECKER(name) \
void ento::register##name(CheckerManager ) { \
registerCStringCheckerBasic(mgr); \
Index: lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
===
--- lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
+++ lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
@@ -20,5 +20,8 @@
/// Register the checker which evaluates CString API calls.
void registerCStringCheckerBasic(CheckerManager );
+/// Register the part of MallocChecker connected to InnerPointerChecker.
+void registerInnerPointerCheckerAux(CheckerManager );
+
}}
#endif /* INTERCHECKERAPI_H_ */
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -15,6 +15,7 @@
#include "AllocationState.h"
#include "ClangSACheckers.h"
+#include "InterCheckerAPI.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -313,6 +314,6 @@
} // end namespace clang
void ento::registerInnerPointerChecker(CheckerManager ) {
- registerNewDeleteChecker(Mgr);
+ registerInnerPointerCheckerAux(Mgr);
Mgr.registerChecker();
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
NoQ added a comment. Yay thx! Yeah, that's the best behavior i can imagine here. Repository: rC Clang https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
rnkovacs added a comment. In https://reviews.llvm.org/D50211#1186630, @NoQ wrote: > I see, so that's how it's done! > > I also noticed that checker name was weird in exploded graph dumps, i.e. it > was showing regular new/delete stuff as if it was done by InnerPointer > checker. I'll check if this is fixed tomorrow. This seemed interesting, so I tried out myself on the `NewDelete-path-notes.cpp` test file. I saw the following (using IP and ND abbreviations for InnerPointer and NewDelete respectively): Before the patch, - If only IP was turned on, I got ND's warnings with IP's name everywhere in the exploded graph. - If only ND was turned on, I got the warnings under ND's tag. - If both IP and ND were turned on, I got the warnings, and saw ND's name beside any allocation state change in the graph, but the bugs seemed to be tagged with IP's name in the end. After this patch, - If only IP is turned on, I get no warning but I see IP's tag in the graph on the last PurgeDeadSymbols node (I guess it's normal?). - If only ND is turned on, I get the warnings with ND's tag everywhere. - If both IP and ND are turned on, I get the warnings, and I see no trace of IP, only ND tags everywhere. It was really messed up before, but I think I see an improvement there. Repository: rC Clang https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. I see, so that's how it's done! I also noticed that checker name was weird in exploded graph dumps, i.e. it was showing regular new/delete stuff as if it was done by InnerPointer checker. I'll check if this is fixed tomorrow. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1685 + +return Optional(); + } It's usually just `None`, but i guess whoever touched this code before didn't know; it's pretty hard to discover. Repository: rC Clang https://reviews.llvm.org/D50211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50211: [analyzer] Fix displayed checker name for InnerPointerChecker
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
For `InnerPointerChecker` to function properly, both the checker itself and
`MallocChecker`'s capabilities that handle relevant use-after-free problems
need to be turned on. So far, the latter part has been developed under the name
of `MallocChecker`'s `NewDelete` sub-checker, often causing warnings to appear
under that name. This patch defines a new `CheckKind` within `MallocChecker`
for the inner pointer checking functionality, so that the correct name is
displayed on warnings.
Tested on `clang-tidy`.
Repository:
rC Clang
https://reviews.llvm.org/D50211
Files:
lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -194,6 +194,7 @@
CK_NewDeleteChecker,
CK_NewDeleteLeaksChecker,
CK_MismatchedDeallocatorChecker,
+CK_InnerPointerChecker,
CK_NumCheckKinds
};
@@ -1666,9 +1667,7 @@
return Optional();
}
case AF_CXXNew:
- case AF_CXXNewArray:
- // FIXME: Add new CheckKind for AF_InnerBuffer.
- case AF_InnerBuffer: {
+ case AF_CXXNewArray: {
if (IsALeakCheck) {
if (ChecksEnabled[CK_NewDeleteLeaksChecker])
return CK_NewDeleteLeaksChecker;
@@ -1679,6 +1678,12 @@
}
return Optional();
}
+ case AF_InnerBuffer: {
+if (ChecksEnabled[CK_InnerPointerChecker])
+ return CK_InnerPointerChecker;
+
+return Optional();
+ }
case AF_None: {
llvm_unreachable("no family");
}
@@ -1980,7 +1985,8 @@
SymbolRef Sym) const {
if (!ChecksEnabled[CK_MallocChecker] &&
- !ChecksEnabled[CK_NewDeleteChecker])
+ !ChecksEnabled[CK_NewDeleteChecker] &&
+ !ChecksEnabled[CK_InnerPointerChecker])
return;
Optional CheckKind = getCheckIfTracked(C, Sym);
@@ -3109,6 +3115,18 @@
}
}
+// Intended to be used in InnerPointerChecker to register the part of
+// MallocChecker connected to it.
+void ento::registerInnerPointerCheckerAux(CheckerManager ) {
+registerCStringCheckerBasic(mgr);
+MallocChecker *checker = mgr.registerChecker();
+checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption(
+"Optimistic", false, checker);
+checker->ChecksEnabled[MallocChecker::CK_InnerPointerChecker] = true;
+checker->CheckNames[MallocChecker::CK_InnerPointerChecker] =
+mgr.getCurrentCheckName();
+}
+
#define REGISTER_CHECKER(name)
\
void ento::register##name(CheckerManager ) {
\
registerCStringCheckerBasic(mgr);
\
Index: lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
===
--- lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
+++ lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
@@ -20,5 +20,8 @@
/// Register the checker which evaluates CString API calls.
void registerCStringCheckerBasic(CheckerManager );
+/// Register the part of MallocChecker connected to InnerPointerChecker.
+void registerInnerPointerCheckerAux(CheckerManager );
+
}}
#endif /* INTERCHECKERAPI_H_ */
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -15,6 +15,7 @@
#include "AllocationState.h"
#include "ClangSACheckers.h"
+#include "InterCheckerAPI.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -316,6 +317,6 @@
} // end namespace clang
void ento::registerInnerPointerChecker(CheckerManager ) {
- registerNewDeleteChecker(Mgr);
+ registerInnerPointerCheckerAux(Mgr);
Mgr.registerChecker();
}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -194,6 +194,7 @@
CK_NewDeleteChecker,
CK_NewDeleteLeaksChecker,
CK_MismatchedDeallocatorChecker,
+CK_InnerPointerChecker,
CK_NumCheckKinds
};
@@ -1666,9 +1667,7 @@
return Optional();
}
case AF_CXXNew:
- case AF_CXXNewArray:
- // FIXME: Add new CheckKind for AF_InnerBuffer.
- case AF_InnerBuffer: {
+ case AF_CXXNewArray: {
if (IsALeakCheck) {
if
