[analyzer] MallocChecker: Add a visitor to leave a note on functions that could have, but did not change ownership on leaked memory

[Szelethus]

Summary

This is a rather common feedback we get from out leak checkers: bug reports are really short, and are contain barely any usable information on what the analyzer did to conclude that a leak actually happened.

This happens because of our bug report minimizing effort. We construct bug reports by inspecting the ExplodedNodes that lead to the error from the bottom up (from the error node all the way to the root of the exploded graph), and mark entities that were the cause of a bug, or have interacted with it as interesting. In order to make the bug report a bit less verbose, whenever we find an entire function call (from CallEnter to CallExitEnd) that didn't talk about any interesting entity, we prune it (click here for more info on bug report generation). Even if the event to highlight is exactly this lack of interaction with interesting entities.

D105553 generalized the visitor that creates notes for these cases. This patch adds a new kind of NoStateChangeVisitor that leaves notes in functions that took a piece of dynamically allocated memory that later leaked as parameter, and didn't change its ownership status.

While there is some code to talk over in MallocChecker.cpp, the main thing to discuss in my mind are the test cases, where I display where I want to see this visitor end up. I hope to be able to reach a point sometime soon when I can run on this on some real projects and post screenshots about it!

[NoQ]

This is amazing, great progress!

I added the parent revision because it didn't compile on pre-merge checks otherwise.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

801

Can you also comment on what's, generally, the default scenario / motivating example where this is necessary? What makes you hunt down store bindings that didn't cause an escape to happen (given that an escape would have been a state change)? IIUC this is for the situation when the callee stores the pointer in a caller-local variable and in this case you don't want to claim that ownership didn't change?

809–810

I suggest: "Returning without deallocating memory or storing the pointer for later deallocation". Still a bit flimsy but less jargony.

832–833

How difficult would it be to re-use this part as well?

clang/test/Analysis/NewDeleteLeaks.cpp

105–107

How do you think this is different from the very first test, memory_allocated_in_fn_call()? Is this just because of how the pointer is put into a variable first? This function is kinda still the only place that could free memory.

[Szelethus]

• Fixes according to reviewer comments!
• Rebase on D105553.
• Primitively check whether the allocated memory was actually passed into the function.

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

801

The comment above is meant to explain it.

// [...] Any pointers
// inside the call that pointed to the allocated memory are of little
// consequence if their lifetime ends before within the function

832–833

Ugh, I gave this a shot, and its worse then expected. I think the original patch that George made was intended to be used by NoStoreFuncVisitor, and nothing else. As a consequence, its practically impossible to factor out without rewriting the whole thing.

At this point, I'm more in favor of just biting the bullet, and reuse how pointees and casts are printed from D50506, and maybe some of the dereferencing technology as well. It might be sliiiiiightly slower, but the majority of the price is only paid when FieldChainInfo is printed, not when its constructed.

For the time being, I made a rather primitive implementation here, which should lean on the conservative side (we under approximate the set of function calls where the allocated memory was actually passed into).

clang/test/Analysis/NewDeleteLeaks.cpp

105–107

That was my idea, yes, but you have a point there...

[Szelethus]

• Fix testfiles.
• Fix a typo in the comments.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

801

Oh, speaking of which, that is a gramarr eror.

[Szelethus]

An ever so gentle ping :^)

[Szelethus]

Ping

[NoQ]

This is amazing and I think we should land this and see how it goes.

clang/test/Analysis/self-assign.cpp

1

Changes in this file seem to be formatting-only, maybe they could be committed separately?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

839

Actually you know what, I'm curious about some IRL data on this.

With uninitialized variable warnings this whole thing worked out really well because there are very few reasons to pass a pointer to an uninitialized local variable into a function, other than to initialize it. There's a lot more reasons to pass allocated memory into a function than to free it though. So I'm curious whether this is going to emit unnecessary notes in such cases and we'll have to tighten this heuristic.

[Szelethus]

Thanks! Here are some results:

