[clang] [analyzer] Suppress out of bounds reports after weak loop assumptions (PR #109804)

[Donát Nagy via cfe-commits]

================
@@ -194,3 +199,99 @@ char test_comparison_with_extent_symbol(struct incomplete *p) {
   return ((char *)p)[-1]; // no-warning
 }
 
+// WeakLoopAssumption suppression
+///////////////////////////////////////////////////////////////////////
+
+int GlobalArray[100];
+int loop_suppress_after_zero_iterations(unsigned len) {
+  for (unsigned i = 0; i < len; i++)
+    if (GlobalArray[i] > 0)
+      return GlobalArray[i];
+  // Previously this would have produced an overflow warning because splitting
+  // the state on the loop condition introduced an execution path where the
+  // analyzer thinks that len == 0.
+  // There are very many situations where the programmer knows that an argument
+  // is positive, but this is not indicated in the source code, so we must
+  // avoid reporting errors (especially out of bounds errors) on these
+  // branches, because otherwise we'd get prohibitively many false positives.
+  return GlobalArray[len - 1]; // no-warning
+}
----------------
NagyDonat wrote:

> If someone DID forget that an overflow can happen here and the analyzer doesn't report an error, we will have a false negative, right?

Yes, in that case we'd have a false negative.

However, false negatives are acceptable, while too many false positives make the checker practically useless, so it's better to err on the side of not reporting in an ambiguous situation like this.

> If the developer knows that `len` is never 0, the proper way to communicate it is `assert(len != 0)`.

This is just your preference, which may be shared by many programmers, but is still very far from being accepted universally. (I'd say that it's often a good idea to add an assertion like this, but I wouldn't want to use a tool that enforces its dogmatic use.)

See also my related remarks in the top-level post.

https://github.com/llvm/llvm-project/pull/109804