[analyzer][WIP] Suppress bug reports where a tracked expression's latest value change was a result of an invalidation

[Szelethus]

Summary

You may be familiar with reports that are demonstrated in the test file -- While invalidation is a fundamental part of static analysis, it is unfortunately not an under-approximation (resulting in fewer but more precise paths of execution). Invalidation are often incorrect, so this patch attempts to suppress reports where a tracked expression's last value change was a result of an invalidation.

This is solved by a adding a new checker that notes which regions at which ProgramPoint were invalidated, and adding a new BugReporterVisitor to the army of trackExpressionValue related visitors that looks for the last value change, and suppresses the report if it was a previously noted invalidation.

The big scare in such changes is always whether we would suppress so many things, so this patch says little without results. I'll share some as soon as I have them :)

[NoQ]

In my head this patch should ideally be reduced to a single if-statement: "This value is a SymbolDerived therefore it was produced by invalidation".

It's harder than that, of course, because some derived symbols are legitimate (i.e., values returned through out-parameters). But this is a separate problem: instead of producing anonymous conjured symbols, invalidation should produce richer symbols that explain what kind of invalidation has happened and which specific effect of this invalidation is represented by this symbol. Or we could instead make better symbols for representing out-parameters.

[Szelethus]

In D75698#1908335, @NoQ wrote:

In my head this patch should ideally be reduced to a single if-statement: "This value is a SymbolDerived therefore it was produced by invalidation".

Getting to this point already forced me to learn a lot about the inner workings of the analyzer, and I suspect I have quite a bit of work ahead of me still. Infact, I'm not even sure what SymbolDerived is. I suspect this approach should be abandoned then?

It's harder than that, of course, because some derived symbols are legitimate (i.e., values returned through out-parameters). But this is a separate problem: instead of producing anonymous conjured symbols, invalidation should produce richer symbols that explain what kind of invalidation has happened and which specific effect of this invalidation is represented by this symbol. Or we could instead make better symbols for representing out-parameters.

Alright, I've got some learning to then :) Thanks!

[NoQ]

Like, the only reason why we have trackExpressionValue is because our concrete values are indistinguishable from each other. Regardless of where it comes from, 0 (Loc) is the same as all other 0 (Loc)s, so we have to manually observe how it was copied around in order to figure out where it comes from.

Symbols, by design, have their origin as their identity. Whenever we encounter an unknown runtime value, we denote it with a symbol, therefore the symbol by construction has all the information about that runtime value: how it appeared, why it's unknown, and so on.

Because invalidation produces symbols and not concrete values (as it destroys information about the program state, while concrete values are such information), this patch doesn't need to have anything to do with value tracking.

[NoQ]

In D75698#1908370, @Szelethus wrote:

In D75698#1908335, @NoQ wrote:

In my head this patch should ideally be reduced to a single if-statement: "This value is a SymbolDerived therefore it was produced by invalidation".

Getting to this point already forced me to learn a lot about the inner workings of the analyzer, and I suspect I have quite a bit of work ahead of me still. Infact, I'm not even sure what SymbolDerived is.

It's a chunk of a bigger symbol. Like, if we already have a symbol for an unknown value of a structure written in a certain region, then the symbol for a field of this structure would be a derived symbol from that first symbol for the field region.

Now, the only reason we have symbols for structures (or sometimes even for entire memory spaces) is because of invalidation. What remains is to tweak the patch in order to account for different kinds of invalidation.

[Szelethus]

This sounds exciting! Back to work then.

[martong]

While invalidation is a fundamental part of static analysis, it is unfortunately not an under-approximation (resulting in fewer but more precise paths of execution)

+1 for saying this out :)

[NoQ]

In D75698#1909541, @martong wrote:

While invalidation is a fundamental part of static analysis, it is unfortunately not an under-approximation (resulting in fewer but more precise paths of execution)

+1 for saying this out :)