All runs can be found here: https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs

Protobuf, Bitcoin, Xerces, TinyXML, PostgreSQL, FFMPEG, OpenSSL, Vim, Redis, Twin, curl:

Nothing changed.

libWebM:

Ignoring the fact that at both places where memory was allocated there is a // NOLINT comment, I think these bug reports have improved greatly. A function is highlighted where each of those memory region could have been added to a container, but were not. Not that I wrote any code to look for this, this is purely luck ;)

No1., No2.

SQLite:

This one isn't as great. I wouldn't expect a function called buildshifts to do any sort of dynamic memory management, and its contents support this claim. stp is only referencing to retrieve one of its members. More worrying is the fact that State_insert seems like the function (or macro, as they are not always capitalized in this project) to deal with this, yet it isn't noted at all in the bug report.

No1.

[Szelethus]

Maybe if I added this under an alpha checker option?

[Szelethus]

• Add the new feature behind a new, off-by-default alpha checker option.
• Restore test/Analysis/self-assign.cpp (to be formatted in a standalone commit).

If you don't mind, I'll commit this as is. We seem to agree on the general direction, and doesn't bother any users from now on.

[NoQ]

Committed as rG2d3668c997faac1f64cd3b8eb336af989069d135 (broken phabricator link).

Yes, great, thanks!, sounds like the right thing to do. I guess the next potential step is to set up syntactic analysis that would tell us whether the function *could have* deallocated memory or stored it for later deallocation. One trivial step would be to scan for free()/delete invocations with the parameter variable as an argument. That would probably turn the feature into a strict improvement that we can turn on by default. The next step would be to figure out what could constitute an escape; a binding to non-local storage might be a good first approximation but i'm not sure how to cover potentially-escaping functions which is probably the most beneficial part.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 //  related headers and non-static functions.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AllocationState.h"
 #include "InterCheckerAPI.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/StoreRef.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <climits>
 #include <functional>
 #include <utility>
 
static LeakInfo getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
                                     CheckerContext &C);
 
   void HandleLeak(SymbolRef Sym, ExplodedNode *N, CheckerContext &C) const;
 
   /// Test if value in ArgVal equals to value in macro `ZERO_SIZE_PTR`.
   bool isArgZERO_SIZE_PTR(ProgramStateRef State, CheckerContext &C,
                           SVal ArgVal) const;
 };
