This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Refactor ArrayBoundCheckerV2
Needs ReviewPublic

Authored by steakhal on Aug 31 2020, 4:53 AM.

Details

Summary

This patch refactors the ArrayBoundCheckerV2 to use more SValVisitor machinery.
IMO this pattern leads to a more functional style, thus more readable - compared to an ad-hoc recursion what getSimplifiedOffsets did.
I also drastically reduce the scope of the mutated local variables for readability.
This resulted in a fairly large change:

  • Use MemRegionVisitor to compute RegionRawOffsetV2 of a memory region.
  • Use SymExprVisitor to simplify subscript expression.
  • Remove getSimplifiedOffsets function.
  • Split up ArrayBoundCheckerV2::checkLocation into checkLowerBound and checkUpperBound.

Diff Detail

Event Timeline

steakhal created this revision.Aug 31 2020, 4:53 AM
steakhal requested review of this revision.Aug 31 2020, 4:53 AM
martong added inline comments.Sep 1 2020, 1:48 AM
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
52

Since you are already deep in refactoring. This calculation together with the enclosing class seems to be useful for other Checkers too. Would make sense to put this to CheckerHelpers?
E.g. the PlacementNewChecker could have benefited from this class if it had been available for use during the implementation of the checker.

298

What is the exact difference in between this algorithm and assumeInBound? What are the benefits (and disadvantages) of using each of them?

And oh wait, there's more: assumeInclusiveRange. Is that also related? (StdLibraryFunctionsChecker uses that for range assumptions.)

Before we dive into this too much, if you can point to discussion that explains why we have a 2 versions of the same checker, that would be nice. Why did you chose to work on this one, and not the other? I recall us talking about this in a meeting, but its always great to have it set in stone.

steakhal added inline comments.Sep 1 2020, 2:02 AM
clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
52

You are probably right.
However, I'm not qualified enough to make such a decision.

By the same token, I don't really understand why would anyone do such unfolding by hand?
Could someone clarify the use-cases for RegionRawOffset and RegionRawOffsetV2? @NoQ @vsavchenko @Szelethus?
Eg. we don't use such facilities in the evalBindOpL# functions, but it would probably come handy.

171–172

Here we 'c-style' integral cast the right-hand side value to match the signess and bitwidth of Index. @martong