It would have been a correct under-approximation assuming that other parts of the static analyzer were working correctly. The reason why invalidation causes false positives is because we're making state splits on every if-statement and never join back, which causes us to explore more execution paths than we're entitled to by the presumption of no dead code. Eg.,

class C {
  bool x;

  void do_not_change_x();

  void foo() {
    if (x) { // assuming 'x' is true
      do_not_change_x(); // 'x' is invalidated
    }

    // We are not entitled to a state split here
    // over the invalidated value of 'x'.
    if (x) {
      ...
    }
  }
};

Normally such eager state splits don't cause any problems because we tell the users to "simply add an assert, the code is obscure anyway". But when invalidation kicks in, like in this example, such asserts become very ugly. I.e., in this case you'd have to do something like this to suppress the warning and you'll have to do it every time you call do_not_change_x() for every variable that it doesn't change:

bool old_x = x;
do_not_change_x();
assert(x == old_x && "x changed!");
// Also suppress unused variable warning in release builds.
(void)old_x;

P.S. So, like, we could try to emit the warning only if we covered enough execution paths to prove that there's either dead code or the warning is true. Then we would no longer care about invalidation problems. Unfortunately, i don't have any specific suggestion of how to prove such facts for an arbitrary CFG.

P.P.S. Actually you know what, maybe we should only drop the report if the constraint over the invalidated value contradicts the constraint over the old value. That'll make things a bit more complicated and will require a visitor indeed, though hopefully not as complicated as concrete value tracking, as we're still interested in only one region at a time.

[NoQ]

clang/test/Analysis/suppress-invalidation-related-reports.cpp

18

In particular, i believe this is a true positive. Because either flag may be set to false by foo() which makes the path feasible, or the if-statement is dead code.

[martong]

P.S. So, like, we could try to emit the warning only if we covered enough execution paths to prove that there's either dead code or the warning is true. Then we would no longer care about invalidation problems. Unfortunately, i don't have any specific suggestion of how to prove such facts for an arbitrary CFG.

If I understand you correctly, this would mean that we have to reason about all possible execution paths at the same time to do this. Actually, that would be possible only with some kind of a fix-point flow-analysis and clearly the symbolic execution we have in CSA is a completely different beast (it reasons about one path where there is a bug).

P.P.S. Actually you know what, maybe we should only drop the report if the constraint over the invalidated value contradicts the constraint over the old value. That'll make things a bit more complicated and will require a visitor indeed, though hopefully not as complicated as concrete value tracking, as we're still interested in only one region at a time.

How would that be different than proving the feasibility of the path with Z3? Could we reuse Mikhail's work here, or that would be overkill for this task?

[NoQ]

In D75698#1914102, @martong wrote:

P.S. So, like, we could try to emit the warning only if we covered enough execution paths to prove that there's either dead code or the warning is true. Then we would no longer care about invalidation problems. Unfortunately, i don't have any specific suggestion of how to prove such facts for an arbitrary CFG.

If I understand you correctly, this would mean that we have to reason about all possible execution paths at the same time to do this. Actually, that would be possible only with some kind of a fix-point flow-analysis and clearly the symbolic execution we have in CSA is a completely different beast (it reasons about one path where there is a bug).

Nah, we just need to cover *enough* paths, not *all* paths. Just take all paths on which we've managed to find a particular bug (we keep track of these paths during de-duplicating anyway) and somehow figure out that it's "enough" paths. The point is to prove that there's dead code if all of these paths are infeasible. I.e., it may require an auxiliary CFG-based analysis that'll reason about all possible execution paths, but it doesn't require us to have much interaction between that analysis and our usual symbolic execution. We already have such auxiliary analysis, eg. see how dead symbol elimination is an all-paths problem and RelaxedLiveVariables analysis is helping us out with it.

See also my old little essay on this subject. I was talking about it in the context of path note generation but the problem is roughly the same here.

I know this is vague but i spent some time trying to come up with an actual solution and failed so far, so take it with a grain of salt^^

