[PATCH] D148355: [analyzer] Fix comparison logic in ArrayBoundCheckerV2

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

steakhal added a subscriber: tomasz-kaminski-sonarsource.
steakhal added a comment.

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

> @steakhal Thanks for the background information!
>
> I didn't know about D86874 <https://reviews.llvm.org/D86874> so I indeed ended up with something very similar to it. I reviewed D88359 <https://reviews.llvm.org/D88359> and I knew that it's a completely general solution of this issue, but I felt that it's too complicated and wanted to create a patch with shorter code than that.
>
> I really like the "use zero instead of negative numbers" trick in the SonarSource patch; if you would upload that for a review, I'd strongly support merging it.
>
> Another alternative is that I'm working on a new version of my patch, which would eliminate the code duplication between the underflow and overflow checks (by introducing a single function compareValueToThreshold that performs offset simplification when needed, handles the unsigned-vs-negative case, calls evalBinOpNN and invokes state->assume). This would be equivalent to the SonarSource patch (it handles unsigned-vs-negative comparison on "both sides") with the added independent benefit of simplifying the codebase. However, I can also do this code simplification as a separate patch after merging the SonarSource solution for the bug.
>
> Which solution would you prefer (upstream the solution used by SonarSource + separate code quality improvement or the combined refactor-and-check-before-evalBinOpNN variant that I could implement)?

Right now I don't have the bandwidth for upstreaming our version, and it seems like you already picked up the subject, so it's probably less intrusive for you. But I should speak for my self I know.
So, you can update this revision to our version, or modify or inspire by our version. Anything you want.
The only thing is, if the patch, in the end, is sufficiently similar to ours, I think we should add credit to the original author, who is I think @tomasz-kaminski-sonarsource (`Tomasz Kamiński <tomasz.kaminski at sonarsource.com>`).

I can help to review and do measurements at scale but I don't think I can contribute in other ways.

And on the note of doing refinement followup patches. If those are NFC (non-function changes), then those should be separated from semantic changes for sure.
Pure NFC changes make validation much easier, equally so the review process. If not, then it's up to you how you think it's the easiest to review the patch set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148355