[analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it

[Szelethus]

Summary

D105553 added NoStateChangeFuncVisitor, an abstract class to aid in creating notes such as "Returning without writing to 'x'", or "Returning without changing the ownership status of allocated memory". Its clients need to define, among other things, what a change of state is.

For code like this:

f() {
  g();
}

foo() {
  f();
  h();
}

We'd have a path in the ExplodedGraph that looks like this:

             -- <g> -->
            /          \       
         ---     <f>    -------->        --- <h> --->
        /                        \      /            \
--------        <foo>             ------    <foo>     -->

When we're interested in whether f neglected to change some property, NoStateChangeFuncVisitor asks these questions:

                       ÷×~     
                -- <g> -->
           ß   /          \$    @&#*       
            ---     <f>    -------->        --- <h> --->
           /                        \      /            \
   --------        <foo>             ------    <foo>     -->

                           
Has anything changed in between # and *?
Has anything changed in between & and *?
Has anything changed in between @ and *?
...
Has anything changed in between $ and *?
Has anything changed in between × and ~?
Has anything changed in between ÷ and ~?
...
Has anything changed in between ß and *?
...

This is a rather thorough line of questioning, which is why in D105819, I was only interested in whether state *right before* and *right after* a function call changed, and early returned to the CallEnter location:

if (!CurrN->getLocationAs<CallEnter>())
  return;

Except that I made a typo, and forgot to negate the condition. So, in this patch, I'm fixing that, and under the same hood allow all clients to decide to do this whole-function check instead of the thorough one.

[Szelethus]

I also added new dump methods -- I had a trouble before realizing that CallEnter's stack frame is actually the *caller's*, not the *callee's* stack frame. Changes in findModifyingFrames aim to make the function more readable, and make sure that the correct stack frame will be marked as modifying.

[NoQ]

Would this work correctly when the property is changed but then reverted to its original state? This probably can't happen to MallocChecker (what has been freed cannot be unfreed) but it may happen to eg. PthreadLockChecker or to, well, Stores. In such cases it would be incorrect to say "returning without changing the state".

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

370

This is the right successor because we're in a heavily trimmed exploded graph, right?

[Szelethus]

In D108695#2967540, @NoQ wrote:

Would this work correctly when the property is changed but then reverted to its original state? This probably can't happen to MallocChecker (what has been freed cannot be unfreed) but it may happen to eg. PthreadLockChecker or to, well, Stores. In such cases it would be incorrect to say "returning without changing the state".

It should! But you have a point, I don't have the code to prove it right away. Maybe if I factored out the symbol part into NoStateChangeToSymbolVisitor, as teased in D105553#2864318, it'd have an easier time to see it. With that said, I don't currently see how the current implementation would be faulty, but even if it is, the patch adds an option, and doesn't the take old one (that NoStoreFuncVisitor still uses) away.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

370

Should be, yes.

[NoQ]

In D108695#2967885, @Szelethus wrote:

It should! But you have a point, I don't have the code to prove it right away. Maybe if I factored out the symbol part into NoStateChangeToSymbolVisitor, as teased in D105553#2864318, it'd have an easier time to see it. With that said, I don't currently see how the current implementation would be faulty, but even if it is, the patch adds an option, and doesn't the take old one (that NoStoreFuncVisitor still uses) away.

I mean, your implementation of findModifyingFrames() mostly boils down to:

return CallEnterN->getState()->get<State>(X) !=
     CallExitEndN->getState()->get<State>(X);

So if, say, X is a mutex and we're wondering whether it was unlocked in the function, but in reality it was unlocked and immediately locked again, then the above test would say "not modified" and you can't notice that it was modified temporarily without scanning all intermediate nodes. So if your note needs to make a distinction between "was not touched" and "was touched but reverted" you'll still need to do it the old way. I think this should be documented loudly in the comments.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

369

Wait, no, I don't think this test is sufficient. You may run into a CallExitEnd of a nested stack frame before you reach the CallExitEnd of the stack frame in question:

-> CallEnter foo()       --->
  -> CallEnter bar()        |
  <- CallExitEnd bar()      <--- ???
<- CallExitEnd foo()

[martong]

Nice work!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

361

Why don't we add the stack frame here to FramesModifyingCalculated as well? If we are in a nested stack frame and we step back to a parent then it would be redundant to do the calculation again for the parent.

Ahh, to be honest, I think the idea of having both FramesModifying and FramesModifyingCalculated separated is prone to errors.
Why don't we have a simple map<StackFramContext, bool> with the boolean true meaning that the frame is modifying? And if the frame is not in the map that means it had never been calculated before.

370

Would it make sense to assert that there are no more than one successor ?

400

Using a for could simplify this loop and would eliminate the need to have a redundant loop variable bump both at line 420 and at 393.

[martong]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

675

You have no such parameter. But I guess we can get that from either of them.

[Szelethus]

• Make sure that the matching CallExitEnd is retrieved.
• Add a unit test to neatly demonstrate how the visitor is intended to be used.
• Fix the rest of the nits.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

361

Yes, its stupid, but its just the one thing I didn't want to bother myself with. Though I agree it'd be better to drop these sets for something a lot more elegant. Btw mind that we insert into the calculated set each time we enter a new stack frame.

I'll leave a TODO, if you don't mind.

369

Good point. Added a bunch of asserts and changed to code to both retrieve, and ensure that the correct node is retrieved.

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

683–685

Whoops, this is wildly incorrect. Gotta fix that. The inlined function's stack frame is retrievable from the CallEnters first successor, or CallExitEnds first predecessor.

[martong]

Thanks! My concerns are addressed, looks good to me!

[thakis]

This seems to break tests: http://45.33.8.238/linux/54991/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[paquette]

I just reverted this again because of a bot failure here:

https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/23380/consoleFull

Please fix and recommit!

[uabelho]

Hi @Szelethus
A couple of tests fail for me on trunk with this patch:

Failed Tests (3):
  Clang-Unit :: StaticAnalyzer/./StaticAnalysisTests/FalsePositiveRefutationBRVisitorTestBase.UnSatAtErrorNodeDueToRefinedConstraintNoReport
  Clang-Unit :: StaticAnalyzer/./StaticAnalysisTests/FalsePositiveRefutationBRVisitorTestBase.UnSatAtErrorNodeWithNewSymbolNoReport
  Clang-Unit :: StaticAnalyzer/./StaticAnalysisTests/FalsePositiveRefutationBRVisitorTestBase.UnSatInTheMiddleNoReport

Details from one failure below:

seroius03977 [12:05] [uabelho/master-github/llvm] -> /repo/uabelho/master-github/llvm/build-all/tools/clang/unittests/StaticAnalyzer/./StaticAnalysisTests --gtest_filter=FalsePositiveRefutationBRVisitorTestBase.UnSatAtErrorNodeWithNewSymbolNoReport
Note: Google Test filter = FalsePositiveRefutationBRVisitorTestBase.UnSatAtErrorNodeWithNewSymbolNoReport
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FalsePositiveRefutationBRVisitorTestBase
[ RUN      ] FalsePositiveRefutationBRVisitorTestBase.UnSatAtErrorNodeWithNewSymbolNoReport
UnSatAtErrorNodeWithNewSymbolNoReport.cc:8:7: warning: REACHED_WITH_NO_CONTRADICTION [test.FalsePositiveGenerator]
      reachedWithNoContradiction();
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
/repo/uabelho/master-github/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:162: Failure
Expected equality of these values:
  Diags
    Which is: "test.FalsePositiveGenerator: Assuming the condition is false | REACHED_WITH_NO_CONTRADICTION\n"
  "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"
UnSatAtErrorNodeWithNewSymbolNoReport.cc:8:7: warning: REACHED_WITH_NO_CONTRADICTION [test.FalsePositiveGenerator]
      reachedWithNoContradiction();
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
UnSatAtErrorNodeWithNewSymbolNoReport.cc:9:7: warning: CAN_BE_TRUE [test.FalsePositiveGenerator]
      reportIfCanBeTrue(x == 0); // contradiction
      ^~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
/repo/uabelho/master-github/clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:171: Failure
Expected equality of these values:
  Diags2
    Which is: "test.FalsePositiveGenerator: Assuming the condition is false | REACHED_WITH_NO_CONTRADICTION\ntest.FalsePositiveGenerator: Assuming the condition is false | CAN_BE_TRUE\n"
  "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n" "test.FalsePositiveGenerator: CAN_BE_TRUE\n"
    Which is: "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\ntest.FalsePositiveGenerator: CAN_BE_TRUE\n"
With diff:
@@ -1,2 +1,2 @@
-test.FalsePositiveGenerator: Assuming the condition is false | REACHED_WITH_NO_CONTRADICTION
-test.FalsePositiveGenerator: Assuming the condition is false | CAN_BE_TRUE\n
+test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION
+test.FalsePositiveGenerator: CAN_BE_TRUE\n

[  FAILED  ] FalsePositiveRefutationBRVisitorTestBase.UnSatAtErrorNodeWithNewSymbolNoReport (41 ms)
[----------] 1 test from FalsePositiveRefutationBRVisitorTestBase (41 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (42 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] FalsePositiveRefutationBRVisitorTestBase.UnSatAtErrorNodeWithNewSymbolNoReport

 1 FAILED TEST

[Szelethus]

I'll attend to this ASAP. Thanks for the heads up!

[steakhal]

In D108695#2999378, @uabelho wrote:

Hi @Szelethus
A couple of tests fail for me on trunk with this patch

Uh, that's my unittest :D
I suspect you are running some sort of CI where you set up Z3.

I suspect we should slightly modify the expected output then.

[uabelho]

In D108695#2999432, @steakhal wrote:

In D108695#2999378, @uabelho wrote:

Hi @Szelethus
A couple of tests fail for me on trunk with this patch

Uh, that's my unittest :D
I suspect you are running some sort of CI where you set up Z3.

Yes, I have Z3 enabled.

[Szelethus]

rGfb4d590a622f4031900516360c07ee6ace01c5e6 should sort this out!

[uabelho]

In D108695#2999677, @Szelethus wrote:

rGfb4d590a622f4031900516360c07ee6ace01c5e6 should sort this out!

Yes now it works.
Thank you!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

 
 /// Put a diagnostic on return statement (or on } in its absence) of all inlined
 /// functions for which some property remained unchanged.
 /// Resulting diagnostics may read such as "Returning without writing to X".
 ///
 /// Descendants can define what a "state change is", like a change of value
 /// to a memory region, liveness, etc. For function calls where the state did
 /// not change as defined, a custom note may be constructed.
+///
+/// For a minimal example, check out
+/// clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp.
 class NoStateChangeFuncVisitor : public BugReporterVisitor {
 private:
   /// Frames modifying the state as defined in \c wasModifiedBeforeCallExit.
   /// This visitor generates a note only if a function does *not* change the
   /// state that way. 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.
+  // TODO: Can't we just use a map instead? This is likely not as cheap as it
+  // makes the code difficult to read.
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifying;
   llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
 
   /// Check and lazily calculate whether the state is modified in the stack
   /// frame to which \p CallExitBeginN belongs.
   /// The calculation is cached in FramesModifying.
   bool isModifiedInFrame(const ExplodedNode *CallExitBeginN);
 
+  void markFrameAsModifying(const StackFrameContext *SCtx);
+
   /// Write to \c FramesModifying all stack frames along the path in the current
   /// stack frame which modifies the state.
   void findModifyingFrames(const ExplodedNode *const CallExitBeginN);
 
 protected:
   bugreporter::TrackingKind TKind;
 
-  /// \return Whether the state was modified from the current node, \CurrN, to
-  /// the end of the stack fram, at \p CallExitBeginN.
-  virtual bool
-  wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
-                            const ExplodedNode *CallExitBeginN) = 0;
+  /// \return Whether the state was modified from the current node, \p CurrN, to
+  /// the end of the stack frame, at \p CallExitBeginN. \p CurrN and
+  /// \p CallExitBeginN are always in the same stack frame.
+  /// Clients should override this callback when a state change is important
+  /// not only on the entire function call, but inside of it as well.
+  /// Example: we may want to leave a note about the lack of locking/unlocking
+  /// on a particular mutex, but not if inside the function its state was
+  /// changed, but also restored. wasModifiedInFunction() wouldn't know of this
+  /// change.
+  virtual bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+                                         const ExplodedNode *CallExitBeginN) {
+    return false;
+  }
+
+  /// \return Whether the state was modified in the inlined function call in
+  /// between \p CallEnterN and \p CallExitEndN. Mind that the stack frame
+  /// retrieved from a CallEnterN and CallExitEndN is the *caller's* stack
+  /// frame! The inlined function's stack should be retrieved from either the
+  /// immediate successor to \p CallEnterN or immediate predecessor to
+  /// \p CallExitEndN.
+  /// Clients should override this function if a state changes local to the
+  /// inlined function are not interesting, only the change occuring as a
+  /// result of it.
+  /// Example: we want to leave a not about a leaked resource object not being
+  /// deallocated / its ownership changed inside a function, and we don't care
+  /// if it was assigned to a local variable (its change in ownership is
+  /// inconsequential).
+  virtual bool wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                                     const ExplodedNode *CallExitEndN) {
+    return false;
+  }
 
   /// Consume the information on the non-modifying stack frame in order to
   /// either emit a note or not. May suppress the report entirely.
   /// \return Diagnostics piece for the unmodified state in the current
   /// function, if it decides to emit one. A good description might start with
   /// "Returning without...".
   virtual PathDiagnosticPieceRef
   maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
