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

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

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

In D150446#4337657 <https://reviews.llvm.org/D150446#4337657>, @donat.nagy wrote:

> After some thinking and discussion with @gamesh411 I decided that it'd be better to replace the callback used by this checker. This is a clean but rough draft of this concept; for a final version I'd consider:
>
> - adding  a secondary callback that handles `*ptr` equivalently to `ptr[0]`;
> - including a special case that it's valid to form an after-the-end pointer as `&arr[N]` where `N` is the length of the array;
> - including a check::Location callback that creates bug reports when these after-the-end pointers are dereferenced.
>
> @steakhal What do you think about this design direction?

Could you elaborate on this part?
To clarify my position, I think we can go with either approach. We can stay with the check::Location, or with the PreStmt route.
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.
For example, we can sacrifice some TPs for dropping a large number of FPs. That seems to be a good deal, even if we "regress" on some aspect.

---

Anyway, I just checked the impact of this patch and it was so big that I rerun the measurement to be sure (thus the delay :D), but it didn't get any better.
There are a lot more reports if I apply this patch, which suggests to me that there is something wrong.

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
We also lose some reports like this: https://godbolt.org/z/8113nrYhe

Please investigate it and also do comparative checks on real codebases to make sure the change is aligned with the expectations.
Given that some of our users depend on the current behavior, we should ensure that no reports disappear or appear unless they are justified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150446