From 0e7d25233ecaa9ba618c2154474aaa53c3df616b Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Thu, 3 Jul 2008 04:29:21 +0000 Subject: [PATCH] Added static analysis check to see if a subclass of NSObject implements -dealloc, and whether or not that implementation calls [super dealloc]. llvm-svn: 53075 --- clang/Driver/AnalysisConsumer.cpp | 44 +++++-- clang/include/clang/Analysis/LocalCheckers.h | 4 + .../Analysis/PathSensitive/BugReporter.h | 10 +- clang/lib/Analysis/CheckObjCDealloc.cpp | 118 ++++++++++++++++++ clang/lib/Analysis/DeadStores.cpp | 17 --- clang/test/Analysis/NSPanel.m | 3 +- clang/test/Analysis/NSString.m | 4 +- 7 files changed, 170 insertions(+), 30 deletions(-) create mode 100644 clang/lib/Analysis/CheckObjCDealloc.cpp diff --git a/clang/Driver/AnalysisConsumer.cpp b/clang/Driver/AnalysisConsumer.cpp index f14fd1dd6650..c557b80c7ee8 100644 --- a/clang/Driver/AnalysisConsumer.cpp +++ b/clang/Driver/AnalysisConsumer.cpp @@ -17,7 +17,6 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclObjC.h" #include "llvm/Support/Compiler.h" -#include "llvm/ADT/ImmutableList.h" #include "llvm/ADT/OwningPtr.h" #include "clang/AST/CFG.h" #include "clang/Analysis/Analyses/LiveVariables.h" @@ -25,6 +24,7 @@ #include "clang/Basic/SourceManager.h" #include "clang/Basic/FileManager.h" #include "clang/AST/ParentMap.h" +#include "clang/AST/TranslationUnit.h" #include "clang/Analysis/PathSensitive/BugReporter.h" #include "clang/Analysis/Analyses/LiveVariables.h" #include "clang/Analysis/LocalCheckers.h" @@ -32,6 +32,8 @@ #include "clang/Analysis/PathSensitive/GRExprEngine.h" #include "llvm/Support/Streams.h" +#include + using namespace clang; @@ -53,11 +55,10 @@ namespace { namespace { class VISIBILITY_HIDDEN AnalysisConsumer : public ASTConsumer { - typedef llvm::ImmutableList Actions; + typedef std::vector Actions; Actions FunctionActions; Actions ObjCMethodActions; - - Actions::Factory F; + Actions ObjCImplementationActions; public: const bool Visualize; @@ -78,16 +79,19 @@ namespace { const std::string& fname, const std::string& htmldir, bool visualize, bool trim, bool analyzeAll) - : FunctionActions(F.GetEmptyList()), ObjCMethodActions(F.GetEmptyList()), - Visualize(visualize), TrimGraph(trim), LOpts(lopts), Diags(diags), + : Visualize(visualize), TrimGraph(trim), LOpts(lopts), Diags(diags), Ctx(0), PP(pp), PPF(ppf), HTMLDir(htmldir), FName(fname), AnalyzeAll(analyzeAll) {} void addCodeAction(CodeAction action) { - FunctionActions = F.Concat(action, FunctionActions); - ObjCMethodActions = F.Concat(action, ObjCMethodActions); + FunctionActions.push_back(action); + ObjCMethodActions.push_back(action); + } + + void addObjCImplementationAction(CodeAction action) { + ObjCImplementationActions.push_back(action); } virtual void Initialize(ASTContext &Context) { @@ -95,6 +99,8 @@ namespace { } virtual void HandleTopLevelDecl(Decl *D); + virtual void HandleTranslationUnit(TranslationUnit &TU); + void HandleCode(Decl* D, Stmt* Body, Actions actions); }; @@ -235,6 +241,18 @@ void AnalysisConsumer::HandleTopLevelDecl(Decl *D) { } } +void AnalysisConsumer::HandleTranslationUnit(TranslationUnit& TU) { + + if (ObjCImplementationActions.empty()) + return; + + for (TranslationUnit::iterator I = TU.begin(), E = TU.end(); I!=E; ++I) { + + if (ObjCImplementationDecl* ID = dyn_cast(*I)) + HandleCode(ID, 0, ObjCImplementationActions); + } +} + void AnalysisConsumer::HandleCode(Decl* D, Stmt* Body, Actions actions) { // Don't run the actions if an error has occured with parsing the file. @@ -259,7 +277,7 @@ void AnalysisConsumer::HandleCode(Decl* D, Stmt* Body, Actions actions) { // Dispatch on the actions. for (Actions::iterator I = actions.begin(), E = actions.end(); I != E; ++I) - ((*I).getHead())(mgr); + (*I)(mgr); } //===----------------------------------------------------------------------===// @@ -351,6 +369,11 @@ static void ActionCFGView(AnalysisManager& mgr) { mgr.getCFG().viewCFG(); } +static void ActionCheckObjCDealloc(AnalysisManager& mgr) { + BugReporter BR(mgr); + CheckObjCDealloc(cast(mgr.getCodeDecl()), BR); +} + //===----------------------------------------------------------------------===// // AnalysisConsumer creation. //===----------------------------------------------------------------------===// @@ -401,6 +424,9 @@ ASTConsumer* clang::CreateAnalysisConsumer(Analyses* Beg, Analyses* End, default: break; } + // Checks we always perform: + C->addObjCImplementationAction(&ActionCheckObjCDealloc); + return C.take(); } diff --git a/clang/include/clang/Analysis/LocalCheckers.h b/clang/include/clang/Analysis/LocalCheckers.h index aaba98595423..08959d3cdf7a 100644 --- a/clang/include/clang/Analysis/LocalCheckers.h +++ b/clang/include/clang/Analysis/LocalCheckers.h @@ -28,6 +28,7 @@ class LangOptions; class ParentMap; class LiveVariables; class BugReporter; +class ObjCImplementationDecl; void CheckDeadStores(LiveVariables& L, BugReporter& BR); @@ -39,6 +40,9 @@ GRTransferFuncs* MakeCFRefCountTF(ASTContext& Ctx, bool GCEnabled, bool StandardWarnings, const LangOptions& lopts); +void CheckObjCDealloc(ObjCImplementationDecl* D, BugReporter& BR); + + } // end namespace clang #endif diff --git a/clang/include/clang/Analysis/PathSensitive/BugReporter.h b/clang/include/clang/Analysis/PathSensitive/BugReporter.h index 7674b04ff32a..f608da0c2e5c 100644 --- a/clang/include/clang/Analysis/PathSensitive/BugReporter.h +++ b/clang/include/clang/Analysis/PathSensitive/BugReporter.h @@ -24,7 +24,6 @@ #include #include - namespace clang { class PathDiagnostic; @@ -286,6 +285,15 @@ public: iterator end() { return Reports.end(); } }; +class SimpleBugType : public BugTypeCacheLocation { + const char* name; +public: + SimpleBugType(const char* n) : name(n) {} + + virtual const char* getName() const { return name; } + virtual const char* getDescription() const { return name; } +}; + } // end clang namespace #endif diff --git a/clang/lib/Analysis/CheckObjCDealloc.cpp b/clang/lib/Analysis/CheckObjCDealloc.cpp new file mode 100644 index 000000000000..9a40fa97ec2b --- /dev/null +++ b/clang/lib/Analysis/CheckObjCDealloc.cpp @@ -0,0 +1,118 @@ +//==- CheckObjCDealloc.cpp - Check ObjC -dealloc implementation --*- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines a DeadStores, a flow-sensitive checker that looks for +// stores to variables that are no longer live. +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/LocalCheckers.h" +#include "clang/Analysis/PathDiagnostic.h" +#include "clang/Analysis/PathSensitive/BugReporter.h" +#include "clang/AST/ExprObjC.h" +#include "clang/AST/Expr.h" +#include "clang/AST/DeclObjC.h" +#include + +using namespace clang; + +static bool scan_dealloc(Stmt* S, Selector Dealloc) { + + if (ObjCMessageExpr* ME = dyn_cast(S)) + if (ME->getSelector() == Dealloc) + if (Expr* Receiver = ME->getReceiver()->IgnoreParenCasts()) + if (PreDefinedExpr* E = dyn_cast(Receiver)) + if (E->getIdentType() == PreDefinedExpr::ObjCSuper) + return true; + + // Recurse to children. + + for (Stmt::child_iterator I = S->child_begin(), E= S->child_end(); I!=E; ++I) + if (*I && scan_dealloc(*I, Dealloc)) + return true; + + return false; +} + +void clang::CheckObjCDealloc(ObjCImplementationDecl* D, BugReporter& BR) { + + ASTContext& Ctx = BR.getContext(); + + // Determine if the class subclasses NSObject. + IdentifierInfo* NSObjectII = &Ctx.Idents.get("NSObject"); + ObjCInterfaceDecl* ID = D->getClassInterface(); + + for ( ; ID ; ID = ID->getSuperClass()) + if (ID->getIdentifier() == NSObjectII) + break; + + if (!ID) + return; + + // Get the "dealloc" selector. + IdentifierInfo* II = &Ctx.Idents.get("dealloc"); + Selector S = Ctx.Selectors.getSelector(0, &II); + + ObjCMethodDecl* MD = 0; + + // Scan the instance methods for "dealloc". + for (ObjCImplementationDecl::instmeth_iterator I = D->instmeth_begin(), + E = D->instmeth_end(); I!=E; ++I) { + + if ((*I)->getSelector() == S) { + MD = *I; + break; + } + } + + if (!MD) { // No dealloc found. + + // FIXME: This code should be reduced to three lines if possible (Refactor). + SimpleBugType BT("missing -dealloc"); + DiagCollector C(BT); + + std::ostringstream os; + os << "Objective-C class '" << D->getName() + << "' lacks a 'dealloc' instance method"; + + Diagnostic& Diag = BR.getDiagnostic(); + Diag.Report(&C, + Ctx.getFullLoc(D->getLocStart()), + Diag.getCustomDiagID(Diagnostic::Warning, os.str().c_str()), + NULL, 0, NULL, 0); + + for (DiagCollector::iterator I = C.begin(), E = C.end(); I != E; ++I) + BR.EmitWarning(*I); + + return; + } + + // dealloc found. Scan for missing [super dealloc]. + if (MD->getBody() && !scan_dealloc(MD->getBody(), S)) { + + // FIXME: This code should be reduced to three lines if possible (Refactor). + SimpleBugType BT("missing [super dealloc]"); + DiagCollector C(BT); + + std::ostringstream os; + os << "The 'dealloc' instance method in Objective-C class '" << D->getName() + << "' does not send a 'dealloc' message to it super class" + " (missing [super dealloc])"; + + Diagnostic& Diag = BR.getDiagnostic(); + Diag.Report(&C, + Ctx.getFullLoc(MD->getLocStart()), + Diag.getCustomDiagID(Diagnostic::Warning, os.str().c_str()), + NULL, 0, NULL, 0); + + for (DiagCollector::iterator I = C.begin(), E = C.end(); I != E; ++I) + BR.EmitWarning(*I); + } +} + diff --git a/clang/lib/Analysis/DeadStores.cpp b/clang/lib/Analysis/DeadStores.cpp index e5bf2a3c1914..02198b290667 100644 --- a/clang/lib/Analysis/DeadStores.cpp +++ b/clang/lib/Analysis/DeadStores.cpp @@ -143,23 +143,6 @@ public: } // end anonymous namespace -//===----------------------------------------------------------------------===// -// BugReporter-based invocation of the Dead-Stores checker. -//===----------------------------------------------------------------------===// - -namespace { - -class SimpleBugType : public BugTypeCacheLocation { - const char* name; -public: - SimpleBugType(const char* n) : name(n) {} - - virtual const char* getName() const { - return name; - } -}; -} // end anonymous namespace - //===----------------------------------------------------------------------===// // Driver function to invoke the Dead-Stores checker on a CFG. //===----------------------------------------------------------------------===// diff --git a/clang/test/Analysis/NSPanel.m b/clang/test/Analysis/NSPanel.m index cbd0056ebf62..a24818e19ed8 100644 --- a/clang/test/Analysis/NSPanel.m +++ b/clang/test/Analysis/NSPanel.m @@ -67,7 +67,8 @@ extern NSString *NSWindowDidBecomeKeyNotification; - (void)myMethod; - (void)myMethod2; @end -@implementation MyClass + +@implementation MyClass // no-warning - (void)myMethod { NSPanel *panel = [[NSPanel alloc] initWithContentRect:NSMakeRect(0, 0, 200, 200) styleMask:NSBorderlessWindowMask backing:NSBackingStoreBuffered defer:(BOOL)1]; diff --git a/clang/test/Analysis/NSString.m b/clang/test/Analysis/NSString.m index a85a87aace02..9a0b94ddcaec 100644 --- a/clang/test/Analysis/NSString.m +++ b/clang/test/Analysis/NSString.m @@ -143,7 +143,7 @@ NSString* f10() { - (NSString*) getShared; + (C1*) sharedInstance; @end -@implementation C1 : NSObject {} +@implementation C1 : NSObject {} // expected-warning{{Objective-C class 'C1' lacks a 'dealloc' instance method}} - (NSString*) getShared { static NSString* s = 0; if (!s) s = [[NSString alloc] init]; @@ -161,7 +161,7 @@ NSString* f10() { @interface SharedClass : NSObject + (id)sharedInstance; @end -@implementation SharedClass +@implementation SharedClass // expected-warning {{Objective-C class 'SharedClass' lacks a 'dealloc' instance method}} - (id)_init { if ((self = [super init])) {