public:
   NoStateChangeFuncVisitor(bugreporter::TrackingKind TKind) : TKind(TKind) {}
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
                                    BugReporterContext &BR,
                                    PathSensitiveBugReport &R) override final;
 };
 
 } // namespace ento
-
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 #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/Analysis/ProgramPoint.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/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
 #include <climits>
 #include <functional>
 #include <utility>
 
 using namespace clang;
 using namespace ento;
 using namespace std::placeholders;
 
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;
     }
+
+    LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); }
+    LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &out) const {
+      out << "Owners: {\n";
+      for (const MemRegion *Owner : Owners) {
+        out << "  ";
+        Owner->dumpToStream(out);
+        out << ",\n";
+      }
+      out << "}\n";
+    }
   };
 
 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;
+  LLVM_DUMP_METHOD static std::string
+  getFunctionName(const ExplodedNode *CallEnterN) {
+    if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>(
+            CallEnterN->getLocationAs<CallEnter>()->getCallExpr()))
+      if (const FunctionDecl *FD = CE->getDirectCallee())
+        return FD->getQualifiedNameAsString();
+    return "";
   }
 
   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))
+  wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                        const ExplodedNode *CallExitEndN) override {
+    if (CallEnterN->getState()->get<RegionState>(Sym) !=
+        CallExitEndN->getState()->get<RegionState>(Sym))
       return true;
 
-    OwnerSet CurrOwners = getOwnersAtNode(CurrN);
-    OwnerSet ExitOwners = getOwnersAtNode(CallExitN);
+    OwnerSet CurrOwners = getOwnersAtNode(CallEnterN);
+    OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN);
 
     // 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);

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 bool NoStateChangeFuncVisitor::isModifiedInFrame(const ExplodedNode *N) {
   const LocationContext *Ctx = N->getLocationContext();
   const StackFrameContext *SCtx = Ctx->getStackFrame();
   if (!FramesModifyingCalculated.count(SCtx))
     findModifyingFrames(N);
   return FramesModifying.count(SCtx);
 }
 
+void NoStateChangeFuncVisitor::markFrameAsModifying(
+    const StackFrameContext *SCtx) {
+  while (!SCtx->inTopFrame()) {
+    auto p = FramesModifying.insert(SCtx);
+    if (!p.second)
+      break; // Frame and all its parents already inserted.
+
+    SCtx = SCtx->getParent()->getStackFrame();
+  }
+}
+
+static const ExplodedNode *getMatchingCallExitEnd(const ExplodedNode *N) {
+  assert(N->getLocationAs<CallEnter>());
+  // The stackframe of the callee is only found in the nodes succeeding
+  // the CallEnter node. CallEnter's stack frame refers to the caller.
+  const StackFrameContext *OrigSCtx = N->getFirstSucc()->getStackFrame();
+
+  // Similarly, the nodes preceding CallExitEnd refer to the callee's stack
+  // frame.
+  auto IsMatchingCallExitEnd = [OrigSCtx](const ExplodedNode *N) {
+    return N->getLocationAs<CallExitEnd>() &&
+           OrigSCtx == N->getFirstPred()->getStackFrame();
+  };
+  while (N && !IsMatchingCallExitEnd(N)) {
+    assert(N->succ_size() <= 1 &&
+           "This function is to be used on the trimmed ExplodedGraph!");
+    N = N->getFirstSucc();
+  }
+  return N;
+}
+
 void NoStateChangeFuncVisitor::findModifyingFrames(
     const ExplodedNode *const CallExitBeginN) {
 
   assert(CallExitBeginN->getLocationAs<CallExitBegin>());
-  const ExplodedNode *LastReturnN = CallExitBeginN;
+
   const StackFrameContext *const OriginalSCtx =
       CallExitBeginN->getLocationContext()->getStackFrame();
 
-  const ExplodedNode *CurrN = CallExitBeginN;
+  const ExplodedNode *CurrCallExitBeginN = CallExitBeginN;
+  const StackFrameContext *CurrentSCtx = OriginalSCtx;
 
-  do {
-    ProgramStateRef State = CurrN->getState();
-    auto CallExitLoc = CurrN->getLocationAs<CallExitBegin>();
-    if (CallExitLoc) {
-      LastReturnN = CurrN;
+  for (const ExplodedNode *CurrN = CallExitBeginN; CurrN;
+       CurrN = CurrN->getFirstPred()) {
+    // Found a new inlined call.
+    if (CurrN->getLocationAs<CallExitBegin>()) {
+      CurrCallExitBeginN = CurrN;
+      CurrentSCtx = CurrN->getStackFrame();
+      FramesModifyingCalculated.insert(CurrentSCtx);
+      // We won't see a change in between two identical exploded nodes: skip.
+      continue;
     }
 
-    FramesModifyingCalculated.insert(
-        CurrN->getLocationContext()->getStackFrame());
+    if (auto CE = CurrN->getLocationAs<CallEnter>()) {
+      if (const ExplodedNode *CallExitEndN = getMatchingCallExitEnd(CurrN))
+        if (wasModifiedInFunction(CurrN, CallExitEndN))
+          markFrameAsModifying(CurrentSCtx);
 
-    if (wasModifiedBeforeCallExit(CurrN, LastReturnN)) {
-      const StackFrameContext *SCtx = CurrN->getStackFrame();
-      while (!SCtx->inTopFrame()) {
-        auto p = FramesModifying.insert(SCtx);
-        if (!p.second)
-          break; // Frame and all its parents already inserted.
-        SCtx = SCtx->getParent()->getStackFrame();
-      }
-    }
+      // We exited this inlined call, lets actualize the stack frame.
+      CurrentSCtx = CurrN->getStackFrame();
 
-    // Stop calculation at the call to the current function.
-    if (auto CE = CurrN->getLocationAs<CallEnter>())
-      if (CE->getCalleeContext() == OriginalSCtx)
+      // Stop calculating at the current function, but always regard it as
+      // modifying, so we can avoid notes like this:
+      //   void f(Foo &F) {
+      //     F.field = 0; // note: 0 assigned to 'F.field'
+      //                  // note: returning without writing to 'F.field'
+      //   }
+      if (CE->getCalleeContext() == OriginalSCtx) {
+        markFrameAsModifying(CurrentSCtx);
         break;
+      }
+    }
 
-    CurrN = CurrN->getFirstPred();
-  } while (CurrN);
+    if (wasModifiedBeforeCallExit(CurrN, CurrCallExitBeginN))
+      markFrameAsModifying(CurrentSCtx);
+  }
 }
 
 PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode(
     const ExplodedNode *N, BugReporterContext &BR, PathSensitiveBugReport &R) {
 
   const LocationContext *Ctx = N->getLocationContext();
   const StackFrameContext *SCtx = Ctx->getStackFrame();
   ProgramStateRef State = N->getState();

clang/unittests/StaticAnalyzer/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
   BugReportInterestingnessTest.cpp
   CallDescriptionTest.cpp
   CallEventTest.cpp
   FalsePositiveRefutationBRVisitorTest.cpp
+  NoStateChangeFuncVisitorTest.cpp
   ParamRegionTest.cpp
   RangeSetTest.cpp
   RegisterCustomCheckersTest.cpp
   StoreTest.cpp
   SymbolReaperTest.cpp
   SValTest.cpp
   TestReturnValueUnderConstruction.cpp
   )

clang/unittests/StaticAnalyzer/CallEventTest.cpp

EXPECT_TRUE(runCheckerOnCode<addCXXDeallocatorChecker>(R"(
     struct A {};
 
     void f() {
       A *a = new A;
       delete a;
     }
   )",
                                                          Diags));
