[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added a comment.   Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG25b9696b61e5: [analyzer] Upstream BitwiseShiftChecker (authored by donat.nagy). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 550706. donat.nagy added a comment. A few minutes ago I accidentally re-uploaded the previous version of the patch; now I'm fixing this by uploading the actually updated variant. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 2 inline comments as done. donat.nagy added inline comments. Comment at: clang/docs/analyzer/checkers.rst:81 + +Ensure the shift operands are in proper range before shifting. + donat.nagy wrote: > steakhal wrote: > > We should exactly elaborate

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 550703. donat.nagy added a comment. Updated the release notes. This could be the final version of this commit if you agree that it's good enough. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files:

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. In D156312#4589251 , @steakhal wrote: > I don't think we should do anything about it unless it's frequent enough. > Try to come up with a heuristic to be able to measure how often this happens, > if you really care. > Once

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal accepted this revision. steakhal added a comment. In D156312#4588406 , @donat.nagy wrote: > After investigating this issue, I added the testcases > `signed_aritmetic_{good,bad}` which document the current sub-optimal state. > The root cause

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. (The release notes entry is still missing, I'll try to add it tomorrow.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 ___

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 550318. donat.nagy edited the summary of this revision. donat.nagy added a comment. I verified that the checker handles the examples in the documentation correctly (and added them to the test suite). However, as I was tweaking the examples in the

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. The test coverage is also impressive. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 549978. donat.nagy marked 7 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files: clang/docs/analyzer/checkers.rst

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 13 inline comments as done. donat.nagy added a comment. I handled the inline comments, I'll check the example code and edit the release notes tomorrow. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:272 + if (const auto ConcreteRight

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added a comment. Great progress. Now, moving to the docs. Have you checked that the docs compile without errors and look as you would expect? Please post how it looks. Please make a note about this new checker in the clang's release docs, as we usually announce new checkers and major

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 549091. donat.nagy marked 2 inline comments as done. donat.nagy added a comment. Uploading a new version based on the reviews. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 21 inline comments as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:29-30 + +using namespace clang; +using namespace ento; + steakhal wrote: > This is used quite often and hinders

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 4 inline comments as done. donat.nagy added a comment. Thanks for the suggestions, I answered those where I had something to say and I'll implement the rest of them. In D156312#4576120 , @steakhal wrote: > Have you considered checking

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added a comment. I haven't gone deep into the implementation, but by only looking at the quality of the code and the structure I assume it implements the right behavior. I've checked the tests though, and they look good. I only found irrelevant nits. Awesome work. --- Have you

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302 + pluralSuffix(MaximalAllowedShift)); +R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(), +

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 6 inline comments as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:94-98 +void BitwiseShiftValidator::run() { + if (isValidShift()) { +Ctx.addTransition(State, createNoteTag()); + } +}

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 548626. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked 2 inline comments as done. donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302 + pluralSuffix(MaximalAllowedShift)); +R->addNote(LeftNote, PathDiagnosticLocation{LHS,

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy planned changes to this revision. donat.nagy marked 5 inline comments as done. donat.nagy added a comment. I'll soon upload a refactored version. Comment at: clang/docs/analyzer/checkers.rst:44-50 +Moreover, if the pedantic mode is activated by +``-analyzer-config

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy marked an inline comment as done. donat.nagy added a comment. @steakhal Thanks for the review! I answered some of your questions; and I'll handle the rest soon. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:150 + SVB.evalBinOp(State,

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

steakhal added inline comments. Comment at: clang/docs/analyzer/checkers.rst:35 +core.BitwiseShift (C, C++) +""" + Comment at: clang/docs/analyzer/checkers.rst:44-50 +Moreover, if the pedantic mode is activated by

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302 + pluralSuffix(MaximalAllowedShift)); +R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(), +

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

gamesh411 added a comment. I like this, especially, that the functionality of UBOR can be merged and extended in a much cleaner way (architecturally). Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302 +

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. In D156312#4537200 , @NoQ wrote: >> (4) UBOR exhibits buggy behavior in code that involves cast expressions > > Hmm, what makes your check more resilient to our overall type-incorrectness? The code of UBOR

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

NoQ added a comment. > (1) UBOR is only triggered when the constant folding performed by the Clang > Static Analyzer engine determines that the value of a binary operator > expression is undefined Yes I wholeheartedly agree, these checks are pre-condition checks, they should have never been

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy updated this revision to Diff 544317. donat.nagy added a comment. (Fix incorrect filename mark in the license header.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156312/new/ https://reviews.llvm.org/D156312 Files:

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy added a comment. I tested this commit on several open source projects (in the default mode Pedantic=false), the following table summarizes the results: | vim_v8.2.1920_baseline_vs_vim_v8.2.1920_with_bitwise_shift | New reports

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

donat.nagy created this revision. Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, gamesh411, dkrupp, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All. donat.nagy requested review of this