Should be fixed in trunk > On Aug 3, 2018, at 1:30 PM, George Karpenkov via cfe-commits > <cfe-commits@lists.llvm.org> wrote: > > Yeah, saw that as well, fix coming. > >> On Aug 3, 2018, at 10:32 AM, Alexander Kornienko <ale...@google.com >> <mailto: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 <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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits