From: Ted Kremenek Date: Thu, 9 Sep 2010 19:05:34 +0000 (+0000) Subject: Use FindReportInEquivalenceClass to identify all the nodes used for the trimmed graph... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=61f52bd3c524268e25b48a1ed3730aedd6cc8374;p=clang Use FindReportInEquivalenceClass to identify all the nodes used for the trimmed graph (in BugReporter). This fixes a problem where a leak that happened to occur on both an exit() path and a non-exit() path was getting reported with the exit() path (which users don't care about). This fixes: leak reports should not show paths that end with exit() (but ones that don't end with exit()) git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@113524 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Checker/BugReporter/BugReporter.h b/include/clang/Checker/BugReporter/BugReporter.h index 370d96552c..bc62392449 100644 --- a/include/clang/Checker/BugReporter/BugReporter.h +++ b/include/clang/Checker/BugReporter/BugReporter.h @@ -306,7 +306,8 @@ public: SourceManager& getSourceManager() { return D.getSourceManager(); } virtual void GeneratePathDiagnostic(PathDiagnostic& PD, - BugReportEquivClass& EQ) {} + BugReportEquivClass& EQ, + llvm::SmallVectorImpl &Nodes) {} void Register(BugType *BT); @@ -368,7 +369,8 @@ public: GRStateManager &getStateManager(); virtual void GeneratePathDiagnostic(PathDiagnostic& PD, - BugReportEquivClass& R); + BugReportEquivClass& R, + llvm::SmallVectorImpl &Nodes); void addNotableSymbol(SymbolRef Sym) { NotableSymbols.insert(Sym); diff --git a/lib/Checker/BugReporter.cpp b/lib/Checker/BugReporter.cpp index bffbd52b7d..5043d90526 100644 --- a/lib/Checker/BugReporter.cpp +++ b/lib/Checker/BugReporter.cpp @@ -1567,23 +1567,18 @@ static void CompactPathDiagnostic(PathDiagnostic &PD, const SourceManager& SM) { } void GRBugReporter::GeneratePathDiagnostic(PathDiagnostic& PD, - BugReportEquivClass& EQ) { + BugReportEquivClass& EQ, + llvm::SmallVectorImpl &Nodes) { - std::vector Nodes; - for (BugReportEquivClass::iterator I=EQ.begin(), E=EQ.end(); I!=E; ++I) { - const ExplodedNode* N = I->getEndNode(); - if (N) Nodes.push_back(N); - } - - if (Nodes.empty()) - return; + assert(!Nodes.empty()); // Construct a new graph that contains only a single path from the error // node to a root. const std::pair, std::pair >& - GPair = MakeReportGraph(&getGraph(), &Nodes[0], &Nodes[0] + Nodes.size()); + GPair = MakeReportGraph(&getGraph(), &Nodes[0], + Nodes.data() + Nodes.size()); // Find the BugReport with the original location. BugReport *R = 0; @@ -1657,21 +1652,36 @@ struct FRIEC_WLItem { }; } -static BugReport *FindReportInEquivalenceClass(BugReportEquivClass& EQ) { +static BugReport * +FindReportInEquivalenceClass(BugReportEquivClass& EQ, + llvm::SmallVectorImpl &Nodes) { + BugReportEquivClass::iterator I = EQ.begin(), E = EQ.end(); assert(I != E); BugReport *R = *I; BugType& BT = R->getBugType(); - - if (!BT.isSuppressOnSink()) + + // If we don't need to suppress any of the nodes because they are post-dominated + // by a sink, simply add all the nodes in the equivalence class to 'Nodes'. + if (!BT.isSuppressOnSink()) { + for (BugReportEquivClass::iterator I=EQ.begin(), E=EQ.end(); I!=E; ++I) { + const ExplodedNode* N = I->getEndNode(); + if (N) { + R = *I; + Nodes.push_back(N); + } + } return R; - + } + // For bug reports that should be suppressed when all paths are post-dominated // by a sink node, iterate through the reports in the equivalence class // until we find one that isn't post-dominated (if one exists). We use a // DFS traversal of the ExplodedGraph to find a non-sink node. We could write // this as a recursive function, but we don't want to risk blowing out the // stack for very long paths. + BugReport *ExampleReport = 0; + for (; I != E; ++I) { R = *I; const ExplodedNode *N = R->getEndNode(); @@ -1682,12 +1692,17 @@ static BugReport *FindReportInEquivalenceClass(BugReportEquivClass& EQ) { if (N->isSink()) { assert(false && "BugType::isSuppressSink() should not be 'true' for sink end nodes"); - return R; + return 0; } - - if (N->succ_empty()) - return R; - + + // No successors? By definition this nodes isn't post-dominated by a sink. + if (N->succ_empty()) { + Nodes.push_back(N); + if (!ExampleReport) + ExampleReport = R; + continue; + } + // At this point we know that 'N' is not a sink and it has at least one // successor. Use a DFS worklist to find a non-sink end-of-path node. typedef FRIEC_WLItem WLItem; @@ -1706,15 +1721,17 @@ static BugReport *FindReportInEquivalenceClass(BugReportEquivClass& EQ) { const ExplodedNode *Succ = *WI.I; // End-of-path node? if (Succ->succ_empty()) { - // If we found an end-of-path node that is not a sink, then return - // this report. - if (!Succ->isSink()) - return R; - + // If we found an end-of-path node that is not a sink. + if (!Succ->isSink()) { + Nodes.push_back(N); + if (!ExampleReport) + ExampleReport = R; + WL.clear(); + break; + } // Found a sink? Continue on to the next successor. continue; } - // Mark the successor as visited. If it hasn't been explored, // enqueue it to the DFS worklist. unsigned &mark = Visited[Succ]; @@ -1724,17 +1741,18 @@ static BugReport *FindReportInEquivalenceClass(BugReportEquivClass& EQ) { break; } } - - if (&WL.back() == &WI) + + // The worklist may have been cleared at this point. First + // check if it is empty before checking the last item. + if (!WL.empty() && &WL.back() == &WI) WL.pop_back(); } } - - // If we reach here, the end nodes for all reports in the equivalence - // class are post-dominated by a sink node. - return NULL; -} + // ExampleReport will be NULL if all the nodes in the equivalence class + // were post-dominated by sinks. + return ExampleReport; +} //===----------------------------------------------------------------------===// // DiagnosticCache. This is a hack to cache analyzer diagnostics. It @@ -1780,7 +1798,8 @@ static bool IsCachedDiagnostic(BugReport *R, PathDiagnostic *PD) { } void BugReporter::FlushReport(BugReportEquivClass& EQ) { - BugReport *R = FindReportInEquivalenceClass(EQ); + llvm::SmallVector Nodes; + BugReport *R = FindReportInEquivalenceClass(EQ, Nodes); if (!R) return; @@ -1797,7 +1816,8 @@ void BugReporter::FlushReport(BugReportEquivClass& EQ) { ? R->getDescription() : R->getShortDescription(), BT.getCategory())); - GeneratePathDiagnostic(*D.get(), EQ); + if (!Nodes.empty()) + GeneratePathDiagnostic(*D.get(), EQ, Nodes); if (IsCachedDiagnostic(R, D.get())) return; diff --git a/test/Analysis/plist-output-alternate.m b/test/Analysis/plist-output-alternate.m index ad9f89fcd1..364289d6d2 100644 --- a/test/Analysis/plist-output-alternate.m +++ b/test/Analysis/plist-output-alternate.m @@ -37,6 +37,25 @@ void test_null_field(void) { *(x.p) = 0xDEADBEEF; } +// leak reports should not show paths that end with exit() (but ones that don't end with exit()) +void panic() __attribute__((noreturn)); +enum { kCFNumberSInt8Type = 1, kCFNumberSInt16Type = 2, kCFNumberSInt32Type = 3, kCFNumberSInt64Type = 4, kCFNumberFloat32Type = 5, kCFNumberFloat64Type = 6, kCFNumberCharType = 7, kCFNumberShortType = 8, kCFNumberIntType = 9, kCFNumberLongType = 10, kCFNumberLongLongType = 11, kCFNumberFloatType = 12, kCFNumberDoubleType = 13, kCFNumberCFIndexType = 14, kCFNumberNSIntegerType = 15, kCFNumberCGFloatType = 16, kCFNumberMaxType = 16 }; +typedef const struct __CFAllocator * CFAllocatorRef; +extern const CFAllocatorRef kCFAllocatorDefault; +typedef signed long CFIndex; +typedef CFIndex CFNumberType; +typedef const struct __CFNumber * CFNumberRef; + +extern CFNumberRef CFNumberCreate(CFAllocatorRef allocator, CFNumberType theType, const void *valuePtr); + +void rdar8331641(int x) { + signed z = 1; + CFNumberRef value = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &z); // expected-warning{{leak}} + if (x) + panic(); + (void) value; +} + // CHECK: // CHECK: // CHECK: @@ -750,7 +769,246 @@ void test_null_field(void) { // CHECK: file0 // CHECK: // CHECK: +// CHECK: +// CHECK: path +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line52 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line52 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col23 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col82 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindevent +// CHECK: location +// CHECK: +// CHECK: line53 +// CHECK: col23 +// CHECK: file0 +// CHECK: +// CHECK: ranges +// CHECK: +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col23 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col82 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: extended_message +// CHECK: Call to function 'CFNumberCreate' returns a Core Foundation object with a +1 retain count (owning reference) +// CHECK: message +// CHECK: Call to function 'CFNumberCreate' returns a Core Foundation object with a +1 retain count (owning reference) +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col23 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line53 +// CHECK: col82 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line54 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line54 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line54 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line54 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line56 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line56 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindcontrol +// CHECK: edges +// CHECK: +// CHECK: +// CHECK: start +// CHECK: +// CHECK: +// CHECK: line56 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line56 +// CHECK: col3 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: end +// CHECK: +// CHECK: +// CHECK: line57 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line57 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: +// CHECK: kindevent +// CHECK: location +// CHECK: +// CHECK: line57 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: ranges +// CHECK: +// CHECK: +// CHECK: +// CHECK: line57 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: line57 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: +// CHECK: +// CHECK: extended_message +// CHECK: Object allocated on line 53 and stored into 'value' is no longer referenced after this point and has a retain count of +1 (object leaked) +// CHECK: message +// CHECK: Object allocated on line 53 and stored into 'value' is no longer referenced after this point and has a retain count of +1 (object leaked) +// CHECK: +// CHECK: +// CHECK: descriptionPotential leak of an object allocated on line 53 and stored into 'value' +// CHECK: categoryMemory (Core Foundation/Objective-C) +// CHECK: typeLeak of returned object +// CHECK: location +// CHECK: +// CHECK: line57 +// CHECK: col1 +// CHECK: file0 +// CHECK: +// CHECK: // CHECK: // CHECK: // CHECK: -