<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Aug 2, 2018 at 4:03 AM George Karpenkov via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: george.karpenkov<br>
Date: Wed Aug 1 19:02:40 2018<br>
New Revision: 338667<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=338667&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=338667&view=rev</a><br>
Log:<br>
[analyzer] Extend NoStoreFuncVisitor to follow fields.<br>
<br>
rdar://39701823<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D49901" rel="noreferrer" target="_blank">https://reviews.llvm.org/D49901</a><br>
<br>
Modified:<br>
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp<br>
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.c<br>
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp<br>
cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m<br>
<br>
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=338667&r1=338666&r2=338667&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=338667&r1=338666&r2=338667&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)<br>
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Aug 1 19:02:40 2018<br>
@@ -269,10 +269,14 @@ namespace {<br>
/// pointer dereference outside.<br>
class NoStoreFuncVisitor final : public BugReporterVisitor {<br>
const SubRegion *RegionOfInterest;<br>
+ MemRegionManager &MmrMgr;<br>
const SourceManager &SM;<br>
const PrintingPolicy &PP;<br>
- static constexpr const char *DiagnosticsMsg =<br>
- "Returning without writing to '";<br>
+<br>
+ /// Recursion limit for dereferencing fields when looking for the<br>
+ /// region of interest.<br>
+ /// The limit of two indicates that we will dereference fields only once.<br>
+ static const unsigned DEREFERENCE_LIMIT = 2;<br>
<br>
/// Frames writing into \c RegionOfInterest.<br>
/// This visitor generates a note only if a function does not write into<br>
@@ -285,15 +289,17 @@ class NoStoreFuncVisitor final : public<br>
llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingRegion;<br>
llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;<br>
<br>
+ using RegionVector = SmallVector<const MemRegion *, 5>;<br>
public:<br>
NoStoreFuncVisitor(const SubRegion *R)<br>
- : RegionOfInterest(R),<br>
- SM(R->getMemRegionManager()->getContext().getSourceManager()),<br>
- PP(R->getMemRegionManager()->getContext().getPrintingPolicy()) {}<br>
+ : RegionOfInterest(R), MmrMgr(*R->getMemRegionManager()),<br>
+ SM(MmrMgr.getContext().getSourceManager()),<br>
+ PP(MmrMgr.getContext().getPrintingPolicy()) {}<br>
<br>
void Profile(llvm::FoldingSetNodeID &ID) const override {<br>
static int Tag = 0;<br>
ID.AddPointer(&Tag);<br>
+ ID.AddPointer(RegionOfInterest);<br>
}<br>
<br>
std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,<br>
@@ -307,48 +313,69 @@ public:<br>
auto CallExitLoc = N->getLocationAs<CallExitBegin>();<br>
<br>
// No diagnostic if region was modified inside the frame.<br>
- if (!CallExitLoc)<br>
+ if (!CallExitLoc || isRegionOfInterestModifiedInFrame(N))<br>
return nullptr;<br>
<br>
CallEventRef<> Call =<br>
BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);<br>
<br>
+ if (SM.isInSystemHeader(Call->getDecl()->getSourceRange().getBegin()))<br>
+ return nullptr;<br>
+<br>
// Region of interest corresponds to an IVar, exiting a method<br>
// which could have written into that IVar, but did not.<br>
- if (const auto *MC = dyn_cast<ObjCMethodCall>(Call))<br>
- if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest))<br>
- if (potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),<br>
- IvarR->getDecl()) &&<br>
- !isRegionOfInterestModifiedInFrame(N))<br>
- return notModifiedMemberDiagnostics(<br>
- Ctx, *CallExitLoc, Call, MC->getReceiverSVal().getAsRegion());<br>
+ if (const auto *MC = dyn_cast<ObjCMethodCall>(Call)) {<br>
+ if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest)) {<br>
+ const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();<br>
+ if (RegionOfInterest->isSubRegionOf(SelfRegion) &&<br>
+ potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),<br>
+ IvarR->getDecl()))<br>
+ return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, SelfRegion,<br>
+ "self", /*FirstIsReferenceType=*/false,<br>
+ 1);<br>
+ }<br>
+ }<br>
<br>
if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {<br>
const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();<br>
if (RegionOfInterest->isSubRegionOf(ThisR)<br>
- && !CCall->getDecl()->isImplicit()<br>
- && !isRegionOfInterestModifiedInFrame(N))<br>
- return notModifiedMemberDiagnostics(Ctx, *CallExitLoc, Call, ThisR);<br>
+ && !CCall->getDecl()->isImplicit())<br>
+ return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, ThisR,<br>
+ "this",<br>
+ /*FirstIsReferenceType=*/false, 1);<br>
+<br>
+ // Do not generate diagnostics for not modified parameters in<br>
+ // constructors.<br>
+ return nullptr;<br>
}<br>
<br>
ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);<br>
for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {<br>
- const ParmVarDecl *PVD = parameters[I];<br>
+ Optional<unsigned> AdjustedIdx = Call->getAdjustedParameterIndex(I);<br>
+ if (!AdjustedIdx)<br>
+ continue;<br>
+ const ParmVarDecl *PVD = parameters[*AdjustedIdx];<br>
SVal S = Call->getArgSVal(I);<br>
- unsigned IndirectionLevel = 1;<br>
+ bool ParamIsReferenceType = PVD->getType()->isReferenceType();<br>
+ std::string ParamName = PVD->getNameAsString();<br>
+<br>
+ int IndirectionLevel = 1;<br>
QualType T = PVD->getType();<br>
while (const MemRegion *R = S.getAsRegion()) {<br>
- if (RegionOfInterest->isSubRegionOf(R)<br>
- && !isPointerToConst(PVD->getType())) {<br>
-<br>
- if (isRegionOfInterestModifiedInFrame(N))<br>
- return nullptr;<br>
+ if (RegionOfInterest->isSubRegionOf(R) && !isPointerToConst(T))<br>
+ return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, R,<br>
+ ParamName, ParamIsReferenceType,<br>
+ IndirectionLevel);<br>
<br>
- return notModifiedParameterDiagnostics(<br>
- Ctx, *CallExitLoc, Call, PVD, R, IndirectionLevel);<br>
- }<br>
QualType PT = T->getPointeeType();<br>
if (PT.isNull() || PT->isVoidType()) break;<br>
+<br>
+ if (const RecordDecl *RD = PT->getAsRecordDecl())<br>
+ if (auto P = findRegionOfInterestInRecord(RD, State, R))<br>
+ return notModifiedDiagnostics(<br>
+ Ctx, *CallExitLoc, Call, *P, RegionOfInterest, ParamName,<br>
+ ParamIsReferenceType, IndirectionLevel);<br>
+<br>
S = State->getSVal(R, PT);<br>
T = PT;<br>
IndirectionLevel++;<br>
@@ -359,20 +386,94 @@ public:<br>
}<br>
<br>
private:<br>
+ /// Attempts to find the region of interest in a given CXX decl,<br>
+ /// by either following the base classes or fields.<br>
+ /// Dereferences fields up to a given recursion limit.<br>
+ /// Note that \p Vec is passed by value, leading to quadratic copying cost,<br>
+ /// but it's OK in practice since its length is limited to DEREFERENCE_LIMIT.<br>
+ /// \return A chain fields leading to the region of interest or None.<br>
+ const Optional<RegionVector><br>
+ findRegionOfInterestInRecord(const RecordDecl *RD, ProgramStateRef State,<br>
+ const MemRegion *R,<br>
+ RegionVector Vec = {},<br>
+ int depth = 0) {<br>
+<br>
+ if (depth == DEREFERENCE_LIMIT) // Limit the recursion depth.<br>
+ return None;<br>
+<br>
+ if (const auto *RDX = dyn_cast<CXXRecordDecl>(RD))<br>
+ if (!RDX->hasDefinition())<br>
+ return None;<br>
+<br>
+ // Recursively examine the base classes.<br>
+ // Note that following base classes does not increase the recursion depth.<br>
+ if (const auto *RDX = dyn_cast<CXXRecordDecl>(RD))<br>
+ for (const auto II : RDX->bases())<br>
+ if (const RecordDecl *RRD = II.getType()->getAsRecordDecl())<br>
+ if (auto Out = findRegionOfInterestInRecord(RRD, State, R, Vec, depth))<br>
+ return Out;<br>
+<br>
+ for (const FieldDecl *I : RD->fields()) {<br>
+ QualType FT = I->getType();<br>
+ const FieldRegion *FR = MmrMgr.getFieldRegion(I, cast<SubRegion>(R));<br>
+ const SVal V = State->getSVal(FR);<br>
+ const MemRegion *VR = V.getAsRegion();<br>
+<br>
+ RegionVector VecF = Vec;<br>
+ VecF.push_back(FR);<br>
+<br>
+ if (RegionOfInterest == VR)<br>
+ return VecF;<br>
+<br>
+ if (const RecordDecl *RRD = FT->getAsRecordDecl())<br>
+ if (auto Out =<br>
+ findRegionOfInterestInRecord(RRD, State, FR, VecF, depth + 1))<br>
+ return Out;<br>
+<br>
+ QualType PT = FT->getPointeeType();<br>
+ if (PT.isNull() || PT->isVoidType() || !VR) continue;<br>
+<br>
+ if (const RecordDecl *RRD = PT->getAsRecordDecl())<br>
+ if (auto Out =<br>
+ findRegionOfInterestInRecord(RRD, State, VR, VecF, depth + 1))<br>
+ return Out;<br>
+<br>
+ }<br>
+<br>
+ return None;<br>
+ }<br>
<br>
/// \return Whether the method declaration \p Parent<br>
/// syntactically has a binary operation writing into the ivar \p Ivar.<br>
bool potentiallyWritesIntoIvar(const Decl *Parent,<br>
const ObjCIvarDecl *Ivar) {<br>
using namespace ast_matchers;<br>
- if (!Parent || !Parent->getBody())<br>
+ const char * IvarBind = "Ivar";<br>
+ if (!Parent || !Parent->hasBody())<br>
return false;<br>
StatementMatcher WriteIntoIvarM = binaryOperator(<br>
- hasOperatorName("="), hasLHS(ignoringParenImpCasts(objcIvarRefExpr(<br>
- hasDeclaration(equalsNode(Ivar))))));<br>
+ hasOperatorName("="),<br>
+ hasLHS(ignoringParenImpCasts(<br>
+ objcIvarRefExpr(hasDeclaration(equalsNode(Ivar))).bind(IvarBind))));<br>
StatementMatcher ParentM = stmt(hasDescendant(WriteIntoIvarM));<br>
auto Matches = match(ParentM, *Parent->getBody(), Parent->getASTContext());<br>
- return !Matches.empty();<br>
+ for (BoundNodes &Match : Matches) {<br>
+ auto IvarRef = Match.getNodeAs<ObjCIvarRefExpr>(IvarBind);<br>
+ if (IvarRef->isFreeIvar())<br>
+ return true;<br>
+<br>
+ const Expr *Base = IvarRef->getBase();<br>
+ if (const auto *ICE = dyn_cast<ImplicitCastExpr>(Base))<br>
+ Base = ICE->getSubExpr();<br>
+<br>
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(Base))<br>
+ if (const auto *ID = dyn_cast<ImplicitParamDecl>(DRE->getDecl()))<br>
+ if (ID->getParameterKind() == ImplicitParamDecl::ObjCSelf)<br>
+ return true;<br>
+<br>
+ return false;<br>
+ }<br>
+ return false;<br>
}<br>
<br>
/// Check and lazily calculate whether the region of interest is<br>
@@ -433,6 +534,8 @@ private:<br>
RuntimeDefinition RD = Call->getRuntimeDefinition();<br>
if (const auto *FD = dyn_cast_or_null<FunctionDecl>(RD.getDecl()))<br>
return FD->parameters();<br>
+ if (const auto *MD = dyn_cast_or_null<ObjCMethodDecl>(RD.getDecl()))<br>
+ return MD->parameters();<br>
<br>
return Call->parameters();<br>
}<br>
@@ -443,123 +546,105 @@ private:<br>
Ty->getPointeeType().getCanonicalType().isConstQualified();<br>
}<br>
<br>
- /// \return Diagnostics piece for the member field not modified<br>
- /// in a given function.<br>
- std::shared_ptr<PathDiagnosticPiece> notModifiedMemberDiagnostics(<br>
- const LocationContext *Ctx,<br>
- CallExitBegin &CallExitLoc,<br>
- CallEventRef<> Call,<br>
- const MemRegion *ArgRegion) {<br>
- const char *TopRegionName = isa<ObjCMethodCall>(Call) ? "self" : "this";<br>
- SmallString<256> sbuf;<br>
- llvm::raw_svector_ostream os(sbuf);<br>
- os << DiagnosticsMsg;<br>
- bool out = prettyPrintRegionName(TopRegionName, "->", /*IsReference=*/true,<br>
- /*IndirectionLevel=*/1, ArgRegion, os, PP);<br>
-<br>
- // Return nothing if we have failed to pretty-print.<br>
- if (!out)<br>
- return nullptr;<br>
-<br>
- os << "'";<br>
- PathDiagnosticLocation L =<br>
- getPathDiagnosticLocation(CallExitLoc.getReturnStmt(), SM, Ctx, Call);<br>
- return std::make_shared<PathDiagnosticEventPiece>(L, os.str());<br>
- }<br>
-<br>
- /// \return Diagnostics piece for the parameter \p PVD not modified<br>
- /// in a given function.<br>
- /// \p IndirectionLevel How many times \c ArgRegion has to be dereferenced<br>
- /// before we get to the super region of \c RegionOfInterest<br>
+ /// \return Diagnostics piece for region not modified in the current function.<br>
std::shared_ptr<PathDiagnosticPiece><br>
- notModifiedParameterDiagnostics(const LocationContext *Ctx,<br>
+ notModifiedDiagnostics(const LocationContext *Ctx,<br>
CallExitBegin &CallExitLoc,<br>
CallEventRef<> Call,<br>
- const ParmVarDecl *PVD,<br>
- const MemRegion *ArgRegion,<br>
+ RegionVector FieldChain,<br>
+ const MemRegion *MatchedRegion,<br>
+ StringRef FirstElement,<br>
+ bool FirstIsReferenceType,<br>
unsigned IndirectionLevel) {<br>
- PathDiagnosticLocation L = getPathDiagnosticLocation(<br>
- CallExitLoc.getReturnStmt(), SM, Ctx, Call);<br>
+<br>
+ PathDiagnosticLocation L;<br>
+ if (const ReturnStmt *RS = CallExitLoc.getReturnStmt()) {<br>
+ L = PathDiagnosticLocation::createBegin(RS, SM, Ctx);<br>
+ } else {<br>
+ L = PathDiagnosticLocation(<br>
+ Call->getRuntimeDefinition().getDecl()->getSourceRange().getEnd(),<br>
+ SM);<br>
+ }<br>
+<br>
SmallString<256> sbuf;<br>
llvm::raw_svector_ostream os(sbuf);<br>
- os << DiagnosticsMsg;<br>
- bool IsReference = PVD->getType()->isReferenceType();<br>
- const char *Sep = IsReference && IndirectionLevel == 1 ? "." : "->";<br>
- bool Success = prettyPrintRegionName(<br>
- PVD->getQualifiedNameAsString().c_str(),<br>
- Sep, IsReference, IndirectionLevel, ArgRegion, os, PP);<br>
-<br>
- // Print the parameter name if the pretty-printing has failed.<br>
- if (!Success)<br>
- PVD->printQualifiedName(os);<br>
+ os << "Returning without writing to '";<br>
+ prettyPrintRegionName(FirstElement, FirstIsReferenceType, MatchedRegion,<br>
+ FieldChain, IndirectionLevel, os);<br>
+<br>
os << "'";<br>
return std::make_shared<PathDiagnosticEventPiece>(L, os.str());<br>
}<br>
<br>
- /// \return a path diagnostic location for the optionally<br>
- /// present return statement \p RS.<br>
- PathDiagnosticLocation getPathDiagnosticLocation(const ReturnStmt *RS,<br>
- const SourceManager &SM,<br>
- const LocationContext *Ctx,<br>
- CallEventRef<> Call) {<br>
- if (RS)<br>
- return PathDiagnosticLocation::createBegin(RS, SM, Ctx);<br>
- return PathDiagnosticLocation(<br>
- Call->getRuntimeDefinition().getDecl()->getSourceRange().getEnd(), SM);<br>
- }<br>
-<br>
- /// Pretty-print region \p ArgRegion starting from parent to \p os.<br>
- /// \return whether printing has succeeded<br>
- bool prettyPrintRegionName(StringRef TopRegionName,<br>
- StringRef Sep,<br>
- bool IsReference,<br>
- int IndirectionLevel,<br>
- const MemRegion *ArgRegion,<br>
- llvm::raw_svector_ostream &os,<br>
- const PrintingPolicy &PP) {<br>
- SmallVector<const MemRegion *, 5> Subregions;<br>
+ /// Pretty-print region \p MatchedRegion to \p os.<br>
+ void prettyPrintRegionName(StringRef FirstElement, bool FirstIsReferenceType,<br>
+ const MemRegion *MatchedRegion,<br>
+ RegionVector FieldChain, int IndirectionLevel,<br>
+ llvm::raw_svector_ostream &os) {<br>
+<br>
+ if (FirstIsReferenceType)<br>
+ IndirectionLevel--;<br>
+<br>
+ RegionVector RegionSequence;<br>
+<br>
+ // Add the regions in the reverse order, then reverse the resulting array.<br>
+ assert(RegionOfInterest->isSubRegionOf(MatchedRegion));<br>
const MemRegion *R = RegionOfInterest;<br>
- while (R != ArgRegion) {<br>
- if (!(isa<FieldRegion>(R) || isa<CXXBaseObjectRegion>(R) ||<br>
- isa<ObjCIvarRegion>(R)))<br>
- return false; // Pattern-matching failed.<br>
- Subregions.push_back(R);<br>
+ while (R != MatchedRegion) {<br>
+ RegionSequence.push_back(R);<br>
R = cast<SubRegion>(R)->getSuperRegion();<br>
}<br>
- bool IndirectReference = !Subregions.empty();<br>
+ std::reverse(RegionSequence.begin(), RegionSequence.end());<br>
+ RegionSequence.append(FieldChain.begin(), FieldChain.end());<br>
<br>
- if (IndirectReference)<br>
- IndirectionLevel--; // Due to "->" symbol.<br>
+ StringRef Sep;<br>
+ for (const MemRegion *R : RegionSequence) {<br>
<br>
- if (IsReference)<br>
- IndirectionLevel--; // Due to reference semantics.<br>
+ // Just keep going up to the base region.<br>
+ if (isa<CXXBaseObjectRegion>(R) || isa<CXXTempObjectRegion>(R))<br>
+ continue;<br>
+<br>
+ if (Sep.empty())<br>
+ Sep = prettyPrintFirstElement(FirstElement,<br>
+ /*MoreItemsExpected=*/true,<br>
+ IndirectionLevel, os);<br>
+<br>
+ os << Sep;<br>
+<br>
+ const auto *DR = cast<DeclRegion>(R);<br>
+ Sep = DR->getValueType()->isAnyPointerType() ? "->" : ".";<br>
+ DR->getDecl()->getDeclName().print(os, PP);<br></blockquote><div><br></div><div>We have crashes on this line no test case yet.</div></div></div>