[analyzer] Extend NoStoreFuncVisitor to recursively follow fields

[george.karpenkov]

Summary

It's probably worth to refactor a machinery for finding a path to a particular region from a particular region into a common subroutine.

rdar://39701823

[george.karpenkov]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

398 ↗

(On Diff #157826)

@Szelethus the code below does pointer-chasing and tries to find a chain of regions from an input region to the region of interest.
Do you think it would make sense to extract in and share with your checker?

[NoQ]

Looks great! Marking as "requested changes" as per repeated requests from the working class.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

294–296 ↗

(On Diff #158028)

This would need a rebase after an update to D49772.

326–331 ↗

(On Diff #158028)

Hmm, shouldn't we also verify that the receiver is the super-region of the region of interest, like we do in other places?

350–351 ↗

(On Diff #158028)

I think you should use my new function, Call->getAdjustedParameterIndex(I), that's added in D49715, because parameters and arguments aren't always in sync. Then you also don't have to check parameters explicitly.

390 ↗

(On Diff #158028)

Maybe let's add a TODO to warn people that we're passing vectors around quadratically?

462 ↗

(On Diff #158028)

Let's instead cast it to ImplicitParamDecl and check its kind, just to be sure.

598 ↗

(On Diff #158028)

I'm not sure i understand this comment.

398 ↗

(On Diff #157826)

+

[NoQ]

Looks good, apart from waiting on D49715 for getAdjustedParameterIndex().

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

330–332 ↗

(On Diff #158141)

Let's re-order the checks. The isSubRegionOf() check is cheaper.

[Szelethus]

Is something like this worth adding to the C++ test file?

struct HasRefToItself {
  HasRefToItself &Ref; // no infinite loop
  int &z;
  HasRefToItself(int &z) : Ref(*this), z(z) {}
};

Maybe a list-like struct that has a cycle?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

398 ↗

(On Diff #157826)

Whoo, thanks for tagging me! Sadly I have little knowledge about visitors so I can't add anything meaningful to the conversation just yet, but I really like what I'm seeing in this function. I'll definitely take a look how I could incorporate this into my checker, as pointer handling is kind of a mess there.

[george.karpenkov]

Is something like this worth adding to the C++ test file?

We limit the recursion depth to 1 for fields, so it won't dereference fields more than once. It could be still worthwhile to have this test to make sure this condition does not break.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

390 ↗

(On Diff #158028)

I'll add a note. It's not an issue if our vectors are of size <= 2, and for a bigger size we'll have other problems first.

462 ↗

(On Diff #158028)

OK. Probably deserves a separate matcher.

398 ↗

(On Diff #157826)

@Szelethus the fact that it's in the visitor is immaterial here, I'm just talking about the function findRegionOfInterestInRecord.
Basically, it tries to solve a (subset) of the problem of "whether there's a path from region A to region B".
The output should either be "no path" or a sequence of regions denoting a path.

In a broad sense, that's a breadth-first-search where possible operations are

• Dereferencing the region
• Looking for subregions
• Going through the fields
• Going through superclasses

That seems too expensive though, so here we only dereference fields twice.

[george.karpenkov]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

330–332 ↗

(On Diff #158141)

it depends, but OK

350–351 ↗

(On Diff #158028)

I don't understand the semantics if the optional is empty. What then?

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

350–351 ↗

(On Diff #158028)

If the optional is empty, then the argument doesn't correspond to any parameter. For example, CXXOperatorCallExpr takes this-argument as an explicit argument, but CXXMethodDecl of a member operator doesn't take it as a parameter, but treats it as an implicit parameter instead, so no valid index into parameters() can be returned.

[NoQ]

Looks good!

[NoQ]

cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

354–356

Uhm! I realized that i was wrong about this one. You don't need the adjustment when iterating through CallEvent arguments; you only need it when iterating through CallExpr arguments, which are also conveniently shifted against each other.

cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 namespace {
 
 /// Put a diagnostic on return statement of all inlined functions
 /// for which  the region of interest \p RegionOfInterest was passed into,
 /// but not written inside, and it has caused an undefined read or a null
 /// 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
   /// a region of interest. This information is not immediately available
   /// by looking at the node associated with the exit from the function
   /// (usually the return statement). To avoid recomputing the same information
   /// many times (going up the path for each node and checking whether the
   /// region was written into) we instead lazily compute the
   /// stack frames along the path which write into the region of interest.
   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,
                                                  const ExplodedNode *PrevN,
                                                  BugReporterContext &BRC,
                                                  BugReport &BR) override {
 
     const LocationContext *Ctx = N->getLocationContext();
     const StackFrameContext *SCtx = Ctx->getStackFrame();
     ProgramStateRef State = N->getState();
     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 (RegionOfInterest->isSubRegionOf(R) && !isPointerToConst(T))
+          return notModifiedDiagnostics(Ctx, *CallExitLoc, Call, {}, R,
+                                        ParamName, ParamIsReferenceType,
+                                        IndirectionLevel);
 
-          if (isRegionOfInterestModifiedInFrame(N))
-            return nullptr;
-
-          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++;
       }
     }
 
     return nullptr;
   }
 
 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
   /// modified in the stack frame to which \p N belongs.
   /// The calculation is cached in FramesModifyingRegion.
   bool isRegionOfInterestModifiedInFrame(const ExplodedNode *N) {
     const LocationContext *Ctx = N->getLocationContext();
     const StackFrameContext *SCtx = Ctx->getStackFrame();
private:
 
   /// Get parameters associated with runtime definition in order
   /// to get the correct parameter name.
   ArrayRef<ParmVarDecl *> getCallParameters(CallEventRef<> Call) {
     // Use runtime definition, if available.
     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();
   }
 
   /// \return whether \p Ty points to a const type, or is a const reference.
   bool isPointerToConst(QualType Ty) {
     return !Ty->getPointeeType().isNull() &&
            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,
+  /// \return Diagnostics piece for region not modified in the current function.
+  std::shared_ptr<PathDiagnosticPiece>
+  notModifiedDiagnostics(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;
+                         RegionVector FieldChain,
+                         const MemRegion *MatchedRegion,
+                         StringRef FirstElement,
+                         bool FirstIsReferenceType,
+                         unsigned IndirectionLevel) {
 
-    os << "'";
-    PathDiagnosticLocation L =
-        getPathDiagnosticLocation(CallExitLoc.getReturnStmt(), SM, Ctx, Call);
-    return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
+    PathDiagnosticLocation L;
+    if (const ReturnStmt *RS = CallExitLoc.getReturnStmt()) {
+      L = PathDiagnosticLocation::createBegin(RS, SM, Ctx);
+    } else {
+      L = PathDiagnosticLocation(
+          Call->getRuntimeDefinition().getDecl()->getSourceRange().getEnd(),
+          SM);
     }
 
-  /// \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
-  std::shared_ptr<PathDiagnosticPiece>
-  notModifiedParameterDiagnostics(const LocationContext *Ctx,
-                         CallExitBegin &CallExitLoc,
-                         CallEventRef<> Call,
-                         const ParmVarDecl *PVD,
-                         const MemRegion *ArgRegion,
-                         unsigned IndirectionLevel) {
-    PathDiagnosticLocation L = getPathDiagnosticLocation(
-        CallExitLoc.getReturnStmt(), SM, Ctx, Call);
     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;
 
-    bool ShouldSurround = IndirectReference && IndirectionLevel > 0;
+      if (Sep.empty())
+        Sep = prettyPrintFirstElement(FirstElement,
+                                      /*MoreItemsExpected=*/true,
+                                      IndirectionLevel, os);
 
-    if (ShouldSurround)
+      os << Sep;
+
+      const auto *DR = cast<DeclRegion>(R);
+      Sep = DR->getValueType()->isAnyPointerType() ? "->" : ".";
+      DR->getDecl()->getDeclName().print(os, PP);
+    }
+
+    if (Sep.empty())
+      prettyPrintFirstElement(FirstElement,
+                              /*MoreItemsExpected=*/false, IndirectionLevel,
+                              os);
+  }
+
+  /// Print first item in the chain, return new separator.
+  StringRef prettyPrintFirstElement(StringRef FirstElement,
+                       bool MoreItemsExpected,
+                       int IndirectionLevel,
+                       llvm::raw_svector_ostream &os) {
+    StringRef Out = ".";
+
+    if (IndirectionLevel > 0 && MoreItemsExpected) {
+      IndirectionLevel--;
+      Out = "->";
+    }
+
+    if (IndirectionLevel > 0 && MoreItemsExpected)
       os << "(";
-    for (int i = 0; i < IndirectionLevel; i++)
+
+    for (int i=0; i<IndirectionLevel; i++)
       os << "*";
-    os << TopRegionName;
-    if (ShouldSurround)
+    os << FirstElement;
+
+    if (IndirectionLevel > 0 && MoreItemsExpected)
       os << ")";
 
-    for (auto I = Subregions.rbegin(), E = Subregions.rend(); I != E; ++I) {
-      if (const auto *FR = dyn_cast<FieldRegion>(*I)) {
-        os << Sep;
-        FR->getDecl()->getDeclName().print(os, PP);
-        Sep = ".";
-      } else if (const auto *IR = dyn_cast<ObjCIvarRegion>(*I)) {
-        os << "->";
-        IR->getDecl()->getDeclName().print(os, PP);
-        Sep = ".";
-      } else if (isa<CXXBaseObjectRegion>(*I)) {
-        continue; // Just keep going up to the base region.
-      } else {
-        llvm_unreachable("Previous check has missed an unexpected region");
-      }
-    }
-    return true;
+    return Out;
   }
 };
 
 /// Suppress null-pointer-dereference bugs where dereferenced null was returned
 /// the macro.
 class MacroNullReturnSuppressionVisitor final : public BugReporterVisitor {
   const SubRegion *RegionOfInterest;
   const SVal ValueAtDereference;
if (Inner && ExplodedGraph::isInterestingLValueExpr(Inner)) {
     // In case of C++ references, we want to differentiate between a null
     // reference and reference to null pointer.
     // If the LVal is null, check if we are dealing with null reference.
     // For those, we want to track the location of the reference.
     const MemRegion *R = (RR && LVIsNull) ? RR :
         LVNode->getSVal(Inner).getAsRegion();
 
     if (R) {
+      ProgramStateRef S = N->getState();
 
       // Mark both the variable region and its contents as interesting.
       SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
       report.addVisitor(
           llvm::make_unique<NoStoreFuncVisitor>(cast<SubRegion>(R)));
 
       MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
           N, R, EnableNullFPSuppression, report, V);

cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.c

 int useindirectingstruct() {
   S s;
   S* p = &s;
   indirectingstruct(&p); //expected-note{{Calling 'indirectingstruct'}}
                          //expected-note@-1{{Returning from 'indirectingstruct'}}
   return s.x; // expected-warning{{Undefined or garbage value returned to caller}}
               // expected-note@-1{{Undefined or garbage value returned to caller}}
 }
+
+typedef struct {
+  int *x;
+} D;
+
+void initializeMaybeInStruct(D* pD) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    *pD->x = 120;
+} // expected-note{{Returning without writing to 'pD->x'}}
+
+int useInitializeMaybeInStruct() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  D d;
+  d.x = &z;
+  initializeMaybeInStruct(&d); // expected-note{{Calling 'initializeMaybeInStruct'}}
+                               // expected-note@-1{{Returning from 'initializeMaybeInStruct'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}

cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.cpp

 // RUN: %clang_analyze_cc1 -x c++ -analyzer-checker=core -analyzer-output=text -verify %s
 
 int initializer1(int &p, int x) {
   if (x) { // expected-note{{Taking false branch}}
     p = 1;
     return 0;
   } else {
     return 1; // expected-note {{Returning without writing to 'p'}}
   }
 }
 
 int param_not_initialized_by_func() {
-  int p;                        // expected-note {{'p' declared without an initial value}}
-  int out = initializer1(p, 0); // expected-note{{Calling 'initializer1'}}
+  int outP;                        // expected-note {{'outP' declared without an initial value}}
+  int out = initializer1(outP, 0); // expected-note{{Calling 'initializer1'}}
                                 // expected-note@-1{{Returning from 'initializer1'}}
-  return p;                     // expected-note{{Undefined or garbage value returned to caller}}
+  return outP;                     // expected-note{{Undefined or garbage value returned to caller}}
                                 // expected-warning@-1{{Undefined or garbage value returned to caller}}
 }
 
 struct S {
   int initialize(int *p, int param) {
     if (param) { //expected-note{{Taking false branch}}
       *p = 1;
       return 1;
void rdar40335545() {
     void (*takes_int_ptr_argument)(int *) = (void (*)(int*))has_no_argument_and_returns_null;
 
     takes_int_ptr_argument(&local); // no-crash
 
     int useLocal = local; //expected-warning{{}}
                           //expected-note@-1{{}}
     (void)useLocal;
 }
+
+////////
+
+struct HasRef {
+  int &a;
+  HasRef(int &a) : a(a) {}
+};
+
+
+void maybeInitialize(const HasRef &&pA) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    pA.a = 120;
+} // expected-note{{Returning without writing to 'pA.a'}}
+
+int useMaybeInitializerWritingIntoField() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  maybeInitialize(HasRef(z)); // expected-note{{Calling constructor for 'HasRef'}}
+                              // expected-note@-1{{Returning from constructor for 'HasRef'}}
+                              // expected-note@-2{{Calling 'maybeInitialize'}}
+                              // expected-note@-3{{Returning from 'maybeInitialize'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+////////
+
+struct HasRefToItself {
+  HasRefToItself &Ref; // no infinite loop
+  int &z;
+  HasRefToItself(int &z) : Ref(*this), z(z) {}
+};
+
+void maybeInitialize(const HasRefToItself &&pA) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    pA.z = 120;
+} // expected-note{{Returning without writing to 'pA.Ref.z'}}
+
+int useMaybeInitializerWritingIntoFieldWithRefToItself() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  maybeInitialize(HasRefToItself(z)); // expected-note{{Calling constructor for 'HasRefToItself'}}
+                              // expected-note@-1{{Returning from constructor for 'HasRefToItself'}}
+                              // expected-note@-2{{Calling 'maybeInitialize'}}
+                              // expected-note@-3{{Returning from 'maybeInitialize'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+////
+
+void maybeInitialize(const HasRef *pA) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    pA->a = 120;
+} // expected-note{{Returning without writing to 'pA->a'}}
+
+int useMaybeInitializerStructByPointer() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  HasRef wrapper(z); // expected-note{{Calling constructor for 'HasRef'}}
+                     // expected-note@-1{{Returning from constructor for 'HasRef'}}
+  maybeInitialize(&wrapper); // expected-note{{Calling 'maybeInitialize'}}
+                             // expected-note@-1{{Returning from 'maybeInitialize'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+////////
+
+struct HasParentWithRef : public HasRef {
+  HasParentWithRef(int &a) : HasRef(a) {} // expected-note{{Calling constructor for 'HasRef'}}
+                                          // expected-note@-1{{Returning from constructor for 'HasRef'}}
+};
+
+void maybeInitializeWithParent(const HasParentWithRef &pA) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    pA.a = 120;
+} // expected-note{{Returning without writing to 'pA.a'}}
+
+int useMaybeInitializerWritingIntoParentField() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  maybeInitializeWithParent(HasParentWithRef(z)); // expected-note{{Calling constructor for 'HasParentWithRef'}}
+                              // expected-note@-1{{Returning from constructor for 'HasParentWithRef'}}
+                              // expected-note@-2{{Calling 'maybeInitializeWithParent'}}
+                              // expected-note@-3{{Returning from 'maybeInitializeWithParent'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+////////
+
+struct HasIndirectRef {
+  HasRef &Ref;
+  HasIndirectRef(HasRef &Ref) : Ref(Ref) {}
+};
+
+void maybeInitializeIndirectly(const HasIndirectRef &pA) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    pA.Ref.a = 120;
+} // expected-note{{Returning without writing to 'pA.Ref.a'}}
+
+int useMaybeInitializeIndirectly() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  HasRef r(z); // expected-note{{Calling constructor for 'HasRef'}}
+               // expected-note@-1{{Returning from constructor for 'HasRef'}}
+  maybeInitializeIndirectly(HasIndirectRef(r)); // expected-note{{Calling 'maybeInitializeIndirectly'}}
+                                                // expected-note@-1{{Returning from 'maybeInitializeIndirectly'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+////////
+
+struct HasIndirectRefByValue {
+  HasRef Ref;
+  HasIndirectRefByValue(HasRef Ref) : Ref(Ref) {}
+};
+
+void maybeInitializeIndirectly(const HasIndirectRefByValue &pA) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    pA.Ref.a = 120;
+} // expected-note{{Returning without writing to 'pA.Ref.a'}}
+
+int useMaybeInitializeIndirectlyIndirectRefByValue() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  HasRef r(z); // expected-note{{Calling constructor for 'HasRef'}}
+               // expected-note@-1{{Returning from constructor for 'HasRef'}}
+  maybeInitializeIndirectly(HasIndirectRefByValue(r)); // expected-note{{Calling 'maybeInitializeIndirectly'}}
+                                                // expected-note@-1{{Returning from 'maybeInitializeIndirectly'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+////////
+
+struct HasIndirectPointerRef {
+  HasRef *Ref;
+  HasIndirectPointerRef(HasRef *Ref) : Ref(Ref) {}
+};
+
+void maybeInitializeIndirectly(const HasIndirectPointerRef &pA) {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    pA.Ref->a = 120;
+} // expected-note{{Returning without writing to 'pA.Ref->a'}}
+
+int useMaybeInitializeIndirectlyWithPointer() {
+  int z; // expected-note{{'z' declared without an initial value}}
+  HasRef r(z); // expected-note{{Calling constructor for 'HasRef'}}
+               // expected-note@-1{{Returning from constructor for 'HasRef'}}
+  maybeInitializeIndirectly(HasIndirectPointerRef(&r)); // expected-note{{Calling 'maybeInitializeIndirectly'}}
+                                                // expected-note@-1{{Returning from 'maybeInitializeIndirectly'}}
+  return z; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}

cfe/trunk/test/Analysis/diagnostics/no-store-func-path-notes.m

z = p;               // expected-warning{{Assigned value is garbage or undefined}}
                          // expected-note@-1{{Assigned value is garbage or undefined}}
   }();
   return z;
 }
 
 extern void expectNonNull(NSString * _Nonnull a);
 
 @interface A : NSObject
-- (void) func;
 - (void) initAMaybe;
 @end
 
 @implementation A {
   NSString * a;
 }
 
 - (void) initAMaybe {
   if (coin()) // expected-note{{Assuming the condition is false}}
               // expected-note@-1{{Taking false branch}}
     a = @"string";
 } // expected-note{{Returning without writing to 'self->a'}}
 
-- (void) func {
+- (void) passNullToNonnull {
   a = nil; // expected-note{{nil object reference stored to 'a'}}
   [self initAMaybe]; // expected-note{{Calling 'initAMaybe'}}
                      // expected-note@-1{{Returning from 'initAMaybe'}}
   expectNonNull(a); // expected-warning{{nil passed to a callee that requires a non-null 1st parameter}}
                     // expected-note@-1{{nil passed to a callee that requires a non-null 1st parameter}}
 }
 
+- (void) initAMaybeWithExplicitSelf {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    self->a = @"string";
+} // expected-note{{Returning without writing to 'self->a'}}
+
+- (void) passNullToNonnullWithExplicitSelf {
+  self->a = nil; // expected-note{{nil object reference stored to 'a'}}
+  [self initAMaybeWithExplicitSelf]; // expected-note{{Calling 'initAMaybeWithExplicitSelf'}}
+                     // expected-note@-1{{Returning from 'initAMaybeWithExplicitSelf'}}
+  expectNonNull(a); // expected-warning{{nil passed to a callee that requires a non-null 1st parameter}}
+                    // expected-note@-1{{nil passed to a callee that requires a non-null 1st parameter}}
+}
+
+- (void) initPassedAMaybe:(A *) param {
+  if (coin()) // expected-note{{Assuming the condition is false}}
+              // expected-note@-1{{Taking false branch}}
+    param->a = @"string";
+} // expected-note{{Returning without writing to 'param->a'}}
+
+- (void) useInitPassedAMaybe:(A *) paramA {
+  paramA->a = nil; // expected-note{{nil object reference stored to 'a'}}
+  [self initPassedAMaybe:paramA]; // expected-note{{Calling 'initPassedAMaybe:'}}
+                                  // expected-note@-1{{Returning from 'initPassedAMaybe:'}}
+  expectNonNull(paramA->a); // expected-warning{{nil passed to a callee that requires a non-null 1st parameter}}
+                            // expected-note@-1{{nil passed to a callee that requires a non-null 1st parameter}}
+
+}
+
 @end