-  EXPECT_EQ(Diags, "test.CXXDeallocator:NumArgs: 1\n");
+  EXPECT_EQ(Diags, "test.CXXDeallocator: NumArgs: 1\n");
 }
 
 } // namespace
 } // namespace ento
 } // namespace clang

clang/unittests/StaticAnalyzer/CheckerRegistration.h

 //===- unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp ------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace ento {
 
 class DiagConsumer : public PathDiagnosticConsumer {
   llvm::raw_ostream &Output;
 
 public:
   DiagConsumer(llvm::raw_ostream &Output) : Output(Output) {}
   void FlushDiagnosticsImpl(std::vector<const PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
-    for (const auto *PD : Diags)
-      Output << PD->getCheckerName() << ":" << PD->getShortDescription() << '\n';
+    for (const auto *PD : Diags) {
+      Output << PD->getCheckerName() << ": ";
+
+      for (PathDiagnosticPieceRef Piece :
+           PD->path.flatten(/*ShouldFlattenMacros*/ true)) {
+        if (Piece->getKind() != PathDiagnosticPiece::Event)
+          continue;
+        if (Piece->getString().empty())
+          continue;
+        // The last event is usually the same as the warning message, skip.
+        if (Piece->getString() == PD->getShortDescription())
+          continue;
+
+        Output << Piece->getString() << " | ";
+      }
+      Output << PD->getShortDescription() << '\n';
+    }
   }
 
   StringRef getName() const override { return "Test"; }
 };
 
 using AddCheckerFn = void(AnalysisASTConsumer &AnalysisConsumer,
                           AnalyzerOptions &AnOpts);
 

clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

void test(int x, int y) {
          // x * y != 0  =>  x != 0 && y != 0  => contradict with x == 0
        }
      })";
 
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeAndCrossCheckArgs, Diags));
   EXPECT_EQ(Diags,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n");
   // Single warning. The second report was invalidated by the visitor.
 
   // Without enabling the crosscheck-with-z3 both reports are displayed.
   std::string Diags2;
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeArgs, Diags2));
   EXPECT_EQ(Diags2,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
-            "test.FalsePositiveGenerator:REACHED_WITH_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"
+            "test.FalsePositiveGenerator: REACHED_WITH_CONTRADICTION\n");
 }
 
 TEST_F(FalsePositiveRefutationBRVisitorTestBase,
        UnSatAtErrorNodeWithNewSymbolNoReport) {
   constexpr auto Code = R"(
     void reportIfCanBeTrue(bool);
     void reachedWithNoContradiction();
     void test(int x, int y) {
       if (x * y == 0)
        return;
       // We know that 'x * y': {[MIN,-1], [1,MAX]}
       reachedWithNoContradiction();
       reportIfCanBeTrue(x == 0); // contradiction
       // The function introduces the 'x == 0' constraint in the ErrorNode which
       // leads to contradiction with the constraint of 'x * y'.
       // Note that the new constraint was bound to a new symbol 'x'.
     })";
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeAndCrossCheckArgs, Diags));
   EXPECT_EQ(Diags,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n");
   // Single warning. The second report was invalidated by the visitor.
 
   // Without enabling the crosscheck-with-z3 both reports are displayed.
   std::string Diags2;
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeArgs, Diags2));
   EXPECT_EQ(Diags2,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
-            "test.FalsePositiveGenerator:CAN_BE_TRUE\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"
+            "test.FalsePositiveGenerator: CAN_BE_TRUE\n");
 }
 
 TEST_F(FalsePositiveRefutationBRVisitorTestBase,
        UnSatAtErrorNodeDueToRefinedConstraintNoReport) {
   constexpr auto Code = R"(
     void reportIfCanBeTrue(bool);
     void reachedWithNoContradiction();
     void test(unsigned x, unsigned n) {
void test(unsigned x, unsigned n) {
         }
       }
     })";
 
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeAndCrossCheckArgs, Diags));
   EXPECT_EQ(Diags,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n");
   // Single warning. The second report was invalidated by the visitor.
 
   // Without enabling the crosscheck-with-z3 both reports are displayed.
   std::string Diags2;
   EXPECT_TRUE(runCheckerOnCodeWithArgs<addFalsePositiveGenerator>(
       Code, LazyAssumeArgs, Diags2));
   EXPECT_EQ(Diags2,
-            "test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
-            "test.FalsePositiveGenerator:CAN_BE_TRUE\n");
+            "test.FalsePositiveGenerator: REACHED_WITH_NO_CONTRADICTION\n"
+            "test.FalsePositiveGenerator: CAN_BE_TRUE\n");
 }
 
 } // namespace
 } // namespace ento
 } // namespace clang

clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp

+//===- unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CheckerRegistration.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.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/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include <memory>
+
+//===----------------------------------------------------------------------===//
+// Base classes for testing NoStateChangeFuncVisitor.
+//
+// Testing is done by observing a very simple trait change from one node to
+// another -- the checker sets the ErrorPrevented trait to true if
+// 'preventError()' is called in the source code, and sets it to false if
+// 'allowError()' is called. If this trait is false when 'error()' is called,
+// a warning is emitted.
+//
+// The checker then registers a simple NoStateChangeFuncVisitor to add notes to
+// inlined functions that could have, but neglected to prevent the error.
+//===----------------------------------------------------------------------===//
+
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ErrorPrevented, bool)
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ErrorNotPreventedFuncVisitor : public NoStateChangeFuncVisitor {
+public:
+  ErrorNotPreventedFuncVisitor()
+      : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough) {}
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+                           const ObjCMethodCall &Call,
+                           const ExplodedNode *N) override {
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+                          const CXXConstructorCall &Call,
+                          const ExplodedNode *N) override {
+    return nullptr;
+  }
+
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+                             const ExplodedNode *N) override {
+    PathDiagnosticLocation L = PathDiagnosticLocation::create(
+        N->getLocation(),
+        N->getState()->getStateManager().getContext().getSourceManager());
+    return std::make_shared<PathDiagnosticEventPiece>(
+        L, "Returning without prevening the error");
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int Tag = 0;
+    ID.AddPointer(&Tag);
+  }
+};
+
+template <class Visitor>
+class StatefulChecker : public Checker<check::PreCall> {
+  mutable std::unique_ptr<BugType> BT;
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+    if (Call.isCalled(CallDescription{"preventError", 0})) {
+      C.addTransition(C.getState()->set<ErrorPrevented>(true));
+      return;
+    }
+
+    if (Call.isCalled(CallDescription{"allowError", 0})) {
+      C.addTransition(C.getState()->set<ErrorPrevented>(false));
+      return;
+    }
+
+    if (Call.isCalled(CallDescription{"error", 0})) {
+      if (C.getState()->get<ErrorPrevented>())
+        return;
+      const ExplodedNode *N = C.generateErrorNode();
+      if (!N)
+        return;
+      if (!BT)
+        BT.reset(new BugType(this->getCheckerName(), "error()",
+                             categories::SecurityError));
+      auto R =
+          std::make_unique<PathSensitiveBugReport>(*BT, "error() called", N);
+      R->template addVisitor<Visitor>();
+      C.emitReport(std::move(R));
+    }
+  }
+};
+
+} // namespace
+} // namespace ento
+} // namespace clang
+
+//===----------------------------------------------------------------------===//
+// Non-thorough analysis: only the state right before and right after the
+// function call is checked for the difference in trait value.
+//===----------------------------------------------------------------------===//
+
+namespace clang {
+namespace ento {
+namespace {
+
+class NonThoroughErrorNotPreventedFuncVisitor
+    : public ErrorNotPreventedFuncVisitor {
+public:
+  virtual bool
+  wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                        const ExplodedNode *CallExitEndN) override {
+    return CallEnterN->getState()->get<ErrorPrevented>() !=
+           CallExitEndN->getState()->get<ErrorPrevented>();
+  }
+};
+
+void addNonThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer,
+                                   AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry
+        .addChecker<StatefulChecker<NonThoroughErrorNotPreventedFuncVisitor>>(
+            "test.StatefulChecker", "Description", "");
+  });
+}
+
+TEST(NoStateChangeFuncVisitor, NonThoroughFunctionAnalysis) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode<addNonThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      //preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags,
+            "test.StatefulChecker: Calling 'g' | Returning without prevening "
+            "the error | Returning from 'g' | error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addNonThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+      allowError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags,
+            "test.StatefulChecker: Calling 'g' | Returning without prevening "
+            "the error | Returning from 'g' | error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addNonThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags, "");
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
+
+//===----------------------------------------------------------------------===//
+// Thorough analysis: only the state right before and right after the
+// function call is checked for the difference in trait value.
+//===----------------------------------------------------------------------===//
+
+namespace clang {
+namespace ento {
+namespace {
+
+class ThoroughErrorNotPreventedFuncVisitor
+    : public ErrorNotPreventedFuncVisitor {
+public:
+  virtual bool
+  wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+                            const ExplodedNode *CallExitBeginN) override {
+    return CurrN->getState()->get<ErrorPrevented>() !=
+           CallExitBeginN->getState()->get<ErrorPrevented>();
+  }
+};
+
+void addThoroughStatefulChecker(AnalysisASTConsumer &AnalysisConsumer,
+                                AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.StatefulChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<StatefulChecker<ThoroughErrorNotPreventedFuncVisitor>>(
+        "test.StatefulChecker", "Description", "");
+  });
+}
+
+TEST(NoStateChangeFuncVisitor, ThoroughFunctionAnalysis) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode<addThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      //preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags,
+            "test.StatefulChecker: Calling 'g' | Returning without prevening "
+            "the error | Returning from 'g' | error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+      allowError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags, "test.StatefulChecker: error() called\n");
+
+  Diags.clear();
+
+  EXPECT_TRUE(runCheckerOnCode<addThoroughStatefulChecker>(R"(
+    void error();
+    void preventError();
+    void allowError();
+
+    void g() {
+      preventError();
+    }
+
+    void f() {
+      g();
+      error();
+    }
+  )", Diags));
+  EXPECT_EQ(Diags, "");
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang

clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

void addCustomChecker(AnalysisASTConsumer &AnalysisConsumer,
   AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
     Registry.addChecker<CustomChecker>("test.CustomChecker", "Description", "");
   });
 }
 
 TEST(RegisterCustomCheckers, RegisterChecker) {
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCode<addCustomChecker>("void f() {;}", Diags));
-  EXPECT_EQ(Diags, "test.CustomChecker:Custom diagnostic description\n");
+  EXPECT_EQ(Diags, "test.CustomChecker: Custom diagnostic description\n");
 }
 
 //===----------------------------------------------------------------------===//
 // Pretty much the same.
 //===----------------------------------------------------------------------===//
 
 class LocIncDecChecker : public Checker<check::Location> {
 public:
AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
     addCheckerRegistrationOrderPrinter(Registry);
     Registry.addDependency("test.Dep", "test.Strong");
   });
 }
 
 TEST(RegisterDeps, UnsatisfiedDependency) {
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCode<addDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.RegistrationOrder\n");
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.RegistrationOrder\n");
 }
 
 //===----------------------------------------------------------------------===//
 // Weak checker dependencies.
 //===----------------------------------------------------------------------===//
 
 UNITTEST_CHECKER(WeakDep, "Weak")
 