P.P.S. Actually you know what, maybe we should only drop the report if the constraint over the invalidated value contradicts the constraint over the old value. That'll make things a bit more complicated and will require a visitor indeed, though hopefully not as complicated as concrete value tracking, as we're still interested in only one region at a time.

How would that be different than proving the feasibility of the path with Z3? Could we reuse Mikhail's work here, or that would be overkill for this task?

This problem is fairly orthogonal to constraint solving. It's about providing constraints, not solving them. Like, we have two different values that we previously thought of as separate, now we're telling the solver that they're, emm, maybe slightly less separate, "hey dude, actually, on second thought, can you please take a look at what would have happened if they were actually equal?". We still don't care how exactly does the solver solve this.

clang/include/clang/Analysis/ProgramPoint.h

public:
 
   // For use with DenseMap.  This hash is probably slow.
   unsigned getHashValue() const {
     llvm::FoldingSetNodeID ID;
     Profile(ID);
     return ID.ComputeHash();
   }
 
+  /// If the program point corresponds to a statement, retrieve that statement.
+  /// Useful for figuring out where to put a warning or a note.
+  const Stmt *getStmtForDiagnostics() const;
+
   bool operator==(const ProgramPoint & RHS) const {
     return Data1 == RHS.Data1 &&
            Data2 == RHS.Data2 &&
            L == RHS.L &&
            Tag == RHS.Tag;
   }
 
   bool operator!=(const ProgramPoint &RHS) const {

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   HelpText<"Improve modeling of the C standard library functions">,
   Documentation<NotDocumented>;
 
 def TrustNonnullChecker : Checker<"TrustNonnull">,
   HelpText<"Trust that returns from framework methods annotated with _Nonnull "
            "are not null">,
   Documentation<NotDocumented>;
 
+def SuppressInvalidationRelatedReportsChecker
+  : Checker<"SuppressInvalidationRelatedReports">,
+  HelpText<"Suppress reports where the expression that was a cause of a bug "
+           "or other expressions related to this expression have been "
+           "invalidated">,
+  Documentation<NotDocumented>;
+
 } // end "apiModeling"
 
 //===----------------------------------------------------------------------===//
 // Evaluate "builtin" functions.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = CoreBuiltin in {
 

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

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H
 #define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H
 
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include <memory>
 
 namespace clang {
 
 class BinaryOperator;
public:
   /// to make all PathDiagnosticPieces created by this visitor.
   static const char *getTag();
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *Succ,
                                    BugReporterContext &BRC,
                                    PathSensitiveBugReport &BR) override;
 };
 
+/// Looks for and suppresses reports where the tracked region's last value
+/// change was a result of an invalidation, which is prone to false positives.
+/// In this example, foo *could* change flags value, but it most likely doesn't.
+///
+///   int flag;
+///   void foo(); // Function body is unknown.
+///
+///   void f() {
+///     flag = 0; // flag is initialized to false.
+///     int *x = 0; // x is initialized to a nullptr
+///
+///     foo(); // Invalidate flag's value.
+///
+///     if (flag) // Assume flag is true.
+///       *x = 5; // x is dereferenced as a nullptr
+///   }
+class SuppressInvalidationVisitor final : public BugReporterVisitor {
+  /// The region whose changes we're analyzing.
+  const MemRegion *const R;
+  /// R may not be directly listed in the invalidation set, but its super region
+  /// might be. In case R *is* in the invalidation set, this field is equal to
+  /// it.
+  const MemRegion *SuperRegion;
+
+  /// Track until the last value change only, whether or not its an
+  /// invalidation.
+  bool IsSatisfied = false;
+
+public:
+  SuppressInvalidationVisitor(const MemRegion *R, const ExplodedNode *N);
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override;
+
+  static const char *getTag();
+
+  PathDiagnosticPieceRef VisitNode(const ExplodedNode *Succ,
+                                   BugReporterContext &BRC,
+                                   PathSensitiveBugReport &BR) override;
+};
+
+} // namespace ento
+} // namespace clang
+
+REGISTER_MAP_FACTORY_WITH_PROGRAMSTATE(HadInvalidation,
+                                       const clang::ento::MemRegion *,
+                                       clang::ProgramPoint)
+
+namespace clang {
+namespace ento {
+
 /// The bug visitor will walk all the nodes in a path and collect all the
 /// constraints. When it reaches the root node, will create a refutation
 /// manager and check if the constraints are satisfiable
 class FalsePositiveRefutationBRVisitor final : public BugReporterVisitor {
 private:
   /// Holds the constraints in a given path
   ConstraintRangeTy Constraints;
 
 public:
   FalsePositiveRefutationBRVisitor();
 
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
                                    BugReporterContext &BRC,
                                    PathSensitiveBugReport &BR) override;
 
   void finalizeVisitor(BugReporterContext &BRC, const ExplodedNode *EndPathNode,
                        PathSensitiveBugReport &BR) override;
 };
 
-
 /// The visitor detects NoteTags and displays the event notes they contain.
 class TagVisitor : public BugReporterVisitor {
 public:
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
                                    BugReporterContext &BRC,
                                    PathSensitiveBugReport &R) override;
 };
 
 } // namespace ento
 
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H

