[analyzer] Turn off reports in system headers

[bruntib]

Summary

Include paths can be signed as system header paths. The convention is not to give any diagnostic reports in these system headers by any analyzer tools. ClangSA gives reports in system header by default, but this patch introduces an analyzer config option which prevents this default behavior.

[balazske]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2831

How to interpret this comment? Better would be "Report bugs in system headers only if this option is turned on.".

[NoQ]

This patch only affects path-insensitive checkers. For such checkers disabling the analysis entirely (i.e., not calling checkASTCodeBody in system headers) would probably make more sense.

For some discussion about path-sensitive checkers see https://bugs.llvm.org/show_bug.cgi?id=44065.

[NoQ]

This patch only affects path-insensitive checkers.

Wait, no, i'm wrong, there's a fall-through there. So just see that thread.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

2831

I agree with @balazske. The goal of system headers is to provide a set of simple data structures and algorithms to be reused in programs. This comment should be rephrased..

clang/test/Analysis/system-header.c

1

I think you could simply use -isystem %S/Inputs and the use a function in system-header-simulator.h. I do not think that we need to add extra directory to test this.

[baloghadamsoftware]

In D80210#2044763, @NoQ wrote:

For some discussion about path-sensitive checkers see https://bugs.llvm.org/show_bug.cgi?id=44065.

I think this patch does not disable them only allows the user to disable them since it adds an option to enable such warnings with default value true. Thus they are enabled by default, thus nothing changes in the default behavior. However, if a user decides to hide such warnings this patch allows this.

[bruntib]

@balazske Alright, I rephrased it according to your suggestion.

@baloghadamsoftware I needed a header file which has a bug inside and also not indicated with "expected-warning" in order to properly test that the report is suppressed in a system header. However, it's true that an extra directory is not necessary, so I moved my header file to "Inputs" directory.

@NoQ Thanks for the attached ticket. Now I can see that this is a conscious decision emitting reports even in system headers. And I agree that most of the times a bug can be the caller's fault even if the report is in a system header. On the other hand in the projects this is the reason for adding 3pp libraries via -isystem flag to the compiler so that no analyzer tool reports any warnings on these files. That's why I thought binding this behavior to an analyzer option, just like clang-tidy does with --system-headers flag.

[baloghadamsoftware]

In D80210#2046444, @bruntib wrote:

@baloghadamsoftware I needed a header file which has a bug inside and also not indicated with "expected-warning" in order to properly test that the report is suppressed in a system header. However, it's true that an extra directory is not necessary, so I moved my header file to "Inputs" directory.

Are you sure that you need the bug to be inside? Or just an error caused from outside appearing inside?

[bruntib]

@baloghadamsoftware You're right, but I couldn't find a header file which (1) contains a bug which ClangSA reports, (2) is not an "expected-warning", (3) is inside an already existing directory so it could be considered a system header.

[NoQ]

@Szelethus WDYT about, like, generally, suppressing reports when there exists a common ancestor stack frame to all interesting stack frames and unprunable diagnostic pieces that resides in the system header?

[Szelethus]

In D80210#2050813, @NoQ wrote:

@Szelethus WDYT about, like, generally, suppressing reports when there exists a common ancestor stack frame to all interesting stack frames and unprunable diagnostic pieces that resides in the system header?

What a tongue twister of a sentence :)

The point is to prove, or at the very least guess whether the problem originated from a system header?
What do you mean under ancestor stack frame? That all other interesting stack frames are nested in it, or that its present before any other stack frame in the bug report?

[...] unprunable diagnostic pieces that resides in the system header?

Is this about finding a *part of the bug report* that is in a system header rather than the actual warning? Isn't this patch about the latter?

I failed to come up with an example that fits your description, could you help me out with one please? :)

[NoQ]

Eg., suppress this report:

// system_header.h
int bar() {
  int *x = NULL;
  return *x;
}
// user_file.c
void foo() {
  bar();
}

