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