[PATCH] D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: NoQ, vsavchenko, xazax.hun, Szelethus, martong, balazske, baloghadamsoftware.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.
Herald added a project: clang.
steakhal requested review of this revision.

The main principle remained the same as the original `ArrayBoundCheckerV2`.
However, now during the //simplification// of `0 <= subscript-expr < extent` we will //assume// that no overflow/underflow could have happened during the evaluation of the outer-most operator of `subscript-expr`. This will aggregate such assumptions into the State.

The comments in the code should make the algorithm clear enough. Besides that, I will answer any questions that might arise.

---

**Plans**

- Add more tests, reorganize current ones.
- Substitute the template machinery of the `out-of-bounds-v2-false-positive-hunter.cpp` test file to make it run //significantly// faster [1].
- Improve the readability of the bug report using NoteTags or BRVisitors - yet to be decided which and how to implement that.
- Update docs to reflect the current implementation.
- Document that this checker uses a //heuristic//.

**NoteTags/BRVisitor**
I'm planning to come up with something to make the reports more readable. It could be a visitor or NoteTags, I haven't decided yet.
However, which makes this quite heroic is showing why a given expression is thought to be non-overflowing/under-flowing.
I don't have any source location beside the access itself. Imagine a pointer, which is modified line-by-line to point to the final destination which happens to be out-of-bound.
We check when it is dereferenced, but I can not put a NoteTag to the origin of that modification saying that 'assuming that this expression doesn't overflow/underflow'.
How can I implement meaningful diagnostics accomplishing this?

**Limitations**
If the extent is symbolic, but the access is concrete, then we will not constrain the symbolic extent to draw the access valid.
One might implement this later though.

This patch supersedes D86873 <https://reviews.llvm.org/D86873> and D86874 <https://reviews.llvm.org/D86874>, but implements the very same logic. I will abandon them if you feel this approach is better.

---

[1] I will rewrite the templated test-cases since the current implementation takes too much time to run.
The analyzer would have to simulate all the template magic just to get to the interesting memory access, which is an unacceptable slowdown IMO.
Currently, that single test file takes about 2.7 seconds to run.
Also, if any test-case fails, pretty hard to find the one that failed since warning locations shared across test-cases xD
Just to tackle the latter, I came up with a new debug expression checker that dumps the value of `PRETTY_FUNCTION` if a bug report with a given description is on the flight.
If you are interested, I can upstream that as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88359

Files:
  clang/docs/analyzer/checkers.rst
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/test/Analysis/out-of-bounds-false-positive.c
  clang/test/Analysis/out-of-bounds-new.cpp
  clang/test/Analysis/out-of-bounds-v2-false-positive-hunter.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88359.294502.patch
Type: text/x-patch
Size: 51692 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200926/b32e5534/attachment-0001.bin>