+} // end anonymous namespace
+
+//===----------------------------------------------------------------------===//
+// Definition of NoOwnershipChangeVisitor.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+  SymbolRef Sym;
+  using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
+
+  // Collect which entities point to the allocated memory, and could be
+  // responsible for deallocating it.
+  class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
+    SymbolRef Sym;
+    OwnerSet &Owners;
+
+  public:
+    OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners)
+        : Sym(Sym), Owners(Owners) {}
+
+    bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region,
+                       SVal Val) override {
+      if (Val.getAsSymbol() == Sym)
+        Owners.insert(Region);
+      return true;
+    }
+  };
+
+protected:
+  OwnerSet getOwnersAtNode(const ExplodedNode *N) {
+    OwnerSet Ret;
+
+    ProgramStateRef State = N->getState();
+    OwnershipBindingsHandler Handler{Sym, Ret};
+    State->getStateManager().getStoreManager().iterBindings(State->getStore(),
+                                                            Handler);
+    return Ret;
+  }
+
+  static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
+    while (N && !N->getLocationAs<CallExitEnd>())
+      N = N->getFirstSucc();
+    return N;
+  }
+
+  virtual bool
+  wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+                            const ExplodedNode *CallExitN) override {
+    if (CurrN->getLocationAs<CallEnter>())
+      return true;
+
+    // Its the state right *after* the call that is interesting. Any pointers
+    // inside the call that pointed to the allocated memory are of little
+    // consequence if their lifetime ends within the function.
+    CallExitN = getCallExitEnd(CallExitN);
+    if (!CallExitN)
+      return true;
+
+    if (CurrN->getState()->get<RegionState>(Sym) !=
+        CallExitN->getState()->get<RegionState>(Sym))
+      return true;
+
+    OwnerSet CurrOwners = getOwnersAtNode(CurrN);
+    OwnerSet ExitOwners = getOwnersAtNode(CallExitN);
+
+    // Owners in the current set may be purged from the analyzer later on.
+    // If a variable is dead (is not referenced directly or indirectly after
+    // some point), it will be removed from the Store before the end of its
+    // actual lifetime.
+    // This means that that if the ownership status didn't change, CurrOwners
+    // must be a superset of, but not necessarily equal to ExitOwners.
+    return !llvm::set_is_subset(ExitOwners, CurrOwners);
+  }
+
+  static PathDiagnosticPieceRef emitNote(const ExplodedNode *N) {
+    PathDiagnosticLocation L = PathDiagnosticLocation::create(
+        N->getLocation(),
+        N->getState()->getStateManager().getContext().getSourceManager());
+    return std::make_shared<PathDiagnosticEventPiece>(
+        L, "Returning without deallocating memory or storing the pointer for "
+           "later deallocation");
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+                           const ObjCMethodCall &Call,
+                           const ExplodedNode *N) override {
+    // TODO: Implement.
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+                          const CXXConstructorCall &Call,
+                          const ExplodedNode *N) override {
+    // TODO: Implement.
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+                             const ExplodedNode *N) override {
+    // TODO: Factor the logic of "what constitutes as an entity being passed
+    // into a function call" out by reusing the code in
+    // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating
+    // the printing technology in UninitializedObject's FieldChainInfo.
+    ArrayRef<ParmVarDecl *> Parameters = Call.parameters();
+    for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) {
+      SVal V = Call.getArgSVal(I);
+      if (V.getAsSymbol() == Sym)
+        return emitNote(N);
+    }
+    return nullptr;
+  }
+
+public:
+  NoOwnershipChangeVisitor(SymbolRef Sym)
+      : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough),
+        Sym(Sym) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int Tag = 0;
+    ID.AddPointer(&Tag);
+    ID.AddPointer(Sym);
+  }
+
+  void *getTag() const {
+    static int Tag = 0;
+    return static_cast<void *>(&Tag);
+  }
+};
+
+} // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Definition of MallocBugVisitor.
 //===----------------------------------------------------------------------===//
 
+namespace {
 /// The bug visitor which allows us to print extra diagnostics along the
 /// BugReport path. For example, showing the allocation site of the leaked
 /// region.
 class MallocBugVisitor final : public BugReporterVisitor {
 protected:
   enum NotificationMode { Normal, ReallocationFailed };
 
   // The allocated region symbol tracked by the main analysis.
std::string getMessageForArg(const Expr *ArgE, unsigned ArgIndex) override {
       return std::string(os.str());
     }
 
     std::string getMessageForReturn(const CallExpr *CallExpr) override {
       return "Reallocation of returned value failed";
     }
   };
 };