AnalysisConsumer.AddCheckerRegistrationFn([=](CheckerRegistry &Registry) {
     Registry.addWeakDependency("test.WeakDep", "test.WeakDep2");
   });
 }
 
 TEST(RegisterDeps, SimpleWeakDependency) {
   std::string Diags;
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerBothEnabled>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep\ntest."
                    "Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // Mind that AnalyzerOption listed the enabled checker list in the same order,
   // but the dependencies are switched.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerBothEnabledSwitched>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.Dep\ntest."
                    "RegistrationOrder\ntest.WeakDep\n");
   Diags.clear();
 
   // Weak dependencies dont prevent dependent checkers from being enabled.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerDepDisabled>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // Nor will they be enabled just because a dependent checker is.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepCheckerDepUnspecified>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepTransitivity>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep2\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep2\ntest."
                    "Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepHasWeakDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep2\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep2\ntest."
                    "WeakDep\ntest.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 }
 
 //===----------------------------------------------------------------------===//
 // Interaction of weak and regular checker dependencies.
 //===----------------------------------------------------------------------===//
 
AnalysisConsumer.AddCheckerRegistrationFn([=](CheckerRegistry &Registry) {
     Registry.addWeakDependency("test.Dep", "test.WeakDep");
   });
 }
 
 TEST(RegisterDeps, DependencyInteraction) {
   std::string Diags;
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepHasStrongDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.StrongDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.StrongDep\ntest."
                    "WeakDep\ntest.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // Weak dependencies are registered before strong dependencies. This is most
   // important for purely diagnostic checkers that are implemented as a part of
   // purely modeling checkers, becuse the checker callback order will have to be
   // established in between the modeling portion and the weak dependency.
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepAndStrongDep>("void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.WeakDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.WeakDep\ntest."
                    "StrongDep\ntest.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // If a weak dependency is disabled, the checker itself can still be enabled.
   EXPECT_TRUE(runCheckerOnCode<addDisabledWeakDepHasStrongDep>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.Dep\ntest."
                    "RegistrationOrder\ntest.StrongDep\n");
   Diags.clear();
 
   // If a weak dependency is disabled, the checker itself can still be enabled,
   // but it shouldn't enable a strong unspecified dependency.
   EXPECT_TRUE(runCheckerOnCode<addDisabledWeakDepHasUnspecifiedStrongDep>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   // A strong dependency of a weak dependency is disabled, so neither of them
   // should be enabled.
   EXPECT_TRUE(runCheckerOnCode<addWeakDepHasDisabledStrongDep>(
       "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.Dep\ntest.RegistrationOrder\n");
+  EXPECT_EQ(Diags,
+            "test.RegistrationOrder: test.Dep\ntest.RegistrationOrder\n");
   Diags.clear();
 
   EXPECT_TRUE(
       runCheckerOnCode<addWeakDepHasUnspecifiedButLaterEnabledStrongDep>(
           "void f() {int i;}", Diags));
-  EXPECT_EQ(Diags, "test.RegistrationOrder:test.StrongDep\ntest.WeakDep\ntest."
+  EXPECT_EQ(Diags, "test.RegistrationOrder: test.StrongDep\ntest.WeakDep\ntest."
                    "Dep\ntest.Dep2\ntest.RegistrationOrder\n");
   Diags.clear();
 }
 } // namespace
 } // namespace ento
 } // namespace clang