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

donat.nagy closed this revision. donat.nagy added a comment. Committed in rGde2547329b41ad6ea4ea876d12731bde5a6b64c5 (which accidentally refers to a wrong Phabricator review). Repository: rG LLVM Github Monorepo CHANGES S

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

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. In D148355#4294798 , @steakhal wrote: > In D148355#4294738 , @donat.nagy > wrote: > >> @steakhal I marke

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

steakhal added a comment. In D148355#4294738 , @donat.nagy wrote: > @steakhal I marked a few comments as Done (I accidentally missed some when I > was creating the most recent patch) and now the only not-Done thing is the > followup commit for refactor

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

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173 + const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace(); + if (SR->getKind() != MemRegion::UnknownSpaceRegionKind) { +// a pointer to UnknownSpaceRegionK

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

donat.nagy marked 4 inline comments as done. donat.nagy added a comment. @steakhal I marked a few comments as Done (I accidentally missed some when I was creating the most recent patch) and now the only not-Done thing is the followup commit for refactoring the optionalness of RegionRawOffsetV2.

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

steakhal added a comment. Please mark comments "Done" where applicable. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173 + const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace(); + if (SR->getKind() != MemRegion::UnknownSpaceRegionKind) {

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

donat.nagy marked an inline comment as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:173 + const MemSpaceRegion *SR = rawOffset.getRegion()->getMemorySpace(); + if (SR->getKind() != MemRegion::UnknownSpaceRegionKin

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

donat.nagy updated this revision to Diff 516365. donat.nagy marked 11 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148355/new/ https://reviews.llvm.org/D148355 Files: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.

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

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:167-168 - NonLoc rawOffsetVal = rawOffset.getByteOffset(); - - // CHECK LOWER BOUND: Is byteOffset < extent begin? - // If so, we are doing a load/store - // before the f

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

donat.nagy added a comment. @steakhal Thanks for the review, it was really instructive :). I'll probably upload the updated commit on Monday. What kind of measurements are you suggesting? I analyzed a few open source projects with the patched Clang SA (see results in my previous commit) and a

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

steakhal added a comment. Okay, so you just added some numbers :D Now it's my turn I guess. I'll compare this version against the version I shared, and use in our product. Stay tuned. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148355/new/ https

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

steakhal added a comment. I like this. Looks really solid. I only had style nitpicks. Have you run measurements? After that we could land it, I think. FYI: I'll also schedule a test run with this patch. You should expect the results in a week - not because it takes that long, but because I need

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

donat.nagy added a comment. I tested this patch on a few open source projects and it significantly reduced the number of alpha.security.ArrayBoundV2 reports: | Project name | memcached | tmux | curl | twin | vim | openssl | | Baseline report #| 0 | 2| 13 | 6

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

donat.nagy updated this revision to Diff 515326. donat.nagy edited the summary of this revision. donat.nagy added a comment. I'm publishing the extended variant of this commit (which I mentioned in earlier comments). This generalizes the first version of the commit to check for the unsigned-vs-

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

steakhal added a comment. Sounds good. Thanks for clarifying. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148355/new/ https://reviews.llvm.org/D148355 ___ cfe-commits mailing list cfe-commits@lists.llv

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

donat.nagy added a comment. @steakhal Then I'll clean up my own solution, which will be completely independent of the patch of Tomasz (except for the obviously identical changes in the test code). The de-duplication that I'm planning is not pure NFC, because it'll probably affect some mostly-t

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

steakhal added a subscriber: tomasz-kaminski-sonarsource. steakhal added a comment. In D148355#4279866 , @donat.nagy wrote: > @steakhal Thanks for the background information! > > I didn't know about D86874 so I indeed >

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

donat.nagy added a comment. @steakhal Thanks for the background information! I didn't know about D86874 so I indeed ended up with something very similar to it. I reviewed D88359 and I knew that it's a completely general soluti

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

steakhal added a comment. This subject haunted us for quite some time now, and there is more behind what it seems at first. If I'm not mistaken, this patch is pretty much what is in D86874 except that it doesn't give up and still checks the upper-bound in this

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

donat.nagy created this revision. donat.nagy added reviewers: steakhal, NoQ, gamesh411, Szelethus. donat.nagy added a project: clang-tools-extra. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a proje