clang/lib/Analysis/ProgramPoint.cpp

case ProgramPoint::PostLValueKind:
       return PostLValue(S, LC, tag);
     case ProgramPoint::PostStmtPurgeDeadSymbolsKind:
       return PostStmtPurgeDeadSymbols(S, LC, tag);
     case ProgramPoint::PreStmtPurgeDeadSymbolsKind:
       return PreStmtPurgeDeadSymbols(S, LC, tag);
   }
 }
 
+const Stmt *ProgramPoint::getStmtForDiagnostics() const {
+  if (auto SP = getAs<StmtPoint>())
+    return SP->getStmt();
+  if (auto BE = getAs<BlockEdge>())
+    return BE->getSrc()->getTerminatorStmt();
+  if (auto CE = getAs<CallEnter>())
+    return CE->getCallExpr();
+  if (auto CEE = getAs<CallExitEnd>())
+    return CEE->getCalleeContext()->getCallSite();
+  if (auto PIPP = getAs<PostInitializer>())
+    return PIPP->getInitializer()->getInit();
+  if (auto CEB = getAs<CallExitBegin>())
+    return CEB->getReturnStmt();
+  if (auto FEP = getAs<FunctionExitPoint>())
+    return FEP->getStmt();
+  return nullptr;
+}
+
 LLVM_DUMP_METHOD void ProgramPoint::dump() const {
   return printJson(llvm::errs());
 }
 
 void ProgramPoint::printJson(llvm::raw_ostream &Out, const char *NL) const {
   const ASTContext &Context =
       getLocationContext()->getAnalysisDeclContext()->getASTContext();
   const SourceManager &SM = Context.getSourceManager();

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
+  SuppressInvalidationRelatedReports.cpp
   Taint.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
   TrustNonnullChecker.cpp
   UndefBranchChecker.cpp
   UndefCapturedBlockVarChecker.cpp
   UndefResultChecker.cpp

clang/lib/StaticAnalyzer/Checkers/SuppressInvalidationRelatedReports.cpp

+//=== FIXME.cpp -------------------------------------------*- C++ -*--//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+//  TODO
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ExprCXX.h"
+#include "clang/Basic/IdentifierTable.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SuppressInvalidationRelatedReportsChecker
+    : public Checker<check::RegionChanges> {
+public:
+  ProgramStateRef
+  checkRegionChanges(ProgramStateRef State,
+                     const InvalidatedSymbols *Invalidated,
+                     ArrayRef<const MemRegion *> ExplicitRegions,
+                     ArrayRef<const MemRegion *> Regions,
+                     const LocationContext *LCtx, const CallEvent *Call) const {
+    if (!Call)
+      return State;
+
+    for (const MemRegion *MR : Regions)
+      State = State->set<HadInvalidation>(MR, Call->getProgramPoint());
+
+    return State;
+  }
+
+  void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
+                  const char *Sep) const override {
+    for (const HadInvalidation::value_type &I : State->get<HadInvalidation>()) {
+      I.first->dumpToStream(Out);
+      Out << " was invalidated on '";
+      if (const Stmt *InvalidatingStmt = I.second.getStmtForDiagnostics())
+        InvalidatingStmt->getBeginLoc().print(
+            Out, State->getAnalysisManager().getSourceManager());
+      Out << "'" << NL;
+    }
+  }
+};
+
+} // end anonymous namespace
+
+void ento::registerSuppressInvalidationRelatedReportsChecker(
+    CheckerManager &mgr) {
+  mgr.registerChecker<SuppressInvalidationRelatedReportsChecker>();
+}
+
+bool ento::shouldRegisterSuppressInvalidationRelatedReportsChecker(
+    const LangOptions &LO) {
+  return true;
+}

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/CFGStmtMap.h"
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetBuiltins.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.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/MemRegion.h"
if (isUnderconstrained(PrevN)) {
     X->setTag(getTag());
     return std::move(X);
   }
 
   return nullptr;
 }
 
 //===----------------------------------------------------------------------===//
