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

Wed Apr 19 03:14:42 PDT 2023

donat.nagy added a comment.

@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)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148355