This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] trackNullOrUndefValue: deduplicate path pieces for each node.
ClosedPublic

Authored by NoQ on Dec 14 2017, 1:42 PM.

Details

Summary

This addresses the regression in D41254 in inlining/path-notes.cpp by adding a new straightforward mechanism that deduplicates path diagnostic pieces constructed by visitors for the same exploded graph node.

It would be incorrect to remove duplicate pieces globally for the whole report, because pieces are attached to source locations without node or even location context information, and source locations can easily coincide within a loop or a recursive function. It would also not be correct to deduplicate subsequent duplicate pieces, because the order in which visitors fire is not reliable, even if deterministic, so there may easily appear an unrelated diagnostic between duplicate diagnostics attached to the same node.

The regression in D41254 appeared because FindLastStoreBRVisitor was added twice - directly by DivZeroChecker for the divisor and indirectly by ReturnVisitor for the function call return value to which the divisor was tracked. However, the EnableNullFPSuppression flag for the two copies of the visitor had different value: true for the former, false for the latter. It is false for the latter because we want to suppress reports on "return null" paths from functions (even if it wasn't an inlined defensive check, just plain null) only when "null" is a pointer. So we get two different visitors finding last store to the visitor.

For now, because we are completely unprotected from such issues, i thought that simply deduplicating nodes should make the whole thing safer without obvious downsides. In general, just turning this whole machinery into a single big visitor might be a better idea.

This also reminds us that duplicate code in path diagnostic generators (the way i made three identical changes here) is worth cleaning up.

Diff Detail

Event Timeline

NoQ created this revision.Dec 14 2017, 1:42 PM
NoQ edited the summary of this revision. (Show Details)Dec 14 2017, 1:54 PM
dcoughlin accepted this revision.Dec 17 2017, 5:25 PM

Looks good to me.

This revision is now accepted and ready to land.Dec 17 2017, 5:25 PM
This revision was automatically updated to reflect the committed changes.