r338667 - [analyzer] Extend NoStoreFuncVisitor to follow fields.
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 3 16:32:51 PDT 2018
Should be fixed in trunk
> On Aug 3, 2018, at 1:30 PM, George Karpenkov via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>
> Yeah, saw that as well, fix coming.
>
>> On Aug 3, 2018, at 10:32 AM, Alexander Kornienko <alexfh at google.com <mailto: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 <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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180803/e9819e8a/attachment-0001.html>
More information about the cfe-commits
mailing list