but don't suppress this report:

// system_header.h
int bar(int *x) {
  return *x;
}
// user_file.c
void foo() {
  int *y = NULL;
  bar(y);
}

In the first example there are two interesting events: (1) x = NULL, (2) return *x but both of them are in the system header. In the second example the interesting event (1) y = NULL is not in the system header, so the report remains.

That all other interesting stack frames are nested in it

I think this is the best take. A more aggressive suppression would be "all interesting things are in a system header", which would allow the suppressed bug report to temporarily leave the system header between interesting events.

[Szelethus]

Sounds nice. @bruntib, would this be sufficient for the user's request? Are we sure that the actual request makes sense in light of the analyzer's viewpoint?

[NoQ]

(if we agree to do this, the next obvious experiment would be to cut out the beginning of the path from the report we *do* emit - i.e., everything until our first interesting stack frame)

[bruntib]

Hi,

Thanks everyone for the investigation in this problem.
I find these suggestions reasonable, and I think this commented example is a strong beating card in our hands to convince our users that paths going through system headers are meaningful and important.
I'll start implementing this idea and we'll see how much it affects the results in real-world projects.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

 ANALYZER_OPTION(bool, ShouldTrackConditionsDebug, "track-conditions-debug",
                 "Whether to place an event at each tracked condition.",
                 false)
 
 ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks",
                 "Emit fix-it hints as remarks for testing purposes",
                 false)
 
+ANALYZER_OPTION(bool, ShouldReportInSystemHeader, "report-in-system-header",
+                "Emit a bug report even if it is located in a system header.",
+                true)
+
 //===----------------------------------------------------------------------===//
 // Unsigned analyzer options.
 //===----------------------------------------------------------------------===//
 
 ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold",
                 "The maximal amount of translation units that is considered "
                 "for import when inlining functions during CTU analysis. "
                 "Lowering this threshold can alleviate the memory burder of "

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

 void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
   bool ValidSourceLoc = R->getLocation().isValid();
   assert(ValidSourceLoc);
   // If we mess up in a release build, we'd still prefer to just drop the bug
   // instead of trying to go on.
   if (!ValidSourceLoc)
     return;
 
+  // The goal of system headers is that no diagnostic report is emitted in
+  // those.
+  if (!getAnalyzerOptions().ShouldReportInSystemHeader &&
+      R->getLocation().asLocation().isInSystemHeader())
+    return;
+
   // Compute the bug report's hash to determine its equivalence class.
   llvm::FoldingSetNodeID ID;
   R->Profile(ID);
 
   // Lookup the equivance class.  If there isn't one, create it.
   void *InsertPos;
   BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
 

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport = false
 // CHECK-NEXT: optin.performance.Padding:AllowedPad = 24
 // CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false
 // CHECK-NEXT: osx.cocoa.RetainCount:CheckOSObject = true
 // CHECK-NEXT: osx.cocoa.RetainCount:TrackNSCFStartParam = false
 // CHECK-NEXT: prune-paths = true
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: report-in-main-source-file = false
+// CHECK-NEXT: report-in-system-header = true
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: silence-checkers = ""
 // CHECK-NEXT: stable-report-filename = false
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 96
+// CHECK-NEXT: num-entries = 97

clang/test/Analysis/system-header.c

+// RUN: %clang_analyze_cc1 -analyzer-opt-analyze-headers -analyzer-checker=security.FloatLoopCounter -analyzer-config report-in-system-header=false -isystem %S/system-include -verify %s
+
+#include <system-header.h>
+
+void normalFunction()
+{
+  for (float f = 0; f < 10; f += 1) // expected-warning {{Variable 'f' with floating point type 'float' should not be used as a loop counter}}
+    ;
+
+  systemFunction();
+}

clang/test/Analysis/system-include/system-header.h

+void systemFunction()
+{
+  for (float f = 0; f < 10; f += 1)
+    ;
+}