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