[analyzer] mark returns of functions where the region passed as parameter was not initialized

[george.karpenkov]

Summary

In the wild, many cases of null pointer dereference, or uninitialized value read occur because the value was meant to be initialized by the inlined function, but did not, most often due to error condition in the inlined function.
This change highlights the return branch taken by the inlined function, in order to help user understand the error report and see why the value was uninitialized.

rdar://36287652

[george.karpenkov]

The case which I don't know how to handle for know is "void" functions without a return statement. Where the note should be attached in those cases?

[NoQ]

Where the note should be attached in those cases?

At the closing brace, i guess. You can actually match for the CallExitBegin program point - regardless of the return statement.

[dcoughlin]

This is great!

I think you'll also want to handle the case where the region of interest is a field and the containing struct/class is passed to the function (same for when the region of interest is an array element). Subregion::isSubRegionOf() will probably be helpful there.

You'll also probably want to handle when the region of interest is a sub region of 'this'/'self'.

If we find that too many notes are being emitted (once you handle 'self' I think that will be a real possibility) you may want to add a heuristic that only displays the notes when a function has control flow or when some other path may write to the region of interest. (This last could be done with a simple, separate, flow-insensitive syntactic analysis).

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

166

It would be good to extend the comment to say that it emits the diagnostic note when the region of interested is a *passed to a function* but the function doesn't store into it.

187

Do you want to only add this note once per path? Or could there be multiple function calls per path that we would want to show to the user?

194

Note that ObjCMethodDecl isn't a subtype of FunctionDecl, so your later logic won't kick in for ObjC methods.

You can use the CallEvent interface to abstract over this. See visitNodeMaybeUnsuppress() for an example.

200

Note that the parent context of the current context may not always be context of the caller. It could be a block invocation context or (in the near future) any other lexical scope of interest.

You should get the parent of SCtx instead (or, really, just use CallEvent instead).

205

I think it would be good to not emit the note for const parameters since it is unlikely that the programmer expects those to update the region.

227

The convention is to start the note with a capital letter.

