[PATCH] D34508: [Analyzer] Bug Reporter Visitor to Display Values of Variables - PRELIMINARY!

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In a perfect world, the analyzer's report would work like a debugger: jumping through stack frames, or even through diagnostic pieces, and printing symbolic values of variables at every piece would be really great.

I'm not entirely understanding the behavior you intend to have in this patch. Are you trying to print out values of all variables as diagnostic pieces? This might be a bit of an overkill, because many variables would be irrelevant to the bug. It could probably be better if a new kind of diagnostic pieces is introduced, instead of `PathDiagnosticEventPiece`, that would be handled by the consumers (text, html, plist) to somehow provide values on demand and avoid clamping up the screen.

Currently, we already highlight the last assignments for the "interesting" variables, which is implemented through, for example, `bugreporter::trackNullOrUndefValue()` - see how various checkers use it. This is, of course, far from perfect as well, because it's very hard to figure out which variables are of interest.

-----

In the first example, the user sees the "Entering loop body" control flow piece twice, from which it is clear that `i` is equal to 1. I disagree that an "assuming..." piece should be added here, because there is no assumption being made. The analyzer //knows// that `i` is first equal to 0, then it becomes 1; he doesn't need to assume it. A quick discussion on this subject happened in https://reviews.llvm.org/D23300.

That said, i agree that it'd be good to improve the existing "Entering loop body..." diagnostic to contain information about the value of `i`. The report is understandable without it, but it'd make a nice addition.

In the second example, `j` is still not being assumed, however upon meeting the condition `j < size + 1`, we should definitely mention that we //assume// that `size` is equal to `UINT_MAX` - because this is indeed an assumption, and we're failing to display it. It's a bug that definitely needs to be fixed, and the aforementioned https://reviews.llvm.org/D23300 was fixing similar bugs.

-----

To summarize:

- It's great to improve diagnostics, and you have pointed out some buggy or missing diagnostic pieces that make reports confusing.
- I'm not sure if we can, or should, afford the overkill of dumping all variables, though in some situations it may indeed be useful.
- I'm not seeing examples that aren't supposed to be covered by existing diagnostic mechanisms - fixing bugs in them might be equally useful and much easier and safer.
- I remember seeing such examples before, so, generally, i suspect that your approach may work, especially with a better UI.
- This is likely to require a more open discussion.


https://reviews.llvm.org/D34508