[PATCH] D53076: [analyzer] ConditionBRVisitor: enhance to write out more information

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Btw, did you try running this patch against big projects? 'Cause it's a high-impact change and we'd have to be careful with it.

I ran it on LLVM real quick and noticed that 97.3% (!) of reports contained at least one of the new notes, with number of HTML path pieces increased by 23.7% on average (note that the impact on plist reports would be much higher because they don't have any text for gray pieces).

I'll try to get back with some actual hand-picked examples to demonstrate my impressions. My overall feel was that it increases quality of life quite a bit by reducing the amount of time spent scrolling and jumping around the report - the necessary information becomes available exactly where you need it and it's wonderful. At the same time, i noticed a few corner-cases where the new pieces were duplicating the existing information or otherwise not helpful:

F8520276: operator_question_mark.png <https://reviews.llvm.org/F8520276> F8520282: operator_question_mark.html <https://reviews.llvm.org/F8520282>

F8520283: more_repeats.png <https://reviews.llvm.org/F8520283> F8520285: more_repeats.html <https://reviews.llvm.org/F8520285>

This doesn't look like a fundamental problem to me; it should be possible to improve upon these corner-cases by varying note messages; probably it also makes sense to skip some of them when they're clearly saying the same info 10 times in a row, but i'm not sure it's easy to decide where to apply that.

Because it might be a large amount of work, i'm thinking about committing this new feature off-by-default first (eg., `-analyzer-config knowing-notes=[false|true]`) and then doing follow-up commits (you already have quite a bit!) to make sure it's polished when we turn it on for everyone, WDYT?



================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:169-173
+  // If the constraint information is changed between the current and the
+  // previous program state we assuming the newly seen constraint information.
+  // If we cannot evaluate the condition (and the constraints are the same)
+  // the analyzer has no information about the value and just assuming it.
+  bool IsAssuming;
----------------
Does this absolutely need to be a member variable? Maybe pass it down the stack, like `tookTrue`? I understand that there are already a lot of flags passed down, but global mutable state is still more dangerous and can easily get out of hand.


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187-190
+  // The value may hard-coded.
+  SVal HardCodedSVal = State->getSVal(CondVarExpr, LCtx);
+  if (auto HardCodedCI = HardCodedSVal.getAs<nonloc::ConcreteInt>())
+    return &HardCodedCI->getValue();
----------------
I don't see this triggered on tests. It looks to me that this function is for now only applied to `DeclRefExpr`s that are always glvalues and therefore should never be evaluated to a `nonloc` value.


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1991-1992
   return std::make_shared<PathDiagnosticEventPiece>(
-      Loc, tookTrue ? GenericTrueMessage : GenericFalseMessage);
+      Loc, IsAssuming ? tookTrue ? AssumingTrueMessage : AssumingFalseMessage
+                      : tookTrue ? TrueMessage : FalseMessage);
 }
----------------
I advocate for more parentheses :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53076/new/

https://reviews.llvm.org/D53076