[PATCH] D34508: [Analyzer] Bug Reporter Visitor to Display Values of Variables - PRELIMINARY!
baloghadamsoftware added a comment. I am considering to restrict the assumptions to nodes marked as interesting and to the location of the bug. However, I have difficulties with the latter, it seems that the bug location itself is not part of the bug path. https://reviews.llvm.org/D34508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34508: [Analyzer] Bug Reporter Visitor to Display Values of Variables - PRELIMINARY!
xazax.hun added a comment. In https://reviews.llvm.org/D34508#791048, @NoQ wrote: > 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. I think concentrating on "interesting" things would be a great addition to this patch. So in case an `elementRegion` is marked as interesting, we could also mark the index as interesting automatically and later print out the relevant information only for interesting symbols, regions. https://reviews.llvm.org/D34508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34508: [Analyzer] Bug Reporter Visitor to Display Values of Variables - PRELIMINARY!
zaks.anna added a comment. I agree that we should not print the values of all variables. The users will be overwhelmed with the huge amount of information. It is very valuable to highlight just the right information. (I believe even the current diagnostics often produce too much info and highlighting the more important notes would be very valuable.) However, the examples you presented are **very compelling**. Are there ways we could highlight the same information without printing values of all variables? For example, for the overflow case, we could experiment with printing notes along the path at the locations a variable overflows. This would be very valuable for the array overflow alpha checker, which often flags the bugs that only occur if an integer value along the path overflows. I am not sure how noisy this approach will be. If it is too noisy, we could refine this further. For the uninitialized variable example, we could have some pattern-matching logic that would check if the expression is an array element and if so print the value of the index. (Another problem with variables that are failed to be initialized in a function is that we do not display the path on which the variable is not initialized, making it hard to understand the reports. Though, that is a separate problem.) https://reviews.llvm.org/D34508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34508: [Analyzer] Bug Reporter Visitor to Display Values of Variables - PRELIMINARY!
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