-
 } // end anonymous namespace
 
 // A map from the freed symbol to the symbol representing the return value of
 // the free function.
 REGISTER_MAP_WITH_PROGRAMSTATE(FreeReturnValue, SymbolRef, SymbolRef)
 
 namespace {
 class StopTrackingCallback final : public SymbolVisitor {
if (Region && Region->canPrintPretty()) {
     os << "Potential memory leak";
   }
 
   auto R = std::make_unique<PathSensitiveBugReport>(
       *BT_Leak[*CheckKind], os.str(), N, LocUsedForUniqueing,
       AllocNode->getLocationContext()->getDecl());
   R->markInteresting(Sym);
   R->addVisitor<MallocBugVisitor>(Sym, true);
+  R->addVisitor<NoOwnershipChangeVisitor>(Sym);
   C.emitReport(std::move(R));
 }
 
 void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                      CheckerContext &C) const
 {
   ProgramStateRef state = C.getState();
   RegionStateTy OldRS = state->get<RegionState>();

clang/test/Analysis/NewDeleteLeaks.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,unix -verify -analyzer-output=text %s
+
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+//===----------------------------------------------------------------------===//
+// Report for which we expect NoOwnershipChangeVisitor to add a new note.
+//===----------------------------------------------------------------------===//
+
+bool coin();
+
+namespace memory_allocated_in_fn_call {
+
+void sink(int *P) {
+} // expected-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  sink(new int(5)); // expected-note {{Memory is allocated}}
+                    // expected-note@-1 {{Calling 'sink'}}
+                    // expected-note@-2 {{Returning from 'sink'}}
+} // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential memory leak}}
+
+} // namespace memory_allocated_in_fn_call
+
+namespace memory_passed_to_fn_call {
+
+void sink(int *P) {
+  if (coin()) // expected-note {{Assuming the condition is false}}
+              // expected-note@-1 {{Taking false branch}}
+    delete P;
+} // expected-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);             // expected-note {{Calling 'sink'}}
+                         // expected-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_shorter_lifetime {
+
+void sink(int *P) {
+  int *Q = P;
+  if (coin()) // expected-note {{Assuming the condition is false}}
+              // expected-note@-1 {{Taking false branch}}
+    delete P;
+  (void)Q;
+} // expected-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);             // expected-note {{Calling 'sink'}}
+                         // expected-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_shorter_lifetime
+
+//===----------------------------------------------------------------------===//
+// Report for which we *do not* expect NoOwnershipChangeVisitor add a new note,
+// nor do we want it to.
+//===----------------------------------------------------------------------===//
+
+namespace memory_not_passed_to_fn_call {
+
+void sink(int *P) {
+  if (coin())
+    delete P;
+}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  int *q = nullptr;
+  sink(q);
+  (void)ptr;
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_not_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_same_lifetime {
+
+void sink(int *P, int **Q) {
+  // NOTE: Not a job of NoOwnershipChangeVisitor, but maybe this could be
+  // highlighted still?
+  *Q = P;
+}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  int *q = nullptr;
+  sink(ptr, &q);
+} // expected-warning {{Potential leak of memory pointed to by 'q' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_same_lifetime
+
+// TODO: We don't want a note here. sink() doesn't seem like a function that
+// even attempts to take care of any memory ownership problems.
+namespace memory_passed_into_fn_that_doesnt_intend_to_free {
+
+void sink(int *P) {
+} // expected-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);             // expected-note {{Calling 'sink'}}
+                         // expected-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_into_fn_that_doesnt_intend_to_free
+
+namespace refkind_from_unoallocated_to_allocated {
+
+// RefKind of the symbol changed from nothing to Allocated. We don't want to
+// emit notes when the RefKind changes in the stack frame.
+static char *malloc_wrapper_ret() {
+  return (char *)malloc(12); // expected-note {{Memory is allocated}}
+}
+void use_ret() {
+  char *v;
+  v = malloc_wrapper_ret(); // expected-note {{Calling 'malloc_wrapper_ret'}}
+                            // expected-note@-1 {{Returned allocated memory}}
+} // expected-warning {{Potential leak of memory pointed to by 'v' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak of memory pointed to by 'v'}}
+
+} // namespace refkind_from_unoallocated_to_allocated

clang/test/Analysis/self-assign.cpp

-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,unix.Malloc,debug.ExprInspection -analyzer-config eagerly-assume=false %s -verify -analyzer-output=text
+// RUN: %clang_analyze_cc1 -std=c++11 %s -verify -analyzer-output=text \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false
 
 extern "C" char *strdup(const char* s);
 extern "C" void free(void* ptr);
 
 namespace std {
 template<class T> struct remove_reference      { typedef T type; };
 template<class T> struct remove_reference<T&>  { typedef T type; };
 template<class T> struct remove_reference<T&&> { typedef T type; };

 private:
   char *str;
 };
 
 StringUsed::~StringUsed() {
   free(str);
 }
 
-StringUsed& StringUsed::operator=(const StringUsed &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
-  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUsed &StringUsed::operator=(const StringUsed &rhs) {
+  // expected-note@-1{{Assuming rhs == *this}}
+  // expected-note@-2{{Assuming rhs == *this}}
+  // expected-note@-3{{Assuming rhs != *this}}
+  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+                                     // expected-warning@-1{{UNKNOWN}}
+                                     // expected-note@-2{{TRUE}}
+                                     // expected-note@-3{{UNKNOWN}}
   free(str); // expected-note{{Memory is released}}
-  str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}  expected-note{{Use of memory after it is freed}}
-// expected-note@-1{{Memory is allocated}}
+  str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}
+                         // expected-note@-1{{Use of memory after it is freed}}
+                         // expected-note@-2{{Memory is allocated}}
   return *this;
 }
 
-StringUsed& StringUsed::operator=(StringUsed &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
-  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUsed &StringUsed::operator=(StringUsed &&rhs) {
+  // expected-note@-1{{Assuming rhs == *this}}
+  // expected-note@-2{{Assuming rhs != *this}}
+  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+                                     // expected-warning@-1{{UNKNOWN}}
+                                     // expected-note@-2{{TRUE}}
+                                     // expected-note@-3{{UNKNOWN}}
   str = rhs.str;
-  rhs.str = nullptr; // expected-warning{{Potential memory leak}} expected-note{{Potential memory leak}}
+  rhs.str = nullptr; // expected-warning{{Potential memory leak}}
+                     // expected-note@-1{{Potential memory leak}}
   return *this;
 }
 
 StringUsed::operator const char*() const {
   return str;
 }
 
 class StringUnused {
 public:
   StringUnused(const char *s = "") : str(strdup(s)) {}
   StringUnused(const StringUnused &rhs) : str(strdup(rhs.str)) {}
   ~StringUnused();
   StringUnused& operator=(const StringUnused &rhs);
   StringUnused& operator=(StringUnused &&rhs);
   operator const char*() const;
 private:
   char *str;
 };
 
 StringUnused::~StringUnused() {
   free(str);
 }
 
-StringUnused& StringUnused::operator=(const StringUnused &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
-  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUnused &StringUnused::operator=(const StringUnused &rhs) {
+  // expected-note@-1{{Assuming rhs == *this}}
+  // expected-note@-2{{Assuming rhs == *this}}
+  // expected-note@-3{{Assuming rhs != *this}}
+  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+                                     // expected-warning@-1{{UNKNOWN}}
+                                     // expected-note@-2{{TRUE}}
+                                     // expected-note@-3{{UNKNOWN}}
   free(str); // expected-note{{Memory is released}}
-  str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}  expected-note{{Use of memory after it is freed}}
+  str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}}
+                         // expected-note@-1{{Use of memory after it is freed}}
   return *this;
 }
 
-StringUnused& StringUnused::operator=(StringUnused &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
-  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
+StringUnused &StringUnused::operator=(StringUnused &&rhs) {
+  // expected-note@-1{{Assuming rhs == *this}}
+  // expected-note@-2{{Assuming rhs != *this}}
+  clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}}
+                                     // expected-warning@-1{{UNKNOWN}}
+                                     // expected-note@-2{{TRUE}}
+                                     // expected-note@-3{{UNKNOWN}}
   str = rhs.str;
   rhs.str = nullptr; // FIXME: An improved leak checker should warn here
   return *this;
 }
 
 StringUnused::operator const char*() const {
   return str;
 }
 
 
 int main() {
   StringUsed s1 ("test"), s2;
-  s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}} // expected-note{{Returned allocated memory}}
+  s2 = s1;            // expected-note{{Calling copy assignment operator for 'StringUsed'}}
+                      // expected-note@-1{{Returned allocated memory}}
   s2 = std::move(s1); // expected-note{{Calling move assignment operator for 'StringUsed'}}
   return 0;
 }