From 2672a4cc0c34c7b231d39302d924894d7a59ab04 Mon Sep 17 00:00:00 2001 From: Anna Zaks Date: Thu, 14 Mar 2013 22:31:56 +0000 Subject: [PATCH] [analyzer] Change the way in which IDC Visitor decides to kick in and make sure it attaches in the given edge case MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit In the test case below, the value V is not constrained to 0 in ErrorNode but it is in node N. So we used to fail to register the Suppression visitor. We also need to change the way we determine that the Visitor should kick in because the node N belongs to the ExplodedGraph and might not be on the BugReporter path that the visitor sees. Instead of trying to match the node, turn on the visitor when we see the last node in which the symbol is ‘0’. llvm-svn: 177121 --- .../Core/BugReporter/BugReporterVisitor.h | 12 +++---- .../StaticAnalyzer/Core/BugReporterVisitors.cpp | 38 ++++++++-------------- .../Analysis/inlining/inline-defensive-checks.cpp | 25 +++++++++++++- 3 files changed, 42 insertions(+), 33 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h index 17c1009..aad2ab1 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h @@ -290,12 +290,12 @@ class SuppressInlineDefensiveChecksVisitor /// Track if we found the node where the constraint was first added. bool IsSatisfied; - /// \brief The node from which we should start tracking the value. - /// Note: Since the visitors can be registered on nodes previous to the last + /// Since the visitors can be registered on nodes previous to the last /// node in the BugReport, but the path traversal always starts with the last /// node, the visitor invariant (that we start with a node in which V is null) - /// might not hold when node visitation starts. - const ExplodedNode *StartN; + /// might not hold when node visitation starts. We are going to start tracking + /// from the last node in which the value is null. + bool IsTrackingTurnedOn; public: SuppressInlineDefensiveChecksVisitor(DefinedSVal Val, const ExplodedNode *N); @@ -306,10 +306,6 @@ public: /// to make all PathDiagnosticPieces created by this visitor. static const char *getTag(); - PathDiagnosticPiece *getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR); - PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ, const ExplodedNode *Pred, BugReporterContext &BRC, diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 56d6d26..22c148b 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -700,16 +700,14 @@ TrackConstraintBRVisitor::VisitNode(const ExplodedNode *N, SuppressInlineDefensiveChecksVisitor:: SuppressInlineDefensiveChecksVisitor(DefinedSVal Value, const ExplodedNode *N) - : V(Value), IsSatisfied(false), StartN(N) { - - assert(N->getState()->isNull(V).isConstrainedTrue() && - "The visitor only tracks the cases where V is constrained to 0"); + : V(Value), IsSatisfied(false), IsTrackingTurnedOn(false) { + assert(N->getState()->isNull(V).isConstrainedTrue() && + "The visitor only tracks the cases where V is constrained to 0"); } void SuppressInlineDefensiveChecksVisitor::Profile(FoldingSetNodeID &ID) const { static int id = 0; ID.AddPointer(&id); - ID.AddPointer(StartN); ID.Add(V); } @@ -718,33 +716,25 @@ const char *SuppressInlineDefensiveChecksVisitor::getTag() { } PathDiagnosticPiece * -SuppressInlineDefensiveChecksVisitor::getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) { - if (StartN == BR.getErrorNode()) - StartN = 0; - return 0; -} - -PathDiagnosticPiece * SuppressInlineDefensiveChecksVisitor::VisitNode(const ExplodedNode *Succ, const ExplodedNode *Pred, BugReporterContext &BRC, BugReport &BR) { if (IsSatisfied) return 0; - - // Start tracking after we see node StartN. - if (StartN == Succ) - StartN = 0; - if (StartN) - return 0; - AnalyzerOptions &Options = - BRC.getBugReporter().getEngine().getAnalysisManager().options; + BRC.getBugReporter().getEngine().getAnalysisManager().options; if (!Options.shouldSuppressInlinedDefensiveChecks()) return 0; + // Start tracking after we see the first state in which the value is null. + if (!IsTrackingTurnedOn) + if (Succ->getState()->isNull(V).isConstrainedTrue()) + IsTrackingTurnedOn = true; + if (!IsTrackingTurnedOn) + return 0; + + // Check if in the previous state it was feasible for this value // to *not* be null. if (Pred->getState()->assume(V, true)) { @@ -899,10 +889,10 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *ErrorNode, report.addVisitor(ConstraintTracker); // Add visitor, which will suppress inline defensive checks. - if (ErrorNode->getState()->isNull(V).isConstrainedTrue()) { + if (N->getState()->isNull(V).isConstrainedTrue()) { BugReporterVisitor *IDCSuppressor = new SuppressInlineDefensiveChecksVisitor(V.castAs(), - ErrorNode); + N); report.addVisitor(IDCSuppressor); } } diff --git a/clang/test/Analysis/inlining/inline-defensive-checks.cpp b/clang/test/Analysis/inlining/inline-defensive-checks.cpp index c77961d..37bccbd 100644 --- a/clang/test/Analysis/inlining/inline-defensive-checks.cpp +++ b/clang/test/Analysis/inlining/inline-defensive-checks.cpp @@ -1,6 +1,12 @@ // RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s // expected-no-diagnostics +extern void __assert_fail (__const char *__assertion, __const char *__file, + unsigned int __line, __const char *__function) +__attribute__ ((__noreturn__)); +#define assert(expr) \ +((expr) ? (void)(0) : __assert_fail (#expr, __FILE__, __LINE__, __func__)) + class ButterFly { private: ButterFly() { } @@ -29,4 +35,21 @@ class X{ subtest1(); return subtest2(); } -}; \ No newline at end of file +}; + +typedef const int *Ty; +extern +Ty notNullArg(Ty cf) __attribute__((nonnull)); +typedef const void *CFTypeRef; +extern Ty getTyVal(); +inline void radar13224271_callee(Ty def, Ty& result ) { + result = def; + // Clearly indicates that result cannot be 0 if def is not NULL. + assert( (result != 0) || (def == 0) ); +} +void radar13224271_caller() +{ + Ty value; + radar13224271_callee(getTyVal(), value ); + notNullArg(value); // no-warning +} \ No newline at end of file -- 2.7.4