[analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[spaits]

Summary

As discussed in my previous patch (D142354) the analyzer can understand std::variant and std::any through regular inlining. However warnings coming from these classes were suppressed by NoStateChangeFuncVisitor, more specificly by an instance of NoStoreFuncVisitor. The reason behind that is that we suppress bug reports when a system header function was supposed to initialize its parameter but failed to (as you can read about it in a comment block visible in this patch).

Now the problem is that these bug report were suppressed, but they had nothing to do with uninitialized values. In a follow up patch I intend to fix that and see how this suppression logic works as it was meant to be, but my initial testing shows that they no longer falsely suppress reports like this:

std::variant<int, std::string> v = 0;
int i = std::get<int>(v);
10/i; // FIXME: This should warn for division by zero!

In this patch I only separated these two functionalities because I don't think this suppression should be implemented in a NoStateChangeVisitor but in it's own.

[Szelethus]

We worked on this together, so I waited a bit for others to have a say in this, but this design seems like a no brainer to me. Please fix those comments, otherwise LGTM.

Also, while the patch is LGTM (moving code around is okay), the comment says "system headers having a chance to initialize the value but failing to do so", but do we ever check anywhere whether the function actually had the chance to change the value (passed by reference/pointer)?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

454–455

Since we don't check within this visitor whether the value is uninitialized, these comments are no longer up-to-date (and were not before your patch either).

What we are really doing here is flat out suppressing the report if it contains an inlined standard library function. What also needs to be said is that this visitor is as dumb as it comes -- its really down to the caller (or, more specifically, the one who registers this visitor) to make sure that this heuristic should be employed.

For example, if the bug report is about an uninitialized value that was passed to a system header function as a pointer/reference, this visitor could suppress the warning on the grounds that the system header likely would have initialized this value, but in some obscure cases the function could fail and not initialize the value, which could theoretically occur but practice it yields only (or mostly) false positives.

454–455

Here as well, drop the bit about uninitializedness, unless you talk about it in the context of an example.

[spaits]

As @Szelethus has mentioned in his reply I tried to improve on the comment. I hope now it describes correctly what the new visitor is good for.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

449

We also lost this, unfortunately, and this is kind of the point where we check whether the std function has anything to do with the uninitialized value (its been passed as parameter or something).

Please investigate whether losing this has any effect (and it very likely does).

[xazax.hun]

Do you plan to selectively enable warnings coming from the STL to catch misuses of certain STL types?

I think we should probably discuss this direction first. It is, in general, a fragile approach. Users might use Clang with GCC's, LLVM's, or MSVC's standard library. Today, we do not have the means to ensure that these checks continue to work as expected on all STL implementations over time. Also, those warning reports might be leaky in a sense the reported path might contain implementations details from the STL that is hard to interpret.

I am afraid, if we want to provide a good user experience, we might be doomed to manually simulate the behavior of STL classes.

What do you think?

[spaits]

In D145069#4191046, @xazax.hun wrote:

Do you plan to selectively enable warnings coming from the STL to catch misuses of certain STL types?

No. At first when we have found out that the Static Analyzer can reason about std::variant but it suppresses the diagnostics (D142354#4079643), we were suspecting a too strong heuristic somewhere, that invalidates even true positives. Since the Static Analyzer was able to reason about std::variant by itself we we did not want to write a checker to do the same thing. So the plan was to find the point where the suppression happen and change on the heuristics so it can let thru every kind of true positive about STL types/functions. But as it turns out, it is kind of impossible to do that without letting a lot of false positives to not be suppressed.

While it seems like it may be very difficult to unsuppress all the reports from std::variant, it still made sense to fine-tune some of these suppression.

In D145069#4191046, @xazax.hun wrote:

Also, those warning reports might be leaky in a sense the reported path might contain implementations details from the STL that is hard to interpret.

We have tested the new heuristics and the new reports did not contain implementation details form STL. Obviously the test do not cover every possibility so there might be some leaking but we haven't seen it yet.

In D145069#4191046, @xazax.hun wrote:

I am afraid, if we want to provide a good user experience, we might be doomed to manually simulate the behavior of STL classes.

That might be the best approach to prevent the mentioned implementation dependency. Plus it would probably make it easier to write my BSc thesis if I have created a brand new checker.

On another note, it might be interesting to see how the checker approach and the force-inlining analyses compare (even if one of those approaches turn out to be a dead end).

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode(
     const ExplodedNode *N, BugReporterContext &BR, PathSensitiveBugReport &R) {
 
   const LocationContext *Ctx = N->getLocationContext();
   const StackFrameContext *SCtx = Ctx->getStackFrame();
   ProgramStateRef State = N->getState();
   auto CallExitLoc = N->getLocationAs<CallExitBegin>();
 
   // No diagnostic if region was modified inside the frame.
   if (!CallExitLoc || isModifiedInFrame(N))
     return nullptr;
 
   CallEventRef<> Call =
       BR.getStateManager().getCallEventManager().getCaller(SCtx, State);
 
-  // Optimistically suppress uninitialized value bugs that result
-  // from system headers having a chance to initialize the value
-  // but failing to do so. It's too unlikely a system header's fault.
-  // It's much more likely a situation in which the function has a failure
-  // mode that the user decided not to check. If we want to hunt such
-  // omitted checks, we should provide an explicit function-specific note
-  // describing the precondition under which the function isn't supposed to
-  // initialize its out-parameter, and additionally check that such
-  // precondition can actually be fulfilled on the current path.
-  if (Call->isInSystemHeader()) {
-    // We make an exception for system header functions that have no branches.
-    // Such functions unconditionally fail to initialize the variable.
-    // If they call other functions that have more paths within them,
-    // this suppression would still apply when we visit these inner functions.
-    // One common example of a standard function that doesn't ever initialize
-    // its out parameter is operator placement new; it's up to the follow-up
-    // constructor (if any) to initialize the memory.
-    if (!N->getStackFrame()->getCFG()->isLinear()) {
-      static int i = 0;
-      R.markInvalid(&i, nullptr);
-    }
+  if (Call->isInSystemHeader())
     return nullptr;
-  }
 
   if (const auto *MC = dyn_cast<ObjCMethodCall>(Call)) {
     // If we failed to construct a piece for self, we still want to check
     // whether the entity of interest is in a parameter.
     if (PathDiagnosticPieceRef Piece = maybeEmitNoteForObjCSelf(R, *MC, N))
       return Piece;
   }
 
   if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
     // Do not generate diagnostics for not modified parameters in
     // constructors.
     return maybeEmitNoteForCXXThis(R, *CCall, N);
   }
 
   return maybeEmitNoteForParameters(R, *Call, N);
 }
 
 //===----------------------------------------------------------------------===//
+// Implementation of SuppressSystemHeaderWarningVistor.
+//===----------------------------------------------------------------------===//
+
+// This visitor suppresses warnings coming from inlined system
+// headers functions when we exit the call that have no branches.
+// This is a very primitive visitor. It flat-out suppresses every report that
+// have a node that matches all the conditions above.
+// It should be used carefully!
+//
+// A good example for the usage of this visitor is when we want to suppress
+// warnings when a system header function does not initialize their arguments.
+// It's too unlikely a system header's fault.
+// It's much more likely a situation in which the function has a failure
+// mode that the user decided not to check. If we want to hunt such
+// omitted checks, we should provide an explicit function-specific note
+// describing the precondition under which the function isn't supposed to
+// initialize its out-parameter, and additionally check that such
+// precondition can actually be fulfilled on the current path.
+//
+// System header functions that have no branches unconditionally fail to
+// initialize the variable.
+// If they call other functions that have more paths within them,
+// this suppression would still apply when we visit these inner functions.
+// One common example of a standard function that doesn't ever initialize
+// its out parameter is operator placement new; it's up to the follow-up
+// constructor (if any) to initialize the memory
+namespace {
+class SuppressSystemHeaderWarningVisitor : public BugReporterVisitor {
+public:
+  virtual PathDiagnosticPieceRef VisitNode(const ExplodedNode *,
+                                           BugReporterContext &,
+                                           PathSensitiveBugReport &) override;
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    static int Tag = 0;
+    ID.AddPointer(&Tag);
+  }
+};
+} // namespace
+
+PathDiagnosticPieceRef
+SuppressSystemHeaderWarningVisitor::VisitNode(const ExplodedNode *Succ,
+                                              BugReporterContext &BRC,
+                                              PathSensitiveBugReport &BR) {
+  const LocationContext *Ctx = Succ->getLocationContext();
+  const StackFrameContext *SCtx = Ctx->getStackFrame();
+  ProgramStateRef State = Succ->getState();
+  auto CallExitLoc = Succ->getLocationAs<CallExitBegin>();
+
+  if (!CallExitLoc)
+    return nullptr;
+
+  CallEventRef<> Call =
+      BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
+
+  if (Call->isInSystemHeader()) {
+    if (!Succ->getStackFrame()->getCFG()->isLinear()) {
+      static int i = 0;
+      BR.markInvalid(&i, nullptr);
+    }
+  }
+  return nullptr;
+}
+
+//===----------------------------------------------------------------------===//
 // Implementation of NoStoreFuncVisitor.
 //===----------------------------------------------------------------------===//
 
 namespace {
 /// Put a diagnostic on return statement of all inlined functions
 /// for which  the region of interest \p RegionOfInterest was passed into,
 /// but not written inside, and it has caused an undefined read or a null
 /// pointer dereference outside.
if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
       const MemRegion *R =
           (RR && LVIsNull) ? RR : LVNode->getSVal(Inner).getAsRegion();
 
       if (R) {
 
         // Mark both the variable region and its contents as interesting.
         SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
         Report.addVisitor<NoStoreFuncVisitor>(cast<SubRegion>(R), Opts.Kind);
-
+        Report.addVisitor<SuppressSystemHeaderWarningVisitor>();
         // When we got here, we do have something to track, and we will
         // interrupt.
         Result.FoundSomethingToTrack = true;
         Result.WasInterrupted = true;
 
         MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
             LVNode, R, Opts.EnableNullFPSuppression, Report, V);