+// Implementation of SuppressInvalidationVisitor.
+//===----------------------------------------------------------------------===//
+
+SuppressInvalidationVisitor::SuppressInvalidationVisitor(const MemRegion *R,
+                                                         const ExplodedNode *N)
+    : R(R), SuperRegion(R) {
+  for (HadInvalidation::value_type &HI : N->getState()->get<HadInvalidation>())
+    if (R->isSubRegionOf(HI.first))
+      SuperRegion = HI.first;
+  if (!N->getState()->contains<HadInvalidation>(SuperRegion))
+    IsSatisfied = true;
+}
+
+void SuppressInvalidationVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
+  static int id = 0;
+  ID.AddPointer(&id);
+  ID.Add(R);
+}
+
+const char *SuppressInvalidationVisitor::getTag() {
+  return "InvalidSuppVisitor";
+}
+
+PathDiagnosticPieceRef
+SuppressInvalidationVisitor::VisitNode(const ExplodedNode *Succ,
+                                       BugReporterContext &BRC,
+                                       PathSensitiveBugReport &BR) {
+  if (IsSatisfied)
+    return nullptr;
+
+  ProgramStateRef SuccState = Succ->getState();
+  ProgramStateRef PredState = Succ->getFirstPred()->getState();
+
+  // Look for the last write of R.
+  if (PredState->getSVal(R) != SuccState->getSVal(R)) {
+    Succ->getStmtForDiagnostics();
+
+    // We found the last write, invalidations previous to this point are not
+    // interesting.
+    IsSatisfied = true;
+
+    // R may have been invalidated multiple times, is the last invalidation
+    // also the last write?
+    if (Succ->getLocation() != *SuccState->get<HadInvalidation>(SuperRegion))
+      return nullptr;
+    const LocationContext *CurLC = Succ->getLocationContext();
+
+    BR.markInvalid("Suppress IV", CurLC);
+    return nullptr;
+  }
+  return nullptr;
+}
+
+//===----------------------------------------------------------------------===//
 // Implementation of SuppressInlineDefensiveChecksVisitor.
 //===----------------------------------------------------------------------===//
 
 SuppressInlineDefensiveChecksVisitor::
 SuppressInlineDefensiveChecksVisitor(DefinedSVal Value, const ExplodedNode *N)
     : V(Value) {
   // Check if the visitor is disabled.
   AnalyzerOptions &Options = N->getState()->getAnalysisManager().options;
if (ControlDeps.isControlDependent(OriginB, NB)) {
     if (llvm::isa_and_nonnull<CXXForRangeStmt>(NB->getTerminatorStmt()))
       return nullptr;
 
     if (const Expr *Condition = NB->getLastCondition()) {
       // Keeping track of the already tracked conditions on a visitor level
       // isn't sufficient, because a new visitor is created for each tracked
       // expression, hence the BugReport level set.
       if (BR.addTrackedCondition(N)) {
-        bugreporter::trackExpressionValue(
-            N, Condition, BR, bugreporter::TrackingKind::Condition,
+        bugreporter::trackExpressionValue(N, Condition, BR,
+                                          bugreporter::TrackingKind::Condition,
                                           /*EnableNullFPSuppression=*/true);
         return constructDebugPieceForTrackedCondition(Condition, N, BRC);
       }
     }
   }
 
   return nullptr;
 }
 
if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
     // reference and reference to null pointer.
     // If the LVal is null, check if we are dealing with null reference.
     // For those, we want to track the location of the reference.
     const MemRegion *R = (RR && LVIsNull) ? RR :
         LVNode->getSVal(Inner).getAsRegion();
 
     if (R) {
 
+      report.addVisitor(
+          std::make_unique<SuppressInvalidationVisitor>(R, InputNode));
+
       // Mark both the variable region and its contents as interesting.
       SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
       report.addVisitor(
           std::make_unique<NoStoreFuncVisitor>(cast<SubRegion>(R), TKind));
 
       MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
           LVNode, R, EnableNullFPSuppression, report, V);
 
       report.markInteresting(V, TKind);
       report.addVisitor(std::make_unique<UndefOrNullArgVisitor>(R));
 
       // If the contents are symbolic and null, find out when they became null.
       if (V.getAsLocSymbol(/*IncludeBaseRegions=*/true))
         if (LVState->isNull(V).isConstrainedTrue())
           report.addVisitor(std::make_unique<TrackConstraintBRVisitor>(
               V.castAs<DefinedSVal>(), false));
 
       // Add visitor, which will suppress inline defensive checks.
-      if (auto DV = V.getAs<DefinedSVal>())
+      if (auto DV = V.getAs<DefinedSVal>()) {
         if (!DV->isZeroConstant() && EnableNullFPSuppression) {
           // Note that LVNode may be too late (i.e., too far from the InputNode)
           // because the lvalue may have been computed before the inlined call
           // was evaluated. InputNode may as well be too early here, because
           // the symbol is already dead; this, however, is fine because we can
           // still find the node in which it collapsed to null previously.
           report.addVisitor(
               std::make_unique<SuppressInlineDefensiveChecksVisitor>(
                   *DV, InputNode));
         }
+      }
 
       if (auto KV = V.getAs<KnownSVal>())
         report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
             *KV, R, EnableNullFPSuppression, TKind, SFC));
       return true;
     }
   }
 

clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp

const Stmt *ExplodedNode::getStmtForDiagnostics() const {
   // code for the first time.
   const LocationContext *LC = getLocationContext();
   if (LC->getAnalysisDeclContext()->isBodyAutosynthesized()) {
     // It must be a stack frame because we only autosynthesize functions.
     return cast<StackFrameContext>(findTopAutosynthesizedParentContext(LC))
         ->getCallSite();
   }
   // Otherwise, see if the node's program point directly points to a statement.
-  // FIXME: Refactor into a ProgramPoint method?
-  ProgramPoint P = getLocation();
-  if (auto SP = P.getAs<StmtPoint>())
-    return SP->getStmt();
-  if (auto BE = P.getAs<BlockEdge>())
-    return BE->getSrc()->getTerminatorStmt();
-  if (auto CE = P.getAs<CallEnter>())
-    return CE->getCallExpr();
-  if (auto CEE = P.getAs<CallExitEnd>())
-    return CEE->getCalleeContext()->getCallSite();
-  if (auto PIPP = P.getAs<PostInitializer>())
-    return PIPP->getInitializer()->getInit();
-  if (auto CEB = P.getAs<CallExitBegin>())
-    return CEB->getReturnStmt();
-  if (auto FEP = P.getAs<FunctionExitPoint>())
-    return FEP->getStmt();
-
-  return nullptr;
+  return getLocation().getStmtForDiagnostics();
 }
 
 const Stmt *ExplodedNode::getNextStmtForDiagnostics() const {
   for (const ExplodedNode *N = getFirstSucc(); N; N = N->getFirstSucc()) {
     if (const Stmt *S = N->getStmtForDiagnostics()) {
       // Check if the statement is '?' or '&&'/'||'.  These are "merges",
       // not actual statement points.
       switch (S->getStmtClass()) {

clang/test/Analysis/analyzer-enabled-checkers.c

 // RUN: %clang --analyze %s --target=x86_64-pc-linux-gnu \
 // RUN:   -Xclang -analyzer-list-enabled-checkers \
 // RUN:   -Xclang -analyzer-display-progress \
 // RUN:   2>&1 | FileCheck %s --implicit-check-not=ANALYZE \
 // RUN:                       --implicit-check-not=\.
 
 // CHECK:      OVERVIEW: Clang Static Analyzer Enabled Checkers List
 // CHECK-EMPTY:
 // CHECK-NEXT: apiModeling.StdCLibraryFunctions
+// CHECK-NEXT: apiModeling.SuppressInvalidationRelatedReports
 // CHECK-NEXT: apiModeling.TrustNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
 // CHECK-NEXT: core.CallAndMessage
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonNullParamChecker
 // CHECK-NEXT: core.NonnilStringConstants

clang/test/Analysis/suppress-invalidation-related-reports.cpp

+// RUN: %clang_analyze_cc1 -verify=expected %s \
+// RUN:   -analyzer-checker=core,apiModeling.SuppressInvalidationRelatedReports
+
+// RUN: %clang_analyze_cc1 -verify=non-suppressed,expected %s \
+// RUN:   -analyzer-checker=core
+
+namespace invalidated_global {
+int flag;
+void foo();
+
+void f() {
+  flag = 0;   // Initialize flag to false.
+  int *x = 0; // x is initialized to a nullptr.
+
+  foo(); // Invalidate flag's value.
+
+  if (flag) // Assume flag is true.
+    *x = 5; // non-suppressed-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_global
+
+namespace invalidated_field {
+struct S {
+  int flag;
+  void invalidate();
+};
+
+void f() {
+  S s;
+  s.flag = 0; // Initialize s.flag to false.
+  int *x = 0; // x is initialized to a nullptr.
+
+  s.invalidate(); // Invalidate s.flag's value.
+
+  if (s.flag) // Assume s.flag is true.
+    *x = 5;   // non-suppressed-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field
+
+namespace invalidated_field_before_bind {
+struct S {
+  int flag;
+  void invalidate();
+  void setFlagToTrue() {
+    invalidate();
+    flag = 1;
+  }
+};
+
+void f() {
+  S s;
+  s.flag = 0; // Initialize s.flag to false.
+  int *x = 0; // x is initialized to a nullptr.
+
+  s.setFlagToTrue(); // Invalidate s.flag's, but also set it to true.
+
+  if (s.flag) // Assume s.flag is true.
+    *x = 5;   // expected-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field_before_bind
+
+namespace invalidated_field_twice {
+struct S {
+  int flag;
+  void invalidate();
+  void setFlagToTrue() {
+    invalidate();
+    flag = 1;
+    invalidate();
+  }
+};
+
+void f() {
+  S s;
+  s.flag = 0; // Initialize s.flag to false.
+  int *x = 0; // x is initialized to a nullptr.
+
+  s.setFlagToTrue(); // Invalidate s.flag's, but also set it to true.
+
+  if (s.flag) // Assume s.flag is true.
+    *x = 5;   // non-suppressed-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field_twice
+
+namespace invalidated_field_through_bind {
+struct S {
+  int flag;
+};
+
+void f() {
+  S s;
+  s.flag = 0; // Initialize s.flag to false.
+  S s2;
+  s2.flag = 1; // Initialize s2.flag to true
+  int *x = 0;  // x is initialized to a nullptr.
+
+  s = s2; // "Invalidate" s.flag's value through bind.
+
+  if (s.flag) // Assume s.flag is true.
+    *x = 5;   // expected-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field_through_bind