From fe6a011a113b3ddcb32f42af152d7476054e7f79 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Mon, 2 Jul 2012 19:28:21 +0000 Subject: [PATCH] [analyzer] Convert existing checkers to use check::preCall and check::postCall. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@159563 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/AttrNonNullChecker.cpp | 47 ++++---- .../Checkers/CallAndMessageChecker.cpp | 101 ++++++++---------- .../Checkers/ObjCSelfInitChecker.cpp | 86 ++++----------- .../Checkers/RetainCountChecker.cpp | 68 ++---------- test/Analysis/misc-ps-region-store.m | 2 +- test/Analysis/nonnull.m | 23 ++++ 6 files changed, 125 insertions(+), 202 deletions(-) create mode 100644 test/Analysis/nonnull.m diff --git a/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp b/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp index ab66e9864f..27faf18d4c 100644 --- a/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp @@ -15,6 +15,7 @@ #include "ClangSACheckers.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/Calls.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" @@ -23,40 +24,32 @@ using namespace ento; namespace { class AttrNonNullChecker - : public Checker< check::PreStmt > { + : public Checker< check::PreCall > { mutable OwningPtr BT; public: - void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; }; } // end anonymous namespace -void AttrNonNullChecker::checkPreStmt(const CallExpr *CE, +void AttrNonNullChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef state = C.getState(); - const LocationContext *LCtx = C.getLocationContext(); - - // Check if the callee has a 'nonnull' attribute. - SVal X = state->getSVal(CE->getCallee(), LCtx); - - const FunctionDecl *FD = X.getAsFunctionDecl(); + const Decl *FD = Call.getDecl(); if (!FD) return; - const NonNullAttr* Att = FD->getAttr(); + const NonNullAttr *Att = FD->getAttr(); if (!Att) return; - // Iterate through the arguments of CE and check them for null. - unsigned idx = 0; - - for (CallExpr::const_arg_iterator I=CE->arg_begin(), E=CE->arg_end(); I!=E; - ++I, ++idx) { + ProgramStateRef state = C.getState(); + // Iterate through the arguments of CE and check them for null. + for (unsigned idx = 0, count = Call.getNumArgs(); idx != count; ++idx) { if (!Att->isNonNull(idx)) continue; - SVal V = state->getSVal(*I, LCtx); + SVal V = Call.getArgSVal(idx); DefinedSVal *DV = dyn_cast(&V); // If the value is unknown or undefined, we can't perform this check. @@ -65,11 +58,16 @@ void AttrNonNullChecker::checkPreStmt(const CallExpr *CE, if (!isa(*DV)) { // If the argument is a union type, we want to handle a potential - // transparent_unoin GCC extension. - QualType T = (*I)->getType(); + // transparent_union GCC extension. + const Expr *ArgE = Call.getArgExpr(idx); + if (!ArgE) + continue; + + QualType T = ArgE->getType(); const RecordType *UT = T->getAsUnionType(); if (!UT || !UT->getDecl()->hasAttr()) continue; + if (nonloc::CompoundVal *CSV = dyn_cast(DV)) { nonloc::CompoundVal::iterator CSV_I = CSV->begin(); assert(CSV_I != CSV->end()); @@ -78,8 +76,7 @@ void AttrNonNullChecker::checkPreStmt(const CallExpr *CE, assert(++CSV_I == CSV->end()); if (!DV) continue; - } - else { + } else { // FIXME: Handle LazyCompoundVals? continue; } @@ -106,10 +103,10 @@ void AttrNonNullChecker::checkPreStmt(const CallExpr *CE, "'nonnull' parameter", errorNode); // Highlight the range of the argument that was null. - const Expr *arg = *I; - R->addRange(arg->getSourceRange()); - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(errorNode, - arg, R)); + R->addRange(Call.getArgSourceRange(idx)); + if (const Expr *ArgE = Call.getArgExpr(idx)) + R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(errorNode, + ArgE, R)); // Emit the bug report. C.EmitReport(R); } diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 083f21ea3f..2f95709e03 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -27,7 +27,8 @@ using namespace ento; namespace { class CallAndMessageChecker - : public Checker< check::PreStmt, check::PreObjCMessage > { + : public Checker< check::PreStmt, check::PreObjCMessage, + check::PreCall > { mutable OwningPtr BT_call_null; mutable OwningPtr BT_call_undef; mutable OwningPtr BT_call_arg; @@ -39,15 +40,13 @@ public: void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; private: - static void PreVisitProcessArgs(CheckerContext &C, const CallEvent &Call, - const char *BT_desc, OwningPtr &BT); - static bool PreVisitProcessArg(CheckerContext &C, SVal V,SourceRange argRange, - const Expr *argEx, + static bool PreVisitProcessArg(CheckerContext &C, SVal V, + SourceRange argRange, const Expr *argEx, const bool checkUninitFields, - const char *BT_desc, - OwningPtr &BT); + const char *BT_desc, OwningPtr &BT); static void EmitBadCall(BugType *BT, CheckerContext &C, const CallExpr *CE); void emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg, @@ -76,26 +75,6 @@ void CallAndMessageChecker::EmitBadCall(BugType *BT, CheckerContext &C, C.EmitReport(R); } -void CallAndMessageChecker::PreVisitProcessArgs(CheckerContext &C, - const CallEvent &Call, - const char *BT_desc, - OwningPtr &BT) { - // Don't check for uninitialized field values in arguments if the - // caller has a body that is available and we have the chance to inline it. - // This is a hack, but is a reasonable compromise betweens sometimes warning - // and sometimes not depending on if we decide to inline a function. - const Decl *D = Call.getDecl(); - const bool checkUninitFields = - !(C.getAnalysisManager().shouldInlineCall() && - (D && D->getBody())); - - for (unsigned i = 0, e = Call.getNumArgs(); i != e; ++i) - if (PreVisitProcessArg(C, Call.getArgSVal(i), - Call.getArgSourceRange(i), Call.getArgExpr(i), - checkUninitFields, BT_desc, BT)) - return; -} - bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, SVal V, SourceRange argRange, const Expr *argEx, @@ -104,10 +83,10 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, OwningPtr &BT) { if (V.isUndef()) { if (ExplodedNode *N = C.generateSink()) { - LazyInit_BT(BT_desc, BT); + LazyInit_BT("Uninitialized argument value", BT); // Generate a report for this bug. - BugReport *R = new BugReport(*BT, BT->getName(), N); + BugReport *R = new BugReport(*BT, BT_desc, N); R->addRange(argRange); if (argEx) R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, argEx, @@ -227,24 +206,47 @@ void CallAndMessageChecker::checkPreStmt(const CallExpr *CE, new BuiltinBug("Called function pointer is null (null dereference)")); EmitBadCall(BT_call_null.get(), C, CE); } +} - // FIXME: This tree of switching can go away if/when we add a check::postCall. - if (dyn_cast_or_null(L.getAsRegion())) { - BlockCall Call(CE, State, LCtx); - PreVisitProcessArgs(C, Call, - "Block call argument is an uninitialized value", - BT_call_arg); - } else if (const CXXMemberCallExpr *me = dyn_cast(CE)) { - CXXMemberCall Call(me, State, LCtx); - PreVisitProcessArgs(C, Call, - "Function call argument is an uninitialized value", - BT_call_arg); - } else { - FunctionCall Call(CE, State, LCtx); - PreVisitProcessArgs(C, Call, - "Function call argument is an uninitialized value", - BT_call_arg); +void CallAndMessageChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + // Don't check for uninitialized field values in arguments if the + // caller has a body that is available and we have the chance to inline it. + // This is a hack, but is a reasonable compromise betweens sometimes warning + // and sometimes not depending on if we decide to inline a function. + const Decl *D = Call.getDecl(); + const bool checkUninitFields = + !(C.getAnalysisManager().shouldInlineCall() && + (D && D->getBody())); + + OwningPtr *BT; + const char *Desc; + + switch (Call.getKind()) { + case CE_ObjCPropertyAccess: + BT = &BT_msg_arg; + // Getters do not have arguments, so we don't need to worry about this. + Desc = "Argument for property setter is an uninitialized value"; + break; + case CE_ObjCMessage: + BT = &BT_msg_arg; + Desc = "Argument in message expression is an uninitialized value"; + break; + case CE_Block: + BT = &BT_call_arg; + Desc = "Block call argument is an uninitialized value"; + break; + default: + BT = &BT_call_arg; + Desc = "Function call argument is an uninitialized value"; + break; } + + for (unsigned i = 0, e = Call.getNumArgs(); i != e; ++i) + if (PreVisitProcessArg(C, Call.getArgSVal(i), + Call.getArgSourceRange(i), Call.getArgExpr(i), + checkUninitFields, Desc, *BT)) + return; } void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg, @@ -289,15 +291,6 @@ void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg, return; } } - - const char *bugDesc = "Argument in message expression is an " - "uninitialized value"; - if (const ObjCPropertyAccess *Prop = dyn_cast(&msg)) - if (Prop->isSetter()) - bugDesc = "Argument for property setter is an uninitialized value"; - - // Check for any arguments that are uninitialized/undefined. - PreVisitProcessArgs(C, msg, bugDesc, BT_msg_arg); } void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C, diff --git a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp index 40eb381999..2bd3e9a0e0 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp @@ -54,27 +54,23 @@ static bool isInitMessage(const ObjCMethodCall &Msg); static bool isSelfVar(SVal location, CheckerContext &C); namespace { -class ObjCSelfInitChecker : public Checker< check::PreObjCMessage, - check::PostObjCMessage, +class ObjCSelfInitChecker : public Checker< check::PostObjCMessage, check::PostStmt, check::PreStmt, - check::PreStmt, - check::PostStmt, + check::PreCall, + check::PostCall, check::Location, check::Bind > { public: - void checkPreObjCMessage(const ObjCMethodCall &Msg, CheckerContext &C) const; void checkPostObjCMessage(const ObjCMethodCall &Msg, CheckerContext &C) const; void checkPostStmt(const ObjCIvarRefExpr *E, CheckerContext &C) const; void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const; - void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; - void checkPostStmt(const CallExpr *CE, CheckerContext &C) const; void checkLocation(SVal location, bool isLoad, const Stmt *S, CheckerContext &C) const; void checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const; - void checkPreStmt(const CallEvent &CE, CheckerContext &C) const; - void checkPostStmt(const CallEvent &CE, CheckerContext &C) const; + void checkPreCall(const CallEvent &CE, CheckerContext &C) const; + void checkPostCall(const CallEvent &CE, CheckerContext &C) const; }; } // end anonymous namespace @@ -208,8 +204,6 @@ void ObjCSelfInitChecker::checkPostObjCMessage(const ObjCMethodCall &Msg, return; } - checkPostStmt(Msg, C); - // We don't check for an invalid 'self' in an obj-c message expression to cut // down false positives where logging functions get information from self // (like its class) or doing "invalidation" on self when the initialization @@ -240,8 +234,8 @@ void ObjCSelfInitChecker::checkPreStmt(const ReturnStmt *S, "'[(super or self) init...]'"); } -// When a call receives a reference to 'self', [Pre/Post]VisitGenericCall pass -// the SelfFlags from the object 'self' point to before the call, to the new +// When a call receives a reference to 'self', [Pre/Post]Call pass +// the SelfFlags from the object 'self' points to before the call to the new // object after the call. This is to avoid invalidation of 'self' by logging // functions. // Another common pattern in classes with multiple initializers is to put the @@ -256,53 +250,13 @@ void ObjCSelfInitChecker::checkPreStmt(const ReturnStmt *S, // Until we can use inter-procedural analysis, in such a call, transfer the // SelfFlags to the result of the call. -void ObjCSelfInitChecker::checkPreStmt(const CallExpr *CE, +void ObjCSelfInitChecker::checkPreCall(const CallEvent &CE, CheckerContext &C) const { - // FIXME: This tree of switching can go away if/when we add a check::postCall. - const Expr *Callee = CE->getCallee()->IgnoreParens(); - ProgramStateRef State = C.getState(); - const LocationContext *LCtx = C.getLocationContext(); - SVal L = State->getSVal(Callee, LCtx); - - if (dyn_cast_or_null(L.getAsRegion())) { - BlockCall Call(CE, State, LCtx); - checkPreStmt(Call, C); - } else if (const CXXMemberCallExpr *me = dyn_cast(CE)) { - CXXMemberCall Call(me, State, LCtx); - checkPreStmt(Call, C); - } else { - FunctionCall Call(CE, State, LCtx); - checkPreStmt(Call, C); - } -} - -void ObjCSelfInitChecker::checkPostStmt(const CallExpr *CE, - CheckerContext &C) const { - // FIXME: This tree of switching can go away if/when we add a check::postCall. - const Expr *Callee = CE->getCallee()->IgnoreParens(); - ProgramStateRef State = C.getState(); - const LocationContext *LCtx = C.getLocationContext(); - SVal L = State->getSVal(Callee, LCtx); - - if (dyn_cast_or_null(L.getAsRegion())) { - BlockCall Call(CE, State, LCtx); - checkPostStmt(Call, C); - } else if (const CXXMemberCallExpr *me = dyn_cast(CE)) { - CXXMemberCall Call(me, State, LCtx); - checkPostStmt(Call, C); - } else { - FunctionCall Call(CE, State, LCtx); - checkPostStmt(Call, C); - } -} - -void ObjCSelfInitChecker::checkPreObjCMessage(const ObjCMethodCall &Msg, - CheckerContext &C) const { - checkPreStmt(Msg, C); -} + // FIXME: A callback should disable checkers at the start of functions. + if (!shouldRunOnFunctionOrMethod(dyn_cast( + C.getCurrentAnalysisDeclContext()->getDecl()))) + return; -void ObjCSelfInitChecker::checkPreStmt(const CallEvent &CE, - CheckerContext &C) const { ProgramStateRef state = C.getState(); unsigned NumArgs = CE.getNumArgs(); // If we passed 'self' as and argument to the call, record it in the state @@ -324,9 +278,19 @@ void ObjCSelfInitChecker::checkPreStmt(const CallEvent &CE, } } -void ObjCSelfInitChecker::checkPostStmt(const CallEvent &CE, +void ObjCSelfInitChecker::checkPostCall(const CallEvent &CE, CheckerContext &C) const { + // FIXME: A callback should disable checkers at the start of functions. + if (!shouldRunOnFunctionOrMethod(dyn_cast( + C.getCurrentAnalysisDeclContext()->getDecl()))) + return; + ProgramStateRef state = C.getState(); + SelfFlagEnum prevFlags = (SelfFlagEnum)state->get(); + if (!prevFlags) + return; + state = state->remove(); + unsigned NumArgs = CE.getNumArgs(); for (unsigned i = 0; i < NumArgs; ++i) { SVal argV = CE.getArgSVal(i); @@ -334,8 +298,6 @@ void ObjCSelfInitChecker::checkPostStmt(const CallEvent &CE, // If the address of 'self' is being passed to the call, assume that the // 'self' after the call will have the same flags. // EX: log(&self) - SelfFlagEnum prevFlags = (SelfFlagEnum)state->get(); - state = state->remove(); addSelfFlag(state, state->getSVal(cast(argV)), prevFlags, C); return; } else if (hasSelfFlag(argV, SelfFlag_Self, C)) { @@ -343,8 +305,6 @@ void ObjCSelfInitChecker::checkPostStmt(const CallEvent &CE, // returns 'self'. So assign the flags, which were set on 'self' to the // return value. // EX: self = performMoreInitialization(self) - SelfFlagEnum prevFlags = (SelfFlagEnum)state->get(); - state = state->remove(); const Expr *CallExpr = CE.getOriginExpr(); if (CallExpr) addSelfFlag(state, state->getSVal(CallExpr, C.getLocationContext()), diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index 80ebaa2e69..936bf9cf97 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -2379,12 +2379,10 @@ class RetainCountChecker check::EndPath, check::PostStmt, check::PostStmt, - check::PostStmt, - check::PostStmt, check::PostStmt, check::PostStmt, check::PostStmt, - check::PostObjCMessage, + check::PostCall, check::PreStmt, check::RegionChanges, eval::Assume, @@ -2523,13 +2521,11 @@ public: void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const; void checkPostStmt(const CastExpr *CE, CheckerContext &C) const; - void checkPostStmt(const CallExpr *CE, CheckerContext &C) const; - void checkPostStmt(const CXXConstructExpr *CE, CheckerContext &C) const; void checkPostStmt(const ObjCArrayLiteral *AL, CheckerContext &C) const; void checkPostStmt(const ObjCDictionaryLiteral *DL, CheckerContext &C) const; void checkPostStmt(const ObjCBoxedExpr *BE, CheckerContext &C) const; - void checkPostObjCMessage(const ObjCMethodCall &Msg, CheckerContext &C) const; + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; void checkSummary(const RetainSummary &Summ, const CallEvent &Call, CheckerContext &C) const; @@ -2685,54 +2681,6 @@ void RetainCountChecker::checkPostStmt(const CastExpr *CE, C.addTransition(state); } -void RetainCountChecker::checkPostStmt(const CallExpr *CE, - CheckerContext &C) const { - if (C.wasInlined) - return; - - // Get the callee. - ProgramStateRef state = C.getState(); - const Expr *Callee = CE->getCallee(); - SVal L = state->getSVal(Callee, C.getLocationContext()); - - RetainSummaryManager &Summaries = getSummaryManager(C); - - // FIXME: This tree of switching can go away if/when we add a check::postCall. - if (dyn_cast_or_null(L.getAsRegion())) { - // FIXME: Better support for blocks. For now we stop tracking anything - // that is passed to blocks. - // FIXME: Need to handle variables that are "captured" by the block. - BlockCall Call(CE, state, C.getLocationContext()); - const RetainSummary *Summ = Summaries.getSummary(Call, state); - checkSummary(*Summ, Call, C); - - } else if (const CXXMemberCallExpr *me = dyn_cast(CE)) { - CXXMemberCall Call(me, state, C.getLocationContext()); - const RetainSummary *Summ = Summaries.getSummary(Call, state); - checkSummary(*Summ, Call, C); - - } else { - FunctionCall Call(CE, state, C.getLocationContext()); - const RetainSummary *Summ = Summaries.getSummary(Call, state); - checkSummary(*Summ, Call, C); - } -} - -void RetainCountChecker::checkPostStmt(const CXXConstructExpr *CE, - CheckerContext &C) const { - const CXXConstructorDecl *Ctor = CE->getConstructor(); - if (!Ctor) - return; - - RetainSummaryManager &Summaries = getSummaryManager(C); - ProgramStateRef state = C.getState(); - CXXConstructorCall Call(CE, state, C.getLocationContext()); - - const RetainSummary *Summ = Summaries.getSummary(Call, state); - - checkSummary(*Summ, Call, C); -} - void RetainCountChecker::processObjCLiterals(CheckerContext &C, const Expr *Ex) const { ProgramStateRef state = C.getState(); @@ -2791,12 +2739,14 @@ void RetainCountChecker::checkPostStmt(const ObjCBoxedExpr *Ex, C.addTransition(State); } -void RetainCountChecker::checkPostObjCMessage(const ObjCMethodCall &Msg, - CheckerContext &C) const { - RetainSummaryManager &Summaries = getSummaryManager(C); - const RetainSummary *Summ = Summaries.getSummary(Msg, C.getState()); +void RetainCountChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + if (C.wasInlined) + return; - checkSummary(*Summ, Msg, C); + RetainSummaryManager &Summaries = getSummaryManager(C); + const RetainSummary *Summ = Summaries.getSummary(Call, C.getState()); + checkSummary(*Summ, Call, C); } /// GetReturnType - Used to get the return type of a message expression or diff --git a/test/Analysis/misc-ps-region-store.m b/test/Analysis/misc-ps-region-store.m index 7b9578eb3e..88860bbe10 100644 --- a/test/Analysis/misc-ps-region-store.m +++ b/test/Analysis/misc-ps-region-store.m @@ -673,7 +673,7 @@ typedef void (^RDar_7462324_Callback)(id obj); builder = ^(id object) { id x; if (object) { - builder(x); // expected-warning{{Function call argument is an uninitialized value}} + builder(x); // expected-warning{{Block call argument is an uninitialized value}} } }; builder(target); diff --git a/test/Analysis/nonnull.m b/test/Analysis/nonnull.m new file mode 100644 index 0000000000..c32a7f780e --- /dev/null +++ b/test/Analysis/nonnull.m @@ -0,0 +1,23 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s + +@interface MyObject +- (void)takePointer:(void *)ptr __attribute__((nonnull(1))); +@end + +void testNonNullMethod(int *p, MyObject *obj) { + if (p) + return; + [obj takePointer:p]; // expected-warning{{nonnull}} +} + + +@interface Subclass : MyObject +// [[nonnull]] is an inherited attribute. +- (void)takePointer:(void *)ptr; +@end + +void testSubclass(int *p, Subclass *obj) { + if (p) + return; + [obj takePointer:p]; // expected-warning{{nonnull}} +} -- 2.40.0