r338149 - [analyzer] Extend NoStoreFuncVisitor to insert a note on IVars
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 27 11:26:40 PDT 2018
Author: george.karpenkov
Date: Fri Jul 27 11:26:40 2018
New Revision: 338149
URL: http://llvm.org/viewvc/llvm-project?rev=338149&view=rev
Log:
[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
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=338149&r1=338148&r2=338149&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Fri Jul 27 11:26:40 2018
@@ -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<ObjCMethodCall>(Call))
+ if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(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<CXXConstructorCall>(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<PathDiagnosticPiece> notModifiedInConstructorDiagnostics(
+ /// \return Diagnostics piece for the member field not modified
+ /// in a given function.
+ std::shared_ptr<PathDiagnosticPiece> notModifiedMemberDiagnostics(
const LocationContext *Ctx,
const SourceManager &SM,
const PrintingPolicy &PP,
CallExitBegin &CallExitLoc,
- const CXXConstructorCall *Call,
+ CallEventRef<> Call,
const MemRegion *ArgRegion) {
+ const char *TopRegionName = isa<ObjCMethodCall>(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<PathDiagnosticEventPiece>(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<PathDiagnosticPiece>
- 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<const MemRegion *, 5> Subregions;
const MemRegion *R = RegionOfInterest;
while (R != ArgRegion) {
- if (!(isa<FieldRegion>(R) || isa<CXXBaseObjectRegion>(R)))
+ if (!(isa<FieldRegion>(R) || isa<CXXBaseObjectRegion>(R) ||
+ isa<ObjCIvarRegion>(R)))
return false; // Pattern-matching failed.
Subregions.push_back(R);
R = cast<SubRegion>(R)->getSuperRegion();
@@ -519,6 +552,10 @@ private:
os << Sep;
FR->getDecl()->getDeclName().print(os, PP);
Sep = ".";
+ } else if (const auto *IR = dyn_cast<ObjCIvarRegion>(*I)) {
+ os << "->";
+ IR->getDecl()->getDeclName().print(os, PP);
+ Sep = ".";
} else if (isa<CXXBaseObjectRegion>(*I)) {
continue; // Just keep going up to the base region.
} else {
Modified: cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp?rev=338149&r1=338148&r2=338149&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp (original)
+++ cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp Fri Jul 27 11:26:40 2018
@@ -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 at -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 at -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 at -1{{Returning from constructor for 'C'}}
+ return c.y; // expected-note{{Undefined or garbage value returned to caller}}
+ // expected-warning at -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 at -1{{Undefined or garbage value returned to caller}}
}
-int coin();
-
struct S2 {
int x;
};
Modified: cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m?rev=338149&r1=338148&r2=338149&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m (original)
+++ cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m Fri Jul 27 11:26:40 2018
@@ -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
- at interface I
+#include "../Inputs/system-header-simulator-for-nullability.h"
+
+extern int coin();
+
+ at 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);
+
+ at interface A : NSObject
+- (void) func;
+- (void) initAMaybe;
+ at end
+
+ at implementation A {
+ NSString * a;
+}
+
+- (void) initAMaybe {
+ if (coin()) // expected-note{{Assuming the condition is false}}
+ // expected-note at -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 at -1{{Returning from 'initAMaybe'}}
+ expectNonNull(a); // expected-warning{{nil passed to a callee that requires a non-null 1st parameter}}
+ // expected-note at -1{{nil passed to a callee that requires a non-null 1st parameter}}
+}
+
+ at end
More information about the cfe-commits
mailing list