r338667 - [analyzer] Extend NoStoreFuncVisitor to follow fields.
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 3 13:30:57 PDT 2018
Yeah, saw that as well, fix coming.
> On Aug 3, 2018, at 10:32 AM, Alexander Kornienko <alexfh at google.com> wrote:
>
>
>
> On Thu, Aug 2, 2018 at 4:03 AM George Karpenkov via cfe-commits <cfe-commits at lists.llvm.org <mailto:cfe-commits at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180803/3ca01051/attachment-0001.html>
More information about the cfe-commits
mailing list