[clang] [analyzer] Workaround for unintended slowdown (scope increase) (PR #136720)

[Donát Nagy via cfe-commits]

================
@@ -2856,8 +2873,29 @@ void ExprEngine::processBranch(
       // conflicts with the widen-loop analysis option (which is off by
       // default). If we intend to support and stabilize the loop widening,
       // we must ensure that it 'plays nicely' with this logic.
-      if (!SkipTrueBranch || AMgr.options.ShouldWidenLoops)
+      if (!SkipTrueBranch || AMgr.options.ShouldWidenLoops) {
         Builder.generateNode(StTrue, true, PredN);
+      } else if (AMgr.options.LegacyInliningPrevention) {
+        // FIXME: There is an ancient and very arbitrary heuristic in
+        // `ExprEngine::processCFGBlockEntrance` which prevents all further
+        // inlining of a function if it finds an execution path within that
+        // function which reaches the `MaxBlockVisitOnPath` limit (a/k/a
+        // `analyzer-max-loop`, by default four iterations in a loop). Adding
+        // this "don't assume third iteration" logic significantly increased
+        // the analysis runtime on some inputs because less functions were
+        // arbitrarily excluded from being inlined, so more entrypoints used
+        // up their full allocated budget. As a hacky compensation for this,
+        // here we apply the "should not inline" mark in cases when the loop
+        // could potentially reach the `MaxBlockVisitOnPath` limit without the
+        // "don't assume third iteration" logic. This slightly overcompensates
+        // (activates if the third iteration can be entered, and will not
+        // recognize cases where the fourth iteration would't be completed), but
+        // should be good enough for practical purposes.
+        if (const LocationContext *LC = getInlinedLocationContext(Pred, G)) {
+          Engine.FunctionSummaries->markShouldNotInline(
+              LC->getStackFrame()->getDecl());
+        }
+      }
----------------
NagyDonat wrote:

> My point is, if Ericsson will ship this true, and we are also going to ship this with true, then who would want to disable this? Do you expect users disabling this?

Good point.

>  Would anyone complain if we would just land this fix without an option for opting out?

I felt that the logic introduced by this commit is so ugly that it should not be unconditionally part of the analyzer behavior (especially if it is `git blame`d to me...). By introducing this option I wanted to express that "this is not a stable permanent part of the analyzer, please don't rely on its existence" -- and I felt that yet another "FIXME" comment is not enough for this hack.

However, I can accept removing this option (and making the hack unconditional) if you strongly prefer that.

> Also I am not sure we should have legacy in the name of something that is enabled by default.

When I wrote this change, I hoped that this config option could be switched off and deprecated in the foreseeable future (in fact it was disabled by default in the first draft of the commit).

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