From: Kristof Umann Date: Wed, 21 Aug 2019 21:33:25 +0000 (+0000) Subject: [analyzer][NFC] Add different interestingness kinds X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a5f916ab4dba511a6d37159cf9184fae6257bc1d;p=clang [analyzer][NFC] Add different interestingness kinds We defined (on the mailing list and here on phabricator) 2 different cases where retrieving information about a control dependency condition is very important: * When the condition's last write happened in a different stack frame * 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). 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. Differential Revision: https://reviews.llvm.org/D65723 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@369583 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index c786ab3adf..76c282aae4 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -102,14 +102,15 @@ protected: /// diagnostics to include when constructing the final path diagnostic. /// The stack is largely used by BugReporter when generating PathDiagnostics /// for multiple PathDiagnosticConsumers. - llvm::DenseSet InterestingSymbols; + llvm::DenseMap 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 InterestingRegions; + llvm::DenseMap + InterestingRegions; /// A set of location contexts that correspoind to call sites which should be /// considered "interesting". @@ -209,9 +210,24 @@ public: /// 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; @@ -219,6 +235,14 @@ public: bool isInteresting(SVal V) const; bool isInteresting(const LocationContext *LC) const; + Optional + getInterestingnessKind(SymbolRef sym) const; + + Optional + getInterestingnessKind(const MemRegion *R) const; + + Optional 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 diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index ccf6dcbf62..2c49beef4a 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2101,30 +2101,61 @@ void BugReport::Profile(llvm::FoldingSetNodeID& hash) const { } } -void BugReport::markInteresting(SymbolRef sym) { +template +static void insertToInterestingnessMap( + llvm::DenseMap &InterestingnessMap, T Val, + bugreporter::TrackingKind TKind) { + auto Result = InterestingnessMap.insert({Val, TKind}); + + if (Result.second) + return; + + // 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. + + switch (TKind) { + case bugreporter::TrackingKind::Thorough: + Result.first->getSecond() = bugreporter::TrackingKind::Thorough; + return; + case bugreporter::TrackingKind::Condition: + return; + } + + llvm_unreachable( + "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!"); +} + +void BugReport::markInteresting(SymbolRef sym, + bugreporter::TrackingKind TKind) { if (!sym) return; - InterestingSymbols.insert(sym); + insertToInterestingnessMap(InterestingSymbols, sym, TKind); if (const auto *meta = dyn_cast(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(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) { @@ -2133,28 +2164,68 @@ void BugReport::markInteresting(const LocationContext *LC) { InterestingLocationContexts.insert(LC); } -bool BugReport::isInteresting(SVal V) const { - return isInteresting(V.getAsRegion()) || isInteresting(V.getAsSymbol()); +Optional +BugReport::getInterestingnessKind(SVal V) const { + auto RKind = getInterestingnessKind(V.getAsRegion()); + auto SKind = getInterestingnessKind(V.getAsSymbol()); + 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. + switch(*RKind) { + case bugreporter::TrackingKind::Thorough: + return RKind; + case bugreporter::TrackingKind::Condition: + return SKind; + } + + llvm_unreachable( + "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!"); + return None; } -bool BugReport::isInteresting(SymbolRef sym) const { +Optional +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 +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(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 { diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 3dc50627b6..92e440ae2c 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1950,7 +1950,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary( LVNode, R, EnableNullFPSuppression, report, V); - report.markInteresting(V); + report.markInteresting(V, TKind); report.addVisitor(std::make_unique(R)); // If the contents are symbolic, find out when they became null. @@ -2011,7 +2011,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, const MemRegion *RegionRVal = RVal.getAsRegion(); if (RegionRVal && isa(RegionRVal)) { - report.markInteresting(RegionRVal); + report.markInteresting(RegionRVal, TKind); report.addVisitor(std::make_unique( loc::MemRegionVal(RegionRVal), /*assumption=*/false)); }