From b293c6bb54c036648f95c256e2281f77e38553a7 Mon Sep 17 00:00:00 2001 From: George Karpenkov Date: Fri, 27 Jul 2018 18:26:40 +0000 Subject: [PATCH] [analyzer] Extend NoStoreFuncVisitor to insert a note on IVars The note is added in the following situation: - We are throwing a nullability-related warning on an IVar - The path goes through a method which *could have* (syntactically determined) written into that IVar, but did not rdar://42444460 Differential Revision: https://reviews.llvm.org/D49689 llvm-svn: 338149 --- .../StaticAnalyzer/Core/BugReporterVisitors.cpp | 63 +++++++++++++++++----- .../diagnostics/no-store-func-path-notes.cpp | 19 ++++++- .../diagnostics/no-store-func-path-notes.m | 35 +++++++++++- 3 files changed, 100 insertions(+), 17 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 6213fc0..c87bc68 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -22,6 +22,7 @@ #include "clang/AST/ExprObjC.h" #include "clang/AST/Stmt.h" #include "clang/AST/Type.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Analysis/CFG.h" #include "clang/Analysis/CFGStmtMap.h" @@ -306,15 +307,26 @@ public: CallEventRef<> Call = BRC.getStateManager().getCallEventManager().getCaller(SCtx, State); - const PrintingPolicy &PP = BRC.getASTContext().getPrintingPolicy(); const SourceManager &SM = BRC.getSourceManager(); + + // Region of interest corresponds to an IVar, exiting a method + // which could have written into that IVar, but did not. + if (const auto *MC = dyn_cast(Call)) + if (const auto *IvarR = dyn_cast(RegionOfInterest)) + if (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(), + IvarR->getDecl()) && + !isRegionOfInterestModifiedInFrame(N)) + return notModifiedMemberDiagnostics( + Ctx, SM, PP, *CallExitLoc, Call, + MC->getReceiverSVal().getAsRegion()); + if (const auto *CCall = dyn_cast(Call)) { const MemRegion *ThisRegion = CCall->getCXXThisVal().getAsRegion(); if (RegionOfInterest->isSubRegionOf(ThisRegion) && !CCall->getDecl()->isImplicit() && !isRegionOfInterestModifiedInFrame(N)) - return notModifiedInConstructorDiagnostics(Ctx, SM, PP, *CallExitLoc, + return notModifiedMemberDiagnostics(Ctx, SM, PP, *CallExitLoc, CCall, ThisRegion); } @@ -331,7 +343,7 @@ public: if (isRegionOfInterestModifiedInFrame(N)) return nullptr; - return notModifiedDiagnostics( + return notModifiedParameterDiagnostics( Ctx, SM, PP, *CallExitLoc, Call, PVD, R, IndirectionLevel); } QualType PT = T->getPointeeType(); @@ -346,6 +358,22 @@ public: } private: + + /// \return Whether the method declaration \p Parent + /// syntactically has a binary operation writing into the ivar \p Ivar. + bool potentiallyWritesIntoIvar(const Decl *Parent, + const ObjCIvarDecl *Ivar) { + using namespace ast_matchers; + if (!Parent || !Parent->getBody()) + return false; + StatementMatcher WriteIntoIvarM = binaryOperator( + hasOperatorName("="), hasLHS(ignoringParenImpCasts(objcIvarRefExpr( + hasDeclaration(equalsNode(Ivar)))))); + StatementMatcher ParentM = stmt(hasDescendant(WriteIntoIvarM)); + auto Matches = match(ParentM, *Parent->getBody(), Parent->getASTContext()); + return !Matches.empty(); + } + /// Check and lazily calculate whether the region of interest is /// modified in the stack frame to which \p N belongs. /// The calculation is cached in FramesModifyingRegion. @@ -414,19 +442,21 @@ private: Ty->getPointeeType().getCanonicalType().isConstQualified(); } - std::shared_ptr notModifiedInConstructorDiagnostics( + /// \return Diagnostics piece for the member field not modified + /// in a given function. + std::shared_ptr notModifiedMemberDiagnostics( const LocationContext *Ctx, const SourceManager &SM, const PrintingPolicy &PP, CallExitBegin &CallExitLoc, - const CXXConstructorCall *Call, + CallEventRef<> Call, const MemRegion *ArgRegion) { + const char *TopRegionName = isa(Call) ? "self" : "this"; SmallString<256> sbuf; llvm::raw_svector_ostream os(sbuf); os << DiagnosticsMsg; - bool out = prettyPrintRegionName( - "this", "->", /*IsReference=*/true, - /*IndirectionLevel=*/1, ArgRegion, os, PP); + bool out = prettyPrintRegionName(TopRegionName, "->", /*IsReference=*/true, + /*IndirectionLevel=*/1, ArgRegion, os, PP); // Return nothing if we have failed to pretty-print. if (!out) @@ -434,14 +464,16 @@ private: os << "'"; PathDiagnosticLocation L = - getPathDiagnosticLocation(nullptr, SM, Ctx, Call); + getPathDiagnosticLocation(CallExitLoc.getReturnStmt(), SM, Ctx, Call); return std::make_shared(L, os.str()); } + /// \return Diagnostics piece for the parameter \p PVD not modified + /// in a given function. /// \p IndirectionLevel How many times \c ArgRegion has to be dereferenced /// before we get to the super region of \c RegionOfInterest std::shared_ptr - notModifiedDiagnostics(const LocationContext *Ctx, + notModifiedParameterDiagnostics(const LocationContext *Ctx, const SourceManager &SM, const PrintingPolicy &PP, CallExitBegin &CallExitLoc, @@ -481,8 +513,8 @@ private: /// Pretty-print region \p ArgRegion starting from parent to \p os. /// \return whether printing has succeeded - bool prettyPrintRegionName(const char *TopRegionName, - const char *Sep, + bool prettyPrintRegionName(StringRef TopRegionName, + StringRef Sep, bool IsReference, int IndirectionLevel, const MemRegion *ArgRegion, @@ -491,7 +523,8 @@ private: SmallVector Subregions; const MemRegion *R = RegionOfInterest; while (R != ArgRegion) { - if (!(isa(R) || isa(R))) + if (!(isa(R) || isa(R) || + isa(R))) return false; // Pattern-matching failed. Subregions.push_back(R); R = cast(R)->getSuperRegion(); @@ -519,6 +552,10 @@ private: os << Sep; FR->getDecl()->getDeclName().print(os, PP); Sep = "."; + } else if (const auto *IR = dyn_cast(*I)) { + os << "->"; + IR->getDecl()->getDeclName().print(os, PP); + Sep = "."; } else if (isa(*I)) { continue; // Just keep going up to the base region. } else { diff --git a/clang/test/Analysis/diagnostics/no-store-func-path-notes.cpp b/clang/test/Analysis/diagnostics/no-store-func-path-notes.cpp index b96dc4c..17ec96a 100644 --- a/clang/test/Analysis/diagnostics/no-store-func-path-notes.cpp +++ b/clang/test/Analysis/diagnostics/no-store-func-path-notes.cpp @@ -97,6 +97,14 @@ struct C { int x; int y; C(int pX, int pY) : x(pX) {} // expected-note{{Returning without writing to 'this->y'}} + + C(int pX, int pY, bool Flag) { + x = pX; + if (Flag) // expected-note{{Assuming 'Flag' is not equal to 0}} + // expected-note@-1{{Taking true branch}} + return; // expected-note{{Returning without writing to 'this->y'}} + y = pY; + } }; int use_constructor() { @@ -106,6 +114,15 @@ int use_constructor() { // expected-warning@-1{{Undefined or garbage value returned to caller}} } +int coin(); + +int use_other_constructor() { + C c(0, 0, coin()); // expected-note{{Calling constructor for 'C'}} + // expected-note@-1{{Returning from constructor for 'C'}} + return c.y; // expected-note{{Undefined or garbage value returned to caller}} + // expected-warning@-1{{Undefined or garbage value returned to caller}} +} + struct D { void initialize(int *); }; @@ -122,8 +139,6 @@ int use_d_initializer(D* d) { // expected-warning@-1{{Undefined or garbage value returned to caller}} } -int coin(); - struct S2 { int x; }; diff --git a/clang/test/Analysis/diagnostics/no-store-func-path-notes.m b/clang/test/Analysis/diagnostics/no-store-func-path-notes.m index 3ae97ef..51d1515 100644 --- a/clang/test/Analysis/diagnostics/no-store-func-path-notes.m +++ b/clang/test/Analysis/diagnostics/no-store-func-path-notes.m @@ -1,6 +1,10 @@ -// RUN: %clang_analyze_cc1 -x objective-c -analyzer-checker=core -analyzer-output=text -Wno-objc-root-class -fblocks -verify %s +// RUN: %clang_analyze_cc1 -x objective-c -analyzer-checker=core,nullability -analyzer-output=text -Wno-objc-root-class -fblocks -verify %s -@interface I +#include "../Inputs/system-header-simulator-for-nullability.h" + +extern int coin(); + +@interface I : NSObject - (int)initVar:(int *)var param:(int)param; @end @@ -44,3 +48,30 @@ int initFromBlock() { }(); return z; } + +extern void expectNonNull(NSString * _Nonnull a); + +@interface A : NSObject +- (void) func; +- (void) initAMaybe; +@end + +@implementation A { + NSString * a; +} + +- (void) initAMaybe { + if (coin()) // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + a = @"string"; +} // expected-note{{Returning without writing to 'self->a'}} + +- (void) func { + a = nil; // expected-note{{nil object reference stored to 'a'}} + [self initAMaybe]; // expected-note{{Calling 'initAMaybe'}} + // expected-note@-1{{Returning from 'initAMaybe'}} + expectNonNull(a); // expected-warning{{nil passed to a callee that requires a non-null 1st parameter}} + // expected-note@-1{{nil passed to a callee that requires a non-null 1st parameter}} +} + +@end -- 2.7.4