[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Thanks for the review and the testing!

>> [...] What do you think about this design direction?
>
> Could you elaborate on this part?

In short, this review should've been a discourse thread about my "switch to the PreStmt route" plan: I wanted to ask for architectural / high-level opinions before working on the details. If you or other reviewers don't reject this plan outright, I'll flesh out the secondary features (e.g. recognizing that `*(arr+5)` is equivalent to `arr[5]`).

> To me, the only important aspect is to not regress, or prove that the places where we regress are for a good reason, and in the end we provide more valuable reports to the user.

My goal is to turn this checker into a useful and stable (i.e. non-alpha) checker as soon as possible.

On this path eliminating FPs (and improving the messages) is a high priority goal, but I would freely accept losing TPs as long as the remaining ones are still enough to be useful. (Theoretical example: if the current code reports a certain unusual TP (with a spartan message), but it'd be difficult to give a good error message in that situation, then I'd prefer sacrificing that TP on the altar of quick progress. Once the checker is out of alpha, we can re-add support for non-essential cases like that.)

> For instance, in this example, we currently don't report any warnings, but with the patch, we would have some. https://godbolt.org/z/sjcrfP8df

I have a suspicion that these issues might be caused by the heuristics that I'm using to guess the "meaning" of the ElementRegions (I marked them with an inline comment). I'll examine and troubleshoot them, but probably only during the next week, because I'm in the middle of another task (upstreaming the ericsson-internal BitwiseShift checker) right now and I'd like to reach a milestone on it (e.g. starting the open source review).

> We also lose some reports like this: https://godbolt.org/z/8113nrYhe

I know that this commit does not handle "array indexing" that's expressed with the pointer dereferencing operator. Re-adding this feature is a very straightforward task, and nothing depends on it, so I'll only invest work into it if I see that the other, less clear steps of my plans are successful.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:318-319
+    const ElementRegion *ER;
+    while ((ER = dyn_cast_or_null<ElementRegion>(R)) &&
+           ER->getElementType() == ElemType) {
+      // Special case to strip ElementRegion layers that represent pointer
----------------
This is the heuristic that I'm talking about in the main comment block. Note that the new code only strips ElementRegion layers where the type is the same, while the old code stripped all ElementRegion layers (and thus produced crazy FNs on multidimensional arrays).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150446/new/

https://reviews.llvm.org/D150446