Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, Charusso, dcoughlin,
baloghadamsoftware, rnkovacs.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, whisperity.
Szelethus added parent revisions: D65575: [analyzer] Mention whether an event
is about a condition in a bug report part 1, D65578: [analyzer][NFC] Make sure
that the BugReport is not modified during the construction of non-visitor
pieces.
We defined (on the mailing list and here on phabricator) 2 different cases
where retrieving information about a control dependency condition is very
important:
1. When the condition's last write happened in a different stack frame
2. When the collapse point of the condition (when we can constrain it to be
true/false) didn't happen in the actual condition.
It seems like we solved this problem with the help of expression value
tracking, and have started working on better diagnostics notes about this
process.
Expression value tracking is nothing more than registering a variety of
visitors to construct reports about it. Each of the registered visitors
(`ReturnVisitor`, `FindLastStoreVisitor`, `NoStoreFuncVisitor`, etc) have
//something// to go by: a `MemRegion`, an `SVal`, an `ExplodedNode`, etc. For
this reason, better explaining a last write is super simple, we can always just
pass on some more information to the visitor in question (as seen in D65575
<https://reviews.llvm.org/D65575>).
`ConditionBRVisitor` is a different beast, as it was built for a different
purpose. It is responsible for constructing events at, well, conditions, and is
registered only once, and isn't a part of the "expression value tracking
family". Unfortunately, it is also //the// visitor to tinker with for
constructing better diagnostics about the collapse point problem.
This creates a need for alternative way to communicate with
`ConditionBRVisitor` that a specific condition is being tracked for for the
reason of being a control dependency. Since at almost all
`PathDiagnosticEventPiece` construction the visitor checks interestingness, it
makes sense to pair interestingness with a reason as to why we marked an entity
as such.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D65723
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1983,7 +1983,7 @@
MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
LVNode, R, EnableNullFPSuppression, report, V);
- report.markInteresting(V);
+ report.markInteresting(V, TKind);
report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(R));
// If the contents are symbolic, find out when they became null.
@@ -2045,7 +2045,7 @@
const MemRegion *RegionRVal = RVal.getAsRegion();
if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
- report.markInteresting(RegionRVal);
+ report.markInteresting(RegionRVal, TKind);
report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(
loc::MemRegionVal(RegionRVal), /*assumption=*/false));
}
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2091,30 +2091,53 @@
}
}
-void BugReport::markInteresting(SymbolRef sym) {
+template <class T>
+static void insertToInterestingnessMap(
+ llvm::DenseMap<T, bugreporter::TrackingKind> &InterestingnessMap, T Val,
+ bugreporter::TrackingKind TKind) {
+ auto Result = InterestingnessMap.insert({Val, TKind});
+
+ static_assert(
+ static_cast<unsigned>(bugreporter::TrackingKind::NumTrackingKinds) == 2,
+ "BugReport::markInteresting currently can only handle 2 different "
+ "tracking kinds! Please define what tracking kind should this entitiy"
+ "have, if it was already marked as interesting with a different kind!");
+ // Even if this symbol/region was already marked as interesting as a
+ // condition, if we later mark it as interesting again but with
+ // thorough tracking, overwrite it. Entities marked with thorough
+ // interestiness are the most important (or most interesting, if you will),
+ // and we wouldn't like to downplay their importance.
+ if (!Result.second)
+ if (TKind == bugreporter::TrackingKind::Thorough)
+ Result.first->getSecond() = bugreporter::TrackingKind::Thorough;
+}
+
+void BugReport::markInteresting(SymbolRef sym,
+ bugreporter::TrackingKind TKind) {
if (!sym)
return;
- InterestingSymbols.insert(sym);
+ insertToInterestingnessMap(InterestingSymbols, sym, TKind);
if (const auto *meta = dyn_cast<SymbolMetadata>(sym))
- InterestingRegions.insert(meta->getRegion());
+ markInteresting(meta->getRegion(), TKind);
}
-void BugReport::markInteresting(const MemRegion *R) {
+void BugReport::markInteresting(const MemRegion *R,
+ bugreporter::TrackingKind TKind) {
if (!R)
return;
R = R->getBaseRegion();
- InterestingRegions.insert(R);
+ insertToInterestingnessMap(InterestingRegions, R, TKind);
if (const auto *SR = dyn_cast<SymbolicRegion>(R))
- InterestingSymbols.insert(SR->getSymbol());
+ markInteresting(SR->getSymbol(), TKind);
}
-void BugReport::markInteresting(SVal V) {
- markInteresting(V.getAsRegion());
- markInteresting(V.getAsSymbol());
+void BugReport::markInteresting(SVal V, bugreporter::TrackingKind TKind) {
+ markInteresting(V.getAsRegion(), TKind);
+ markInteresting(V.getAsSymbol(), TKind);
}
void BugReport::markInteresting(const LocationContext *LC) {
@@ -2123,28 +2146,61 @@
InterestingLocationContexts.insert(LC);
}
-bool BugReport::isInteresting(SVal V) const {
- return isInteresting(V.getAsRegion()) || isInteresting(V.getAsSymbol());
-}
-
-bool BugReport::isInteresting(SymbolRef sym) const {
+Optional<bugreporter::TrackingKind>
+BugReport::getInterestingnessKind(SVal V) const {
+ auto RKind = getInterestingnessKind(V.getAsRegion());
+ auto SKind = getInterestingnessKind(V.getAsSymbol());
+ static_assert(
+ static_cast<unsigned>(bugreporter::TrackingKind::NumTrackingKinds) == 2,
+ "BugReport::getInterestingnessKind currently can only handle 2 different "
+ "tracking kinds! Please define what tracking kind should we return here "
+ "when the kind of getAsRegion() and getAsSymbol() is different!");
+ if (!RKind)
+ return SKind;
+ if (!SKind)
+ return RKind;
+ // If either is marked with throrough tracking, return that, we wouldn't like
+ // to downplay a note's importance by 'only' mentioning it as a condition.
+ return *RKind != *SKind ? bugreporter::TrackingKind::Thorough : RKind;
+}
+
+Optional<bugreporter::TrackingKind>
+BugReport::getInterestingnessKind(SymbolRef sym) const {
if (!sym)
- return false;
+ return None;
// We don't currently consider metadata symbols to be interesting
// even if we know their region is interesting. Is that correct behavior?
- return InterestingSymbols.count(sym);
+ auto It = InterestingSymbols.find(sym);
+ if (It == InterestingSymbols.end())
+ return None;
+ return It->getSecond();
}
-bool BugReport::isInteresting(const MemRegion *R) const {
+Optional<bugreporter::TrackingKind>
+BugReport::getInterestingnessKind(const MemRegion *R) const {
if (!R)
- return false;
+ return None;
+
R = R->getBaseRegion();
- bool b = InterestingRegions.count(R);
- if (b)
- return true;
+ auto It = InterestingRegions.find(R);
+ if (It != InterestingRegions.end())
+ return It->getSecond();
+
if (const auto *SR = dyn_cast<SymbolicRegion>(R))
- return InterestingSymbols.count(SR->getSymbol());
- return false;
+ return getInterestingnessKind(SR->getSymbol());
+ return None;
+}
+
+bool BugReport::isInteresting(SVal V) const {
+ return getInterestingnessKind(V).hasValue();
+}
+
+bool BugReport::isInteresting(SymbolRef sym) const {
+ return getInterestingnessKind(sym).hasValue();
+}
+
+bool BugReport::isInteresting(const MemRegion *R) const {
+ return getInterestingnessKind(R).hasValue();
}
bool BugReport::isInteresting(const LocationContext *LC) const {
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -91,12 +91,14 @@
enum class TrackingKind {
/// Default tracking kind -- specifies that as much information should be
/// gathered about the tracked expression value as possible.
- Thorough,
+ Thorough = 0,
/// Specifies that a more moderate tracking should be used for the expression
/// value. This will essentially make sure that functions relevant to the it
/// aren't pruned, but otherwise relies on the user reading the code or
/// following the arrows.
- Condition
+ Condition = 1,
+ /// Number of different tracking kinds for sanity checks.
+ NumTrackingKinds = 2
};
/// Attempts to add visitors to track expression value back to its point of
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -114,14 +114,15 @@
/// diagnostics to include when constructing the final path diagnostic.
/// The stack is largely used by BugReporter when generating PathDiagnostics
/// for multiple PathDiagnosticConsumers.
- llvm::DenseSet<SymbolRef> InterestingSymbols;
+ llvm::DenseMap<SymbolRef, bugreporter::TrackingKind> InterestingSymbols;
/// A (stack of) set of regions that are registered with this report as being
/// "interesting", and thus used to help decide which diagnostics
/// to include when constructing the final path diagnostic.
/// The stack is largely used by BugReporter when generating PathDiagnostics
/// for multiple PathDiagnosticConsumers.
- llvm::DenseSet<const MemRegion *> InterestingRegions;
+ llvm::DenseMap<const MemRegion *, bugreporter::TrackingKind>
+ InterestingRegions;
/// A set of location contexts that correspoind to call sites which should be
/// considered "interesting".
@@ -219,9 +220,24 @@
/// Disable all path pruning when generating a PathDiagnostic.
void disablePathPruning() { DoNotPrunePath = true; }
- void markInteresting(SymbolRef sym);
- void markInteresting(const MemRegion *R);
- void markInteresting(SVal V);
+ /// Marks a symbol as interesting. Different kinds of interestingness will
+ /// be processed differently by visitors (e.g. if the tracking kind is
+ /// condition, will append "will be used as a condition" to the message).
+ void markInteresting(SymbolRef sym, bugreporter::TrackingKind TKind =
+ bugreporter::TrackingKind::Thorough);
+
+ /// Marks a region as interesting. Different kinds of interestingness will
+ /// be processed differently by visitors (e.g. if the tracking kind is
+ /// condition, will append "will be used as a condition" to the message).
+ void markInteresting(
+ const MemRegion *R,
+ bugreporter::TrackingKind TKind = bugreporter::TrackingKind::Thorough);
+
+ /// Marks a symbolic value as interesting. Different kinds of interestingness
+ /// will be processed differently by visitors (e.g. if the tracking kind is
+ /// condition, will append "will be used as a condition" to the message).
+ void markInteresting(SVal V, bugreporter::TrackingKind TKind =
+ bugreporter::TrackingKind::Thorough);
void markInteresting(const LocationContext *LC);
bool isInteresting(SymbolRef sym) const;
@@ -229,6 +245,14 @@
bool isInteresting(SVal V) const;
bool isInteresting(const LocationContext *LC) const;
+ Optional<bugreporter::TrackingKind>
+ getInterestingnessKind(SymbolRef sym) const;
+
+ Optional<bugreporter::TrackingKind>
+ getInterestingnessKind(const MemRegion *R) const;
+
+ Optional<bugreporter::TrackingKind> getInterestingnessKind(SVal V) const;
+
/// Returns whether or not this report should be considered valid.
///
/// Invalid reports are those that have been classified as likely false
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