Also, we typically want the note to describe something that happens at that particular program point (in contrast with something that didn't happen previously along the path).

I would suggest something like "Returning without writing to '*p'".

A nice QoI improvement would be to specialize this for the uninitialized case: "Returning without initializing "'*p'", but that should probably wait until later.

228

Note the difference between writing to 'p' and writing to '*p'. You'll probably want to pass in the region of interest as well as the parameter to construct a human readable description -- although in some cases you probably won't be able to unambiguously come up with one.

test/Analysis/no-store-func-path-notes.m

1 ↗

(On Diff #129022)

I think it would be fine to collapse these tests all into a single .mm file, but that's up to you.

[xazax.hun]

I like this patch, only found a few nits after the other's review.

I have one high-level comment though regarding how could the analyzer be more user-friendly (and only slightly related to this patch). It is really good to have additional notes to aid the understanding of the user but having redundant notes, on the other hand, can make it harder to understand the bugs. My main concern is that we use UndefVal for both uninitialized values and values that are undefined due to an operation like shift by a negative value. I wonder if we would like to use the same set of bug path visitors in both cases.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

173

Nit: extra space before public.

190

Asterisk on the wrong side.

202

arg should start with an uppercase letter.

[NoQ]

In D41848#971436, @xazax.hun wrote:

I have one high-level comment though regarding how could the analyzer be more user-friendly (and only slightly related to this patch). It is really good to have additional notes to aid the understanding of the user but having redundant notes, on the other hand, can make it harder to understand the bugs. My main concern is that we use UndefVal for both uninitialized values and values that are undefined due to an operation like shift by a negative value. I wonder if we would like to use the same set of bug path visitors in both cases.

Btw, do you have any thoughts on treating UndefinedVal as a sub-kind of taint? Like, replace it with a normal symbol (eg. SymbolRegionValue for uninitialized locals, or SymIntExpr for $x << 32) and then attaching a taint to it? It should help tracking the values, understanding their origins, and it matches the idea that undefined values, like user inputs, may indeed be equal to any value within their respective ranges (unlike normal values which are only known to be possibly equal to at least one value within the range). However, a lot of this seems redundant because we are trying to immediately catch undefs (there's really no point in propagating them or allowing the user to remove the taint by constraining the symbol to a constant), while taint analysis is not yet even enabled by default, and also we don't have a good value class to represent many undefined values (eg., how do we represent a result of pointer dereference? - we may still want to make some sort of NullPointerRegion for the different purpose of computing null pointer offsets correctly, so then we'd be able to take SymbolRegionValue from it and taint it, but, again, we're not anywhere near there yet).

[george.karpenkov]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

187

From what I've seen, the situation you describe is very rare: the most common idiom is along the lines of

S s;
int out = initialize(&s);
s->doStuff(); // optionally forgetting to check the exit code

but then it probably does not hurt to add notes to all functions touching the region.

228

Pretty-printing MemRegion * results in a name of the argument at the function call site, which is not very understandable in the context of a called function.
@dcoughlin would it make sense to do something like

"Returning without writing to 'p' (x not populated)"

? I think not.

test/Analysis/no-store-func-path-notes.m

1 ↗

(On Diff #129022)

Technically these languages have different semantics, but on the other hand it's good not to pollute test/Analysis further.

[george.karpenkov]

You'll also probably want to handle when the region of interest is a sub region of 'this'/'self'.
If we find that too many notes are being emitted (once you handle 'self' I think that will be a real possibility) you may want to add a heuristic that only displays the notes when a function has control flow or when some other path may write to the region of interest. (This last could be done with a simple, separate, flow-insensitive syntactic analysis).

I don't think that's a good idea. In fact, I think that's extending this patch to C++/Obj-CPP is already a stretch.

The pattern declare-but-not-initialize-and-pass-address-to-a-func occurs almost exclusively in C,
as in C++ a default constructor is usually invoked (and structs in general are populated using a constructor).

[george.karpenkov]

I think you'll also want to handle the case where the region of interest is a field and the containing struct/class is passed to the function (same for when the region of interest is an array element). Subregion::isSubRegionOf() will probably be helpful there.

done

[dcoughlin]

In D41848#971727, @george.karpenkov wrote:

You'll also probably want to handle when the region of interest is a sub region of 'this'/'self'.
If we find that too many notes are being emitted (once you handle 'self' I think that will be a real possibility) you may want to add a heuristic that only displays the notes when a function has control flow or when some other path may write to the region of interest. (This last could be done with a simple, separate, flow-insensitive syntactic analysis).

I don't think that's a good idea. In fact, I think that's extending this patch to C++/Obj-CPP is already a stretch.

The pattern declare-but-not-initialize-and-pass-address-to-a-func occurs almost exclusively in C,
as in C++ a default constructor is usually invoked (and structs in general are populated using a constructor).

The "pass off the address of an uninitialized local" pattern is quite common in Objective-C. It is also quite common to forget to initialize a field in a C++ constructor.

Note that this visitor will be called when the uninitialized value comes from malloc()/new and not just from an uninitialized local whose address is taken.

Also note that this visitor is called for tracking both uninitialized values and also nil/NULL values.

In the spirit of incremental development, I'm fine with holding off on fully supporting structs and C++/Objective-C classes until a later patch. But these are important cases that you'll need to support.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

187

Do you mean the base region or the actual region of interest? It probably doesn't make sense to emit a note for (for example) a setter method that only touches an other field than the field for the region of interest.

200

Should this use 'getArgSVal()' on CallEvent?

228

I was imagining you would use the MemRegion to construct the series of lvalue projections to get from the base region to the subregion and display that in terms of the parameter.
For example, p->foo.bar[2].blam. Here you would construct ->foo.bar[2].blam from MemRegion and p from the parameter name.

230

Some cases to consider:

void foo(int *p) {
  p = NULL;

  return; // "Returning without writing to 'p'" is not correct since p was written to
}

Also:

struct S {
  int f;
  int g;
};
void foo (struct S *p) {
  p->f = 5;
  // Forget to initialize field 'g'

  return; // "Returning without writing to 'p'" is also misleading.
}

[NoQ]

This looks good so far, i only have minor nits to add to Devin's review. As for checking that the value wasn't written to (in the null value case), i guess you could scan all program states within the call and see if getSVal(R) returns the same value on all of them. Because i always forget in which direction does the visitor scan the nodes, it'd either be done easily when visiting previous nodes, or would require another loop through nodes, but in any case it shouldn't be too complicated.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

240–241

I believe that you should (because you can) assert that you're only dealing with SubRegions (or even type-check it). It doesn't make sense to track null or undefined values for memory spaces, only for [their] sub-regions.

test/Analysis/diagnostics/undef-value-param.c

15

These warnings seem to be out of sync with the patch.

[george.karpenkov]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

246

Any objections on adding this helper to ExplodedNode ? We have it in CheckerContext already, but it's not available for visitors.

[NoQ]

More nits!~

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

229–230

I'm for having a TODO for making an additional diagnostic for this situation, i.e. "Assignment does not change the value of p".

232

Should this also be isSubRegionOf()?

247–248

I suspect that you might want to take the canonical type first (i.e. unwrap typedefs).

[george.karpenkov]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

229–230

Makes sense, but I'm not really in favor of mixing two completely different tasks in one visitor.

[george.karpenkov]

Now with support for constructors.

[george.karpenkov]

Handling signatures with no param names correctly.

Update: I have examined the diagnostics on XNU source, there were 9 messages and 7 of them were helpful.
I guess less then we could have hoped for, but helpful still.

[george.karpenkov]

Minor refactoring.

[george.karpenkov]

Replace quadratic algorithm with linear.

[NoQ]

Replace quadratic algorithm with linear.

Yay, nice. I think it's also much easier to understand now.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

178

SmallPtrSet? (because it's cute)

230–231

Please add a comment explaining why is this pre-computation so good. Like, every time we reach CallExit, we'd need information from CallEnter (pointed-to values), which we didn't reach yet, so it's easier to just do this once for the whole path than to do this for every call, and cheaper on memory than adding every call site's store to the program state as a special trait.

369

I suggest a simple "Not implemented" assertion, because there is a clear intent to support more regions once they actually start showing up.

1344–1346

cast<SubRegion>(R) would combine the assertion and the cast.

Actually, i'm surprised we don't have MemRegion::castAs<>().

[george.karpenkov]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

369

The branch is actually unreachable: otherwise the while loop above would have return false beforehand.

[NoQ]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

369

Whoops i see.

[george.karpenkov]

TODO: for a single parameter, when passed as a pointer, display the star (the function is not writing into "p", the function is not writing into "*p")
TODO2: update the backup text, writing "inside the".

[george.karpenkov]

TODO3: heuristics for handling arrays?

[george.karpenkov]

I've realized that we don't handle the case where the function parameter is not directly pointing to a region-of-interested, but needs to be dereferenced a couple of times before that.

@dcoughlin I think this is ready to go. The "stars" are aligned correctly, however the default case is still the same. Basically the default case is for unforeseen cases which I currently don't know exist --- normally, it would be an assertion, but it seems to much to crash an analyzer over that.
If the current output is undesirable I think that another good option is just not printing the note in that case at all.

[NoQ]

I think this is ready to go. The "stars" are aligned correctly

LGTM. Hope they're still aligned =)

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

357

Please add parentheses because nobody remembers operation priority which makes the code hard to understand.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
+#include <set>
 
 using namespace clang;
 using namespace ento;
 
 using llvm::FoldingSetNodeID;
 
 //===----------------------------------------------------------------------===//
 // Utility functions.
std::unique_ptr<PathDiagnosticPiece> BugReporterVisitor::getDefaultEndPath(
   auto P = llvm::make_unique<PathDiagnosticEventPiece>(
       L, BR.getDescription(), Ranges.begin() == Ranges.end());
   for (SourceRange Range : Ranges)
     P->addRange(Range);
 
   return std::move(P);
 }
 
+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 BugReporterVisitorImpl<NoStoreFuncVisitor> {
+
+  const SubRegion *RegionOfInterest;
+  static constexpr const char *DiagnosticsMsg =
+      "Returning without writing to '";
+  bool Initialized = false;
+
+  /// Frames writing into \c RegionOfInterest.
+  std::set<const StackFrameContext *> FramesModifyingRegionOfInterest;
+
+public:
+  NoStoreFuncVisitor(const SubRegion *R) : RegionOfInterest(R) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int Tag = 0;
+    ID.AddPointer(&Tag);
+  }
+
+  std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                 const ExplodedNode *PrevN,
+                                                 BugReporterContext &BRC,
+                                                 BugReport &BR) override {
+    if (!Initialized) {
+      findModifyingFrames(N);
+      Initialized = true;
+    }
+
+    const LocationContext *Ctx = N->getLocationContext();
+    const StackFrameContext *SCtx = Ctx->getCurrentStackFrame();
+    ProgramStateRef State = N->getState();
+    auto CallExitLoc = N->getLocationAs<CallExitBegin>();
+
+    // No diagnostic if region was modified inside the frame.
+    if (!CallExitLoc || FramesModifyingRegionOfInterest.count(SCtx))
+      return nullptr;
+
+    CallEventRef<> Call =
+        BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
+
+    if (auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
+      const MemRegion *ThisRegion = CCall->getCXXThisVal().getAsRegion();
+      if (RegionOfInterest->isSubRegionOf(ThisRegion) &&
+          !CCall->getDecl()->isImplicit())
+        return notModifiedInConstructorDiagnostics(Ctx, BRC, *CallExitLoc,
+                                                   CCall, ThisRegion);
+    }
+
+    ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);
+    for (unsigned I = 0, E = Call->getNumArgs(); I != E; ++I) {
+      const ParmVarDecl *PVD = parameters[I];
+      const MemRegion *R = Call->getArgSVal(I).getAsRegion();
+      if (RegionOfInterest->isSubRegionOf(R) &&
+          !isPointerToConst(PVD->getType()))
+        return notModifiedDiagnostics(Ctx, BRC, *CallExitLoc, Call, PVD, R);
+    }
+
+    return nullptr;
+  }
+
+private:
+  /// Write to \c FramesModifyingRegionOfInterest all stack frames along
+  /// the path which modify \c RegionOfInterest.
+  void findModifyingFrames(const ExplodedNode *N) {
+    ProgramStateRef LastReturnState;
+    do {
+      ProgramStateRef State = N->getState();
+      auto CallExitLoc = N->getLocationAs<CallExitBegin>();
+      if (CallExitLoc) {
+        LastReturnState = State;
+      }
+      if (LastReturnState &&
+          wasRegionOfInterestModifiedAt(N, LastReturnState)) {
+        const StackFrameContext *SCtx =
+            N->getLocationContext()->getCurrentStackFrame();
+        while (!SCtx->inTopFrame()) {
+          FramesModifyingRegionOfInterest.insert(SCtx);
+          SCtx = SCtx->getParent()->getCurrentStackFrame();
+        }
+      }
+
+      N = N->getFirstPred();
+    } while (N);
+  }
+
+  /// \return Whether \c RegionOfInterest was modified at \p N,
+  /// where \p ReturnState is a state associated with the return
+  /// from the current frame.
+  bool wasRegionOfInterestModifiedAt(const ExplodedNode *N,
+                                     ProgramStateRef ReturnState) {
+    SVal ValueAtReturn = ReturnState->getSVal(RegionOfInterest);
+
+    // Writing into region of interest.
+    if (auto PS = N->getLocationAs<PostStmt>())
+      if (auto *BO = PS->getStmtAs<BinaryOperator>())
+        if (BO->isAssignmentOp() && RegionOfInterest->isSubRegionOf(
+                                        N->getSVal(BO->getLHS()).getAsRegion()))
+          return true;
+
+    // SVal after the state is possibly different.
+    SVal ValueAtN = N->getState()->getSVal(RegionOfInterest);
+    if (!ReturnState->areEqual(ValueAtN, ValueAtReturn).isConstrainedTrue() &&
+        (!ValueAtN.isUndef() || !ValueAtReturn.isUndef()))
+      return true;
+
+    return false;
+  }
+
+  /// Get parameters associated with runtime definition in order
+  /// to get the correct parameter name.
+  ArrayRef<ParmVarDecl *> getCallParameters(CallEventRef<> Call) {
+    if (isa<FunctionDecl>(Call->getDecl()))
+      return dyn_cast<FunctionDecl>(Call->getRuntimeDefinition().getDecl())
+          ->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();
+  }
+
+  std::shared_ptr<PathDiagnosticPiece> notModifiedInConstructorDiagnostics(
+      const LocationContext *Ctx, BugReporterContext &BRC,
+      CallExitBegin &CallExitLoc, const CXXConstructorCall *Call,
+      const MemRegion *ArgRegion) {
+
+    SmallString<256> sbuf;
+    llvm::raw_svector_ostream os(sbuf);
+    os << DiagnosticsMsg;
+    os << "this";
+    const PrintingPolicy &PP = BRC.getASTContext().getPrintingPolicy();
+    bool out = prettyPrintRegionName("->", ArgRegion, os, PP);
+    if (!out) // Return nothing if we have failed to pretty-print.
+      return nullptr;
+    os << "'";
+    PathDiagnosticLocation L =
+        getPathDiagnosticLocation(nullptr, BRC.getSourceManager(), Ctx, Call);
+    return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
+  }
+
+  std::shared_ptr<PathDiagnosticPiece>
+  notModifiedDiagnostics(const LocationContext *Ctx, BugReporterContext &BRC,
+                         CallExitBegin &CallExitLoc, CallEventRef<> Call,
+                         const ParmVarDecl *PVD, const MemRegion *ArgRegion) {
+
+    PathDiagnosticLocation L = getPathDiagnosticLocation(
+        CallExitLoc.getReturnStmt(), BRC.getSourceManager(), Ctx, Call);
+
+    SmallString<256> sbuf;
+    llvm::raw_svector_ostream os(sbuf);
+    os << DiagnosticsMsg;
+
+    PVD->printQualifiedName(os);
+    const char *Sep = PVD->getType()->isReferenceType() ? "." : "->";
+    const PrintingPolicy &PP = BRC.getASTContext().getPrintingPolicy();
+
+    // If printing fails, we just use previously printed parameter name.
+    prettyPrintRegionName(Sep, ArgRegion, os, PP);
+    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,
+                                                   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(const char *Sep, const MemRegion *ArgRegion,
+                             llvm::raw_svector_ostream &os,
+                             const PrintingPolicy &PP) {
+    SmallVector<const MemRegion *, 5> Regions;
+    const MemRegion *R = RegionOfInterest;
+    while (R != ArgRegion) {
+      if (!(isa<FieldRegion>(R) || isa<CXXBaseObjectRegion>(R)))
+        return false; // Pattern-matching failed.
+      Regions.push_back(R);
+      R = dyn_cast<SubRegion>(R)->getSuperRegion();
+    }
+
+    for (auto I = Regions.rbegin(), E = Regions.rend(); I != E; ++I) {
+      if (auto *FR = dyn_cast<FieldRegion>(*I)) {
+        os << Sep;
+        FR->getDecl()->getDeclName().print(os, PP);
+        Sep = ".";
+      } else if (auto *CXXR = dyn_cast<CXXBaseObjectRegion>(*I)) {
+        continue; // Just keep going up to the base region.
+      } else {
+        llvm_unreachable("Previous check has missed an unexpected region");
+      }
+    }
+    return true;
+  }
+};
+
+} // namespace
 
 namespace {
 /// Emits an extra note at the return statement of an interesting stack frame.
 ///
 /// The returned value is marked as an interesting value, and if it's null,
 /// adds a visitor to track where it became null.
 ///
 /// This visitor is intended to be used when another visitor discovers that an
if (Inner && ExplodedGraph::isInterestingLValueExpr(Inner)) {
     // 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) {
       // Mark both the variable region and its contents as interesting.
       SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
+      assert(isa<SubRegion>(R));
+      report.addVisitor(
+          llvm::make_unique<NoStoreFuncVisitor>(R->getAs<SubRegion>()));
 
       report.markInteresting(R);
       report.markInteresting(V);
       report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(R));
 
       // If the contents are symbolic, find out when they became null.
       if (V.getAsLocSymbol(/*IncludeBaseRegions*/ true))
         report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(

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

+// RUN: %clang_analyze_cc1 -x c -analyzer-checker=core -analyzer-output=text -verify %s
+
+typedef __typeof(sizeof(int)) size_t;
+void *memset(void *__s, int __c, size_t __n);
+
+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'}}
+                                 // expected-note@-1{{Returning from 'initializer1'}}
+  return p; // expected-note{{Undefined or garbage value returned to caller}}
+            // expected-warning@-1{{Undefined or garbage value returned to caller}}
+}
+
+int param_initialized_properly() {
+  int p;
+  int out = initializer1(&p, 1);
+  return p; //no-warning
+}
+
+static int global;
+
+int initializer2(int **p, int x) {
+  if (x) { // expected-note{{Taking false branch}}
+    *p = &global;
+    return 0;
+  } else {
+    return 1; // expected-note {{Returning without writing to 'p'}}
+  }
+}
+
+int param_not_written_into_by_func() {
+  int *p = 0;                    // expected-note{{'p' initialized to a null pointer value}}
+  int out = initializer2(&p, 0); // expected-note{{Calling 'initializer2'}}
+                                 // expected-note@-1{{Returning from 'initializer2'}}
+  return *p;                     // expected-warning{{Dereference of null pointer (loaded from variable 'p')}}
+                                 // expected-note@-1{{Dereference of null pointer (loaded from variable 'p')}}
+}
+
+void initializer3(int *p, int param) {
+  if (param) // expected-note{{Taking false branch}}
+    *p = 0;
+} // expected-note{{Returning without writing to 'p'}}
+
+int param_written_into_by_void_func() {
+  int p;               // expected-note{{'p' declared without an initial value}}
+  initializer3(&p, 0); // expected-note{{Calling 'initializer3'}}
+                       // expected-note@-1{{Returning from 'initializer3'}}
+  return p;            // expected-warning{{Undefined or garbage value returned to caller}}
+                       // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+void initializer4(int *p, int param) {
+  if (param) // expected-note{{Taking false branch}}
+    *p = 0;
+} // expected-note{{Returning without writing to 'p'}}
+
+void initializer5(int *p, int param) {
+  if (!param) // expected-note{{Taking false branch}}
+    *p = 0;
+} // expected-note{{Returning without writing to 'p'}}
+
+int multi_init_tries_func() {
+  int p;               // expected-note{{'p' declared without an initial value}}
+  initializer4(&p, 0); // expected-note{{Calling 'initializer4'}}
+                       // expected-note@-1{{Returning from 'initializer4'}}
+  initializer5(&p, 1); // expected-note{{Calling 'initializer5'}}
+                       // expected-note@-1{{Returning from 'initializer5'}}
+  return p;            // expected-warning{{Undefined or garbage value returned to caller}}
+                       // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+int initializer6(const int *p) {
+  return 0;
+}
+
+int no_msg_on_const() {
+  int p; // expected-note{{'p' declared without an initial value}}
+  initializer6(&p);
+  return p; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+typedef struct {
+  int x;
+} S;
+
+int initializer7(S *s, int param) {
+  if (param) { // expected-note{{Taking false branch}}
+    s->x = 0;
+    return 0;
+  }
+  return 1; // expected-note{{Returning without writing to 's->x'}}
+}
+
+int initialize_struct_field() {
+  S local;
+  initializer7(&local, 0); // expected-note{{Calling 'initializer7'}}
+                           // expected-note@-1{{Returning from 'initializer7'}}
+  return local.x;          // expected-warning{{Undefined or garbage value returned to caller}}
+                           // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+void nullwriter(int **p) {
+  *p = 0; // expected-note{{Null pointer value stored to 'p'}}
+} // no extra note
+
+int usage() {
+  int x = 0;
+  int *p = &x;
+  nullwriter(&p); // expected-note{{Calling 'nullwriter'}}
+                  // expected-note@-1{{Returning from 'nullwriter'}}
+  return *p;      // expected-warning{{Dereference of null pointer (loaded from variable 'p')}}
+                  // expected-note@-1{{Dereference of null pointer (loaded from variable 'p')}}
+}
+
+typedef struct {
+  int x;
+  int y;
+} A;
+
+void partial_initializer(A *a) {
+  a->x = 0;
+} // expected-note{{Returning without writing to 'a->y'}}
+
+int use_partial_initializer() {
+  A a;
+  partial_initializer(&a); // expected-note{{Calling 'partial_initializer'}}
+                           // expected-note@-1{{Returning from 'partial_initializer'}}
+  return a.y;              // expected-warning{{Undefined or garbage value returned to caller}}
+                           // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+typedef struct {
+  int x;
+  int y;
+} B;
+
+typedef struct {
+  B b;
+} C;
+
+void partial_nested_initializer(C *c) {
+  c->b.x = 0;
+} // expected-note{{Returning without writing to 'c->b.y'}}
+
+int use_partial_nested_initializer() {
+  B localB;
+  C localC;
+  localC.b = localB;
+  partial_nested_initializer(&localC); // expected-note{{Calling 'partial_nested_initializer'}}
+                                       // expected-note@-1{{Returning from 'partial_nested_initializer'}}
+  return localC.b.y;                   // expected-warning{{Undefined or garbage value returned to caller}}
+                                       // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+void test_subregion_assignment(C* c) {
+  B b;
+  c->b = b;
+}
+
+int use_subregion_assignment() {
+  C c;
+  test_subregion_assignment(&c); // expected-note{{Calling 'test_subregion_assignment'}}
+                                 // expected-note@-1{{Returning from 'test_subregion_assignment'}}
+  return c.b.x; // expected-warning{{Undefined or garbage value returned to caller}}
+                // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+int confusing_signature(int *);
+int confusing_signature(int *p) {
+  return 0; // expected-note{{Returning without writing to 'p'}}
+}
+
+int use_confusing_signature() {
+  int a; // expected-note {{'a' declared without an initial value}}
+  confusing_signature(&a); // expected-note{{Calling 'confusing_signature'}}
+                           // expected-note@-1{{Returning from 'confusing_signature'}}
+  return a; // expected-note{{Undefined or garbage value returned to caller}}
+            // expected-warning@-1{{Undefined or garbage value returned to caller}}
+}

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'}}
+                                // expected-note@-1{{Returning from 'initializer1'}}
+  return p;                     // 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;
+    }
+    return 0; // expected-note{{Returning without writing to 'p'}}
+  }
+};
+
+int use(S *s) {
+  int p;                //expected-note{{'p' declared without an initial value}}
+  s->initialize(&p, 0); //expected-note{{Calling 'S::initialize'}}
+                        //expected-note@-1{{Returning from 'S::initialize'}}
+  return p;             // expected-warning{{Undefined or garbage value returned to caller}}
+                        // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+int initializer2(const int &p) {
+  return 0;
+}
+
+int no_msg_const_ref() {
+  int p; //expected-note{{'p' declared without an initial value}}
+  initializer2(p);
+  return p; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+void nested() {}
+void init_in_nested_func(int **x) {
+  *x = 0; // expected-note{{Null pointer value stored to 'y'}}
+  nested();
+} // no-note
+
+int call_init_nested() {
+  int x = 0;
+  int *y = &x;
+  init_in_nested_func(&y); // expected-note{{Calling 'init_in_nested_func'}}
+                           // expected-note@-1{{Returning from 'init_in_nested_func'}}
+  return *y;               //expected-warning{{Dereference of null pointer (loaded from variable 'y')}}
+                           //expected-note@-1{{Dereference of null pointer (loaded from variable 'y')}}
+}
+
+struct A {
+  int x;
+  int y;
+};
+
+void partial_init_by_reference(A &a) {
+  a.x = 0;
+} // expected-note {{Returning without writing to 'a.y'}}
+
+int use_partial_init_by_reference() {
+  A a;
+  partial_init_by_reference(a); // expected-note{{Calling 'partial_init_by_reference'}}
+                                // expected-note@-1{{Returning from 'partial_init_by_reference'}}
+  return a.y;                   // expected-warning{{Undefined or garbage value returned to caller}}
+                                // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+struct B : A {
+};
+
+void partially_init_inherited_struct(B *b) {
+  b->x = 0;
+} // expected-note{{Returning without writing to 'b->y'}}
+
+int use_partially_init_inherited_struct() {
+  B b;
+  partially_init_inherited_struct(&b); // expected-note{{Calling 'partially_init_inherited_struct'}}
+                                       // expected-note@-1{{Returning from 'partially_init_inherited_struct'}}
+  return b.y;                          // expected-warning{{Undefined or garbage value returned to caller}}
+                                       // expected-note@-1{{Undefined or garbage value returned to caller}}
+}
+
+struct C {
+  int x;
+  int y;
+  C(int pX, int pY) : x(pX) {} // expected-note{{Returning without writing to 'this->y'}}
+};
+
+int use_constructor() {
+  C c(0, 0); // expected-note{{Calling constructor for 'C'}}
+             // expected-note@-1{{Returning from constructor for 'C'}}
+  return c.y; // expected-note{{Undefined or garbage value returned to caller}}
+              // expected-warning@-1{{Undefined or garbage value returned to caller}}
+}
+
+struct D {
+  void initialize(int *);
+};
+
+void D::initialize(int *p) {
+
+} // expected-note{{Returning without writing to 'p'}}
+
+int use_d_initializer(D* d) {
+  int p; // expected-note {{'p' declared without an initial value}}
+  d->initialize(&p); // expected-note{{Calling 'D::initialize'}}
+                     // expected-note@-1{{Returning from 'D::initialize'}}
+  return p;                     // expected-note{{Undefined or garbage value returned to caller}}
+                                // expected-warning@-1{{Undefined or garbage value returned to caller}}
+}

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

+// RUN: %clang_analyze_cc1 -x objective-c -analyzer-checker=core -analyzer-output=text -Wno-objc-root-class -fblocks -verify %s
+
+@interface I
+- (int)initVar:(int *)var param:(int)param;
+@end
+
+@implementation I
+- (int)initVar:(int *)var param:(int)param {
+  if (param) { // expected-note{{Taking false branch}}
+    *var = 1;
+    return 0;
+  }
+  return 1; // expected-note{{Returning without writing to 'var'}}
+}
+@end
+
+int foo(I *i) {
+  int x;                            //expected-note{{'x' declared without an initial value}}
+  int out = [i initVar:&x param:0]; //expected-note{{Calling 'initVar:param:'}}
+                                    //expected-note@-1{{Returning from 'initVar:param:'}}
+  if (out)                          // expected-note{{Taking true branch}}
+    return x;                       //expected-warning{{Undefined or garbage value returned to caller}}
+                                    //expected-note@-1{{Undefined or garbage value returned to caller}}
+  return 0;
+}
+
+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 initFromBlock() {
+  __block int z;
+  ^{                     // expected-note {{Calling anonymous block}}
+    int p;               // expected-note{{'p' declared without an initial value}}
+    initializer1(&p, 0); // expected-note{{Calling 'initializer1'}}
+                         // expected-note@-1{{Returning from 'initializer1'}}
+    z = p;               // expected-warning{{Assigned value is garbage or undefined}}
+                         // expected-note@-1{{Assigned value is garbage or undefined}}
+  }();
+  return z;
+}

test/Analysis/diagnostics/undef-value-param.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=text -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist-multi-file -analyzer-config path-diagnostics-alternate=false %s -o %t.plist
 // RUN: FileCheck --input-file=%t.plist %s
 
 void foo_irrelevant(int c) {
     if (c)
         return;
     c++;
     return;
 }
 void foo(int c, int *x) {
     if (c)
            //expected-note@-1{{Assuming 'c' is not equal to 0}}
            //expected-note@-2{{Taking true branch}}
-        return;
+           return; // expected-note{{Returning without writing to 'x'}}
     *x = 5;
 }
 
 int use(int c) {
     int xx; //expected-note {{'xx' declared without an initial value}}
     int *y = &xx;
     foo (c, y);
                 //expected-note@-1{{Calling 'foo'}}

 double *getValidPtr();
 struct WithFields {
   double *f1;
 };
 void initStruct(int x, struct WithFields *X) {
   if (x <= 0) //expected-note {{Taking true branch}}
               //expected-note@-1 {{Assuming 'x' is <= 0}}
 
-    return;
+    return; //expected-note{{Returning without writing to 'X->f1'}}
   X->f1 = getValidPtr();
 }
 double testPassingParentRegionStruct(int x) {
   struct WithFields st;
   st.f1 = 0; // expected-note {{Null pointer value stored to 'st.f1'}}
   initStruct(x, &st); //expected-note {{Calling 'initStruct'}}
                       //expected-note@-1 {{Returning from 'initStruct'}}
   return (*st.f1); //expected-warning {{Dereference of null pointer}}

 // CHECK-NEXT:            <key>col</key><integer>9</integer>
 // CHECK-NEXT:            <key>file</key><integer>0</integer>
 // CHECK-NEXT:           </dict>
 // CHECK-NEXT:          </array>
 // CHECK-NEXT:         <key>end</key>
 // CHECK-NEXT:          <array>
 // CHECK-NEXT:           <dict>
 // CHECK-NEXT:            <key>line</key><integer>15</integer>
-// CHECK-NEXT:            <key>col</key><integer>9</integer>
+// CHECK-NEXT:            <key>col</key><integer>12</integer>
 // CHECK-NEXT:            <key>file</key><integer>0</integer>
 // CHECK-NEXT:           </dict>
 // CHECK-NEXT:           <dict>
 // CHECK-NEXT:            <key>line</key><integer>15</integer>
-// CHECK-NEXT:            <key>col</key><integer>14</integer>
+// CHECK-NEXT:            <key>col</key><integer>17</integer>
 // CHECK-NEXT:            <key>file</key><integer>0</integer>
 // CHECK-NEXT:           </dict>
 // CHECK-NEXT:          </array>
 // CHECK-NEXT:        </dict>
 // CHECK-NEXT:       </array>
 // CHECK-NEXT:     </dict>
 // CHECK-NEXT:     <dict>
 // CHECK-NEXT:      <key>kind</key><string>event</string>
 // CHECK-NEXT:      <key>location</key>
 // CHECK-NEXT:      <dict>
+// CHECK-NEXT:       <key>line</key><integer>15</integer>
+// CHECK-NEXT:       <key>col</key><integer>12</integer>
+// CHECK-NEXT:       <key>file</key><integer>0</integer>
+// CHECK-NEXT:      </dict>
+// CHECK-NEXT:      <key>depth</key><integer>1</integer>
+// CHECK-NEXT:      <key>extended_message</key>
+// CHECK-NEXT:      <string>Returning without writing to &apos;x&apos;</string>
+// CHECK-NEXT:      <key>message</key>
+// CHECK-NEXT:      <string>Returning without writing to &apos;x&apos;</string>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>kind</key><string>event</string>
+// CHECK-NEXT:      <key>location</key>
+// CHECK-NEXT:      <dict>
 // CHECK-NEXT:       <key>line</key><integer>22</integer>
 // CHECK-NEXT:       <key>col</key><integer>5</integer>
 // CHECK-NEXT:       <key>file</key><integer>0</integer>
 // CHECK-NEXT:      </dict>
 // CHECK-NEXT:      <key>ranges</key>
 // CHECK-NEXT:      <array>
 // CHECK-NEXT:        <array>
 // CHECK-NEXT:         <dict>

 // CHECK-NEXT:          </array>
 // CHECK-NEXT:        </dict>
 // CHECK-NEXT:       </array>
 // CHECK-NEXT:     </dict>
 // CHECK-NEXT:     <dict>
 // CHECK-NEXT:      <key>kind</key><string>event</string>
 // CHECK-NEXT:      <key>location</key>
 // CHECK-NEXT:      <dict>
+// CHECK-NEXT:       <key>line</key><integer>54</integer>
+// CHECK-NEXT:       <key>col</key><integer>5</integer>
+// CHECK-NEXT:       <key>file</key><integer>0</integer>
+// CHECK-NEXT:      </dict>
+// CHECK-NEXT:      <key>depth</key><integer>1</integer>
+// CHECK-NEXT:      <key>extended_message</key>
+// CHECK-NEXT:      <string>Returning without writing to &apos;X-&gt;f1&apos;</string>
+// CHECK-NEXT:      <key>message</key>
+// CHECK-NEXT:      <string>Returning without writing to &apos;X-&gt;f1&apos;</string>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>kind</key><string>event</string>
+// CHECK-NEXT:      <key>location</key>
+// CHECK-NEXT:      <dict>
 // CHECK-NEXT:       <key>line</key><integer>60</integer>
 // CHECK-NEXT:       <key>col</key><integer>3</integer>
 // CHECK-NEXT:       <key>file</key><integer>0</integer>
 // CHECK-NEXT:      </dict>
 // CHECK-NEXT:      <key>ranges</key>
 // CHECK-NEXT:      <array>
 // CHECK-NEXT:        <array>
 // CHECK-NEXT:         <dict>

test/Analysis/diagnostics/undef-value-param.m

 
 static void CreateRefUndef(SCDynamicStoreRef *storeRef, unsigned x) {
   unsigned err = 0;
   SCDynamicStoreRef ref = anotherCreateRef(&err, x);
   if (err) {
              //expected-note@-1{{Assuming 'err' is not equal to 0}}
              //expected-note@-2{{Taking true branch}}
     CFRelease(ref);
-    return;
+    return; // expected-note{{Returning without writing to 'storeRef'}}
   }
   *storeRef = ref;
 }
 
 // CHECK:  <key>diagnostics</key>
 // CHECK-NEXT:  <array>
 // CHECK-NEXT:   <dict>
 // CHECK-NEXT:    <key>path</key>

 // CHECK-NEXT:            <key>col</key><integer>13</integer>
 // CHECK-NEXT:            <key>file</key><integer>0</integer>
 // CHECK-NEXT:           </dict>
 // CHECK-NEXT:          </array>
 // CHECK-NEXT:        </dict>
 // CHECK-NEXT:       </array>
 // CHECK-NEXT:     </dict>
 // CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>kind</key><string>control</string>
+// CHECK-NEXT:      <key>edges</key>
+// CHECK-NEXT:       <array>
+// CHECK-NEXT:        <dict>
+// CHECK-NEXT:         <key>start</key>
+// CHECK-NEXT:          <array>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>71</integer>
+// CHECK-NEXT:            <key>col</key><integer>5</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>71</integer>
+// CHECK-NEXT:            <key>col</key><integer>13</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:          </array>
+// CHECK-NEXT:         <key>end</key>
+// CHECK-NEXT:          <array>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>72</integer>
+// CHECK-NEXT:            <key>col</key><integer>5</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>72</integer>
+// CHECK-NEXT:            <key>col</key><integer>10</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:          </array>
+// CHECK-NEXT:        </dict>
+// CHECK-NEXT:       </array>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>kind</key><string>event</string>
+// CHECK-NEXT:      <key>location</key>
+// CHECK-NEXT:      <dict>
+// CHECK-NEXT:       <key>line</key><integer>72</integer>
+// CHECK-NEXT:       <key>col</key><integer>5</integer>
+// CHECK-NEXT:       <key>file</key><integer>0</integer>
+// CHECK-NEXT:      </dict>
+// CHECK-NEXT:      <key>depth</key><integer>1</integer>
+// CHECK-NEXT:      <key>extended_message</key>
+// CHECK-NEXT:      <string>Returning without writing to &apos;storeRef&apos;</string>
+// CHECK-NEXT:      <key>message</key>
+// CHECK-NEXT:      <string>Returning without writing to &apos;storeRef&apos;</string>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:     <dict>
 // CHECK-NEXT:      <key>kind</key><string>event</string>
 // CHECK-NEXT:      <key>location</key>
 // CHECK-NEXT:      <dict>
 // CHECK-NEXT:       <key>line</key><integer>45</integer>
 // CHECK-NEXT:       <key>col</key><integer>5</integer>
 // CHECK-NEXT:       <key>file</key><integer>0</integer>
 // CHECK-NEXT:      </dict>
 // CHECK-NEXT:      <key>ranges</key>