[cfe-commits] r166941 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h lib/StaticAnalyzer/Core/AnalyzerOptions.cpp lib/StaticAnalyzer/Core/BugReporterVisitors.cpp lib/StaticAnalyzer/Core/PathDiagnostic.cpp test/Analysis/inlining/false-positive-suppression.c
Jordan Rose
jordan_rose at apple.com
Mon Oct 29 10:31:59 PDT 2012
Author: jrose
Date: Mon Oct 29 12:31:59 2012
New Revision: 166941
URL: http://llvm.org/viewvc/llvm-project?rev=166941&view=rev
Log:
[analyzer] New option to not suppress null return paths if an argument is null.
Our one basic suppression heuristic is to assume that functions do not
usually return NULL. However, when one of the arguments is NULL it is
suddenly much more likely that NULL is a valid return value. In this case,
we don't suppress the report here, but we do attach /another/ visitor to
go find out if this NULL argument also comes from an inlined function's
error path.
This new behavior, controlled by the 'avoid-suppressing-null-argument-paths'
analyzer-config option, is turned off by default. Turning it on produced
two false positives and no new true positives when running over LLVM/Clang.
This is one of the possible refinements to our suppression heuristics.
<rdar://problem/12350829>
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
cfe/trunk/test/Analysis/inlining/false-positive-suppression.c
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h?rev=166941&r1=166940&r2=166941&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h Mon Oct 29 12:31:59 2012
@@ -187,6 +187,9 @@
/// \sa shouldPruneNullReturnPaths
llvm::Optional<bool> PruneNullReturnPaths;
+ /// \sa shouldAvoidSuppressingNullArgumentPaths
+ llvm::Optional<bool> AvoidSuppressingNullArgumentPaths;
+
/// \sa getGraphTrimInterval
llvm::Optional<unsigned> GraphTrimInterval;
@@ -245,6 +248,17 @@
/// which accepts the values "true" and "false".
bool shouldPruneNullReturnPaths();
+ /// Returns whether a bug report should \em not be suppressed if its path
+ /// includes a call with a null argument, even if that call has a null return.
+ ///
+ /// This option has no effect when #shouldPruneNullReturnPaths() is false.
+ ///
+ /// This is a counter-heuristic to avoid false negatives.
+ ///
+ /// This is controlled by the 'avoid-suppressing-null-argument-paths' config
+ /// option, which accepts the values "true" and "false".
+ bool shouldAvoidSuppressingNullArgumentPaths();
+
// Returns the size of the functions (in basic blocks), which should be
// considered to be small enough to always inline.
//
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h?rev=166941&r1=166940&r2=166941&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h Mon Oct 29 12:31:59 2012
@@ -268,7 +268,11 @@
/// \param IsArg Whether the statement is an argument to an inlined function.
/// If this is the case, \p N \em must be the CallEnter node for
/// the function.
-void trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &R,
+///
+/// \return Whether or not the function was able to add visitors for this
+/// statement. Note that returning \c true does not actually imply
+/// that any visitors were added.
+bool trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &R,
bool IsArg = false);
const Stmt *GetDerefExpr(const ExplodedNode *N);
Modified: cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp?rev=166941&r1=166940&r2=166941&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp Mon Oct 29 12:31:59 2012
@@ -102,6 +102,12 @@
/* Default = */ true);
}
+bool AnalyzerOptions::shouldAvoidSuppressingNullArgumentPaths() {
+ return getBooleanOption(AvoidSuppressingNullArgumentPaths,
+ "avoid-suppressing-null-argument-paths",
+ /* Default = */ false);
+}
+
int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal) {
llvm::SmallString<10> StrBuf;
llvm::raw_svector_ostream OS(StrBuf);
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=166941&r1=166940&r2=166941&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Mon Oct 29 12:31:59 2012
@@ -135,10 +135,15 @@
/// interesting value comes from an inlined function call.
class ReturnVisitor : public BugReporterVisitorImpl<ReturnVisitor> {
const StackFrameContext *StackFrame;
- bool Satisfied;
+ enum {
+ Initial,
+ MaybeSuppress,
+ Satisfied
+ } Mode;
+
public:
ReturnVisitor(const StackFrameContext *Frame)
- : StackFrame(Frame), Satisfied(false) {}
+ : StackFrame(Frame), Mode(Initial) {}
static void *getTag() {
static int Tag = 0;
@@ -190,13 +195,16 @@
}
}
- PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
- const ExplodedNode *PrevN,
- BugReporterContext &BRC,
- BugReport &BR) {
- if (Satisfied)
- return 0;
+ /// Returns true if any counter-suppression heuristics are enabled for
+ /// ReturnVisitor.
+ static bool hasCounterSuppression(AnalyzerOptions &Options) {
+ return Options.shouldAvoidSuppressingNullArgumentPaths();
+ }
+ PathDiagnosticPiece *visitNodeInitial(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) {
// Only print a message at the interesting return statement.
if (N->getLocationContext() != StackFrame)
return 0;
@@ -217,7 +225,7 @@
return 0;
// Don't print any more notes after this one.
- Satisfied = true;
+ Mode = Satisfied;
const Expr *RetE = Ret->getRetValue();
assert(RetE && "Tracking a return value for a void function");
@@ -243,8 +251,13 @@
// report invalid. We still want to emit a path note, however, in case
// the report is resurrected as valid later on.
ExprEngine &Eng = BRC.getBugReporter().getEngine();
- if (Eng.getAnalysisManager().options.shouldPruneNullReturnPaths())
- BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+ AnalyzerOptions &Options = Eng.getAnalysisManager().options;
+ if (Options.shouldPruneNullReturnPaths()) {
+ if (hasCounterSuppression(Options))
+ Mode = MaybeSuppress;
+ else
+ BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+ }
if (RetE->getType()->isObjCObjectPointerType())
Out << "Returning nil";
@@ -262,6 +275,74 @@
PathDiagnosticLocation L(Ret, BRC.getSourceManager(), StackFrame);
return new PathDiagnosticEventPiece(L, Out.str());
}
+
+ PathDiagnosticPiece *visitNodeMaybeSuppress(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) {
+ // Are we at the entry node for this call?
+ const CallEnter *CE = N->getLocationAs<CallEnter>();
+ if (!CE)
+ return 0;
+
+ if (CE->getCalleeContext() != StackFrame)
+ return 0;
+
+ Mode = Satisfied;
+
+ ExprEngine &Eng = BRC.getBugReporter().getEngine();
+ AnalyzerOptions &Options = Eng.getAnalysisManager().options;
+ if (Options.shouldAvoidSuppressingNullArgumentPaths()) {
+ // Don't automatically suppress a report if one of the arguments is
+ // known to be a null pointer. Instead, start tracking /that/ null
+ // value back to its origin.
+ ProgramStateManager &StateMgr = BRC.getStateManager();
+ CallEventManager &CallMgr = StateMgr.getCallEventManager();
+
+ ProgramStateRef State = N->getState();
+ CallEventRef<> Call = CallMgr.getCaller(StackFrame, State);
+ for (unsigned I = 0, E = Call->getNumArgs(); I != E; ++I) {
+ SVal ArgV = Call->getArgSVal(I);
+ if (!isa<Loc>(ArgV))
+ continue;
+
+ const Expr *ArgE = Call->getArgExpr(I);
+ if (!ArgE)
+ continue;
+
+ // Is it possible for this argument to be non-null?
+ if (State->assume(cast<Loc>(ArgV), true))
+ continue;
+
+ if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true))
+ return 0;
+
+ // If we /can't/ track the null pointer, we should err on the side of
+ // false negatives, and continue towards marking this report invalid.
+ // (We will still look at the other arguments, though.)
+ }
+ }
+
+ // There is no reason not to suppress this report; go ahead and do it.
+ BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+ return 0;
+ }
+
+ PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext &BRC,
+ BugReport &BR) {
+ switch (Mode) {
+ case Initial:
+ return visitNodeInitial(N, PrevN, BRC, BR);
+ case MaybeSuppress:
+ return visitNodeMaybeSuppress(N, PrevN, BRC, BR);
+ case Satisfied:
+ return 0;
+ }
+
+ llvm_unreachable("Invalid visit mode!");
+ }
};
} // end anonymous namespace
@@ -525,10 +606,10 @@
return NULL;
}
-void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
+bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S,
BugReport &report, bool IsArg) {
if (!S || !N)
- return;
+ return false;
if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(S))
S = OVE->getSourceExpr();
@@ -550,7 +631,7 @@
} while (N);
if (!N)
- return;
+ return false;
}
ProgramStateRef state = N->getState();
@@ -600,7 +681,7 @@
}
report.addVisitor(new FindLastStoreBRVisitor(V, R));
- return;
+ return true;
}
}
}
@@ -634,6 +715,8 @@
S = E->IgnoreParenCasts();
ReturnVisitor::addVisitorIfNecessary(N, S, report);
}
+
+ return true;
}
BugReporterVisitor *
Modified: cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp?rev=166941&r1=166940&r2=166941&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp Mon Oct 29 12:31:59 2012
@@ -586,8 +586,8 @@
const CFGBlock *BSrc = BE->getSrc();
S = BSrc->getTerminatorCondition();
}
- else if (const PostStmt *PS = dyn_cast<PostStmt>(&P)) {
- S = PS->getStmt();
+ else if (const StmtPoint *SP = dyn_cast<StmtPoint>(&P)) {
+ S = SP->getStmt();
}
else if (const PostImplicitCall *PIE = dyn_cast<PostImplicitCall>(&P)) {
return PathDiagnosticLocation(PIE->getLocation(), SMng);
@@ -602,6 +602,9 @@
CEE->getLocationContext(),
SMng);
}
+ else {
+ llvm_unreachable("Unexpected ProgramPoint");
+ }
return PathDiagnosticLocation(S, SMng, P.getLocationContext());
}
Modified: cfe/trunk/test/Analysis/inlining/false-positive-suppression.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/false-positive-suppression.c?rev=166941&r1=166940&r2=166941&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/inlining/false-positive-suppression.c (original)
+++ cfe/trunk/test/Analysis/inlining/false-positive-suppression.c Mon Oct 29 12:31:59 2012
@@ -1,9 +1,14 @@
// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-config suppress-null-return-paths=false -verify %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify -DSUPPRESSED %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify -DSUPPRESSED=1 %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-config avoid-suppressing-null-argument-paths=true -DSUPPRESSED=1 -DNULL_ARGS=1 -verify %s
int opaquePropertyCheck(void *object);
int coin();
+int *getNull() {
+ return 0;
+}
+
int *dynCastToInt(void *ptr) {
if (opaquePropertyCheck(ptr))
return (int *)ptr;
@@ -69,24 +74,108 @@
}
+// --------------------------
+// "Suppression suppression"
+// --------------------------
+
+void testDynCastOrNullOfNull() {
+ // Don't suppress when one of the arguments is NULL.
+ int *casted = dynCastOrNull(0);
+ *casted = 1;
+#if !SUPPRESSED || NULL_ARGS
+ // expected-warning at -2 {{Dereference of null pointer}}
+#endif
+}
+
+void testDynCastOfNull() {
+ // Don't suppress when one of the arguments is NULL.
+ int *casted = dynCastToInt(0);
+ *casted = 1;
+#if !SUPPRESSED || NULL_ARGS
+ // expected-warning at -2 {{Dereference of null pointer}}
+#endif
+}
+
+int *lookUpInt(int unused) {
+ if (coin())
+ return 0;
+ static int x;
+ return &x;
+}
+
+void testZeroIsNotNull() {
+ // /Do/ suppress when the argument is 0 (an integer).
+ int *casted = lookUpInt(0);
+ *casted = 1;
+#ifndef SUPPRESSED
+ // expected-warning at -2 {{Dereference of null pointer}}
+#endif
+}
+
+void testTrackNull() {
+ // /Do/ suppress if the null argument came from another call returning null.
+ int *casted = dynCastOrNull(getNull());
+ *casted = 1;
+#ifndef SUPPRESSED
+ // expected-warning at -2 {{Dereference of null pointer}}
+#endif
+}
+
+void testTrackNullVariable() {
+ // /Do/ suppress if the null argument came from another call returning null.
+ int *ptr;
+ ptr = getNull();
+ int *casted = dynCastOrNull(ptr);
+ *casted = 1;
+#ifndef SUPPRESSED
+ // expected-warning at -2 {{Dereference of null pointer}}
+#endif
+}
+
+
// ---------------------------------------
// FALSE NEGATIVES (over-suppression)
// ---------------------------------------
-void testDynCastOrNullOfNull() {
+void testNoArguments() {
+ // In this case the function has no branches, and MUST return null.
+ int *casted = getNull();
+ *casted = 1;
+#ifndef SUPPRESSED
+ // expected-warning at -2 {{Dereference of null pointer}}
+#endif
+}
+
+int *getNullIfNonNull(void *input) {
+ if (input)
+ return 0;
+ static int x;
+ return &x;
+}
+
+void testKnownPath(void *input) {
+ if (!input)
+ return;
+
// In this case we have a known value for the argument, and thus the path
// through the function doesn't ever split.
- int *casted = dynCastOrNull(0);
+ int *casted = getNullIfNonNull(input);
*casted = 1;
#ifndef SUPPRESSED
// expected-warning at -2 {{Dereference of null pointer}}
#endif
}
-void testDynCastOfNull() {
+int *alwaysReturnNull(void *input) {
+ if (opaquePropertyCheck(input))
+ return 0;
+ return 0;
+}
+
+void testAlwaysReturnNull(void *input) {
// In this case all paths out of the function return 0, but they are all
// dominated by a branch whose condition we don't know!
- int *casted = dynCastToInt(0);
+ int *casted = alwaysReturnNull(input);
*casted = 1;
#ifndef SUPPRESSED
// expected-warning at -2 {{Dereference of null pointer}}
More information about the cfe-commits
mailing list