krememek added a comment. I think the functionality started here by doing something clever with loops is complex enough to warrant putting this into a separate .cpp file. We can keep this here for now, but this seems like complexity that is going to naturally creep up and make this file even more monolithic than it already is.
This change also unconditionally never evaluates the body of a loop if the loop bound is constant. That seems like a **major** degradation in precision, as it is worth analyzing the loop body at least once to get full precision of the paths explored within the loop body. That's why I suggest unrolling at least a couple times. The first unrolling will get good coverage of the code within the loop, and the second unrolling will capture some correlated conditions across loop iterations that can diagnose some interesting behavior. After the second unrolling (or 'nth' for some 'n'), this widening seems applicable. I've also seen some static analyzers that focused on buffer overflow detection unroll looks like this by simply extending 'n' to be the constant loop bounds, opting for maximum precision at the cost of analyzing all those iterations (which isn't necessarily the best option either, but it is an option). It seems the patch needs two things: 1. A general scheme for widening, which can just be invalidation of anything touched within a loop. I think that can be done by having an algorithm that does an AST walk, and looks at all variables whose lvalues are taken (used to store values or otherwise get their address using '&') or anything passed-by-reference to a function. That set of VarDecls can be cached in a side table, mapping ForStmt*'s (other loops as well) to the set of VarDecls that are invalidated. Those could then be passed to something that does the invalidation using the currently invalidation logic we have in place. The invalidation logic should also handle checker state, which also needs to get invalidated alongside variable values. 2. We need to consult the number of times a loop has been executed to determine when to apply this widening. We can look at the basic block counts, which are already used to cull off too many loop iterations. This patch cannot go in as is, as it will seriously degrade the quality of the analyzer without further refinements. It is a great start, and I would be supportive of the patch landing more-or-less as is if it is behind a flag, and not on by default until the improvements suggested are made. http://reviews.llvm.org/D12358 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
