[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG14742443a258: Reland [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible (authored by tomasz-kaminski-sonarsource, committed by

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource added a comment. Thank you for additional checks. Unfortunatelly, I am not able to perform commits for this and next wee (up to 20th of May), so I wonder if you could commit this one for me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Everything passed this time. Thanks for the patience. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 428052. tomasz-kaminski-sonarsource added a comment. Changed variable name and formatting to match coding style. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

uabelho added a comment. In D124658#3500467 , @steakhal wrote: > [...] > Could you please give this another shoot at this @uabelho? Yes, the case that crashed for me works with the updated patch. Haven't done other testing. CHANGES SINCE LAST

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal added a comment. This time I've checked this on our test set, and it seems to run without any issues. I'm still waiting for the results of analyzing LLVM, but I think safe to say that this should work. Could you please give this another shoot at this @uabelho?

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 428030. tomasz-kaminski-sonarsource added a comment. Applied clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 428011. tomasz-kaminski-sonarsource added a comment. Firstly, my apologies for the back and forth. My repo was in some strange state, and I wasn't picking up all the changes that got reverted in my patch (this is why the patch was not

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 428003. tomasz-kaminski-sonarsource added a comment. Updated and manually validated the patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal reopened this revision. steakhal added a comment. This revision is now accepted and ready to land. When I download the "raw diff" using the `Download Raw Diff` button on this page, then I try to `git apply` it still fails. (error `219: new blank line at EOF.`) Anyway, I managed to

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427999. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp clang/test/Analysis/additive-op-on-sym-int-expr.c Index:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427993. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp clang/test/Analysis/additive-op-on-sym-int-expr.c Index:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

uabelho added a comment. Not sure if I'm doing something wrong but I can't apply this patch on current top of tree (a4190037fac0 ) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal added a comment. In D124658#3499883 , @tomasz-kaminski-sonarsource wrote: > Updated patch to match master after revert. It's still messed up. Please double-check. Comment at:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427982. tomasz-kaminski-sonarsource added a comment. Updated patch to match master after revert. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal added a comment. The patch doesn't apply to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource added a comment. Added missing casts in `truncatingToUnsigned` tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 ___ cfe-commits mailing list

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427717. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp clang/test/Analysis/additive-op-on-sym-int-expr.c Index:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal added a comment. @uabelho Could you please apply this change and check if it resolves your crashes? I'm gonna do the same on our side. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 ___

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427695. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp clang/test/Analysis/additive-op-on-sym-int-expr.c Index:

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427693. tomasz-kaminski-sonarsource added a comment. I have removed the assertions and updated the code to handle both extensions of reductions of bitwith from RHS to ResultType. Expanded test, to cover the above scenarios. CHANGES

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource added a comment. I am looking into the crashes. The simplest solution is to not apply normalization in such a situation, and I am slowly converting to that solution. But, want to understand better how such values are produced in the first case, and if performing

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

This revision was automatically updated to reflect the committed changes. Closed by commit rGda5b5ae852c4: Revert [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible (authored by steakhal). Changed prior to commit: https://reviews.llvm.org/D124658?vs=427340=427582#toc

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

uabelho added a comment. In D124658#3495973 , @steakhal wrote: > This patch triggers a crash with this minimized example. > assertion at L205: `"The result operation type must have at least the same > number of bits as its operands."` > [...] > Please

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal reopened this revision. steakhal added a comment. This revision is now accepted and ready to land. This patch triggers a crash with this minimized example. assertion at L205: `"The result operation type must have at least the same number of bits as its operands."` // build/bin/clang

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

This revision was automatically updated to reflect the committed changes. Closed by commit rGf0d6cb4a5cf5: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible (authored by tomasz-kaminski-sonarsource, committed by steakhal). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource added a comment. I do not have commit rights. Would it be possible to you to commit the changes on my behalf? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 ___

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Seems right. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 ___ cfe-commits

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427245. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp clang/test/Analysis/additive-op-on-sym-int-expr.c

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal added a comment. Just a few nits left. Consider marking 'done' the corresponding boxes. Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:211 +// Check if the negation of the RHS is representable: +// * if resultTy is unsigned, then negation is

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 427001. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp clang/test/Analysis/additive-op-on-sym-int-expr.c

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 426999. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658 Files: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp clang/test/Analysis/additive-op-on-sym-int-expr.c

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:204 +// subtraction/addition of the negated value. +if (!RHS.isNegative()) { + ConvertedRHS = (resultTy, RHS); steakhal wrote: >

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:204 +// subtraction/addition of the negated value. +if (!RHS.isNegative()) { + ConvertedRHS = (resultTy, RHS); tomasz-kaminski-sonarsource wrote: >

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource updated this revision to Diff 426056. tomasz-kaminski-sonarsource added a comment. Uploading updated diff with suggested fixes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124658/new/ https://reviews.llvm.org/D124658

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

tomasz-kaminski-sonarsource marked 2 inline comments as done. tomasz-kaminski-sonarsource added a comment. > Could you please elaborate on this? I understand that you'd like to simplify > certain binary operations by merging the RHS and the operand, however, I > don't see what are the empirical

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

martong added a comment. > This avoid producing very large RHS in case when the symbol is cased to > unsigned number, and as consequence makes the value more robust in presence > of cast. Could you please elaborate on this? I understand that you'd like to simplify certain binary operations by

[PATCH] D124658: [analyzer] Canonicalize SymIntExpr so the RHS is positive when possible

steakhal added a comment. It looks good at first glance. For the review, it would be nice to see which `clang_analyzer_dumps` and `reachables` change with the patch. Could you mark the ones which have different results depending on whether you apply your fix or not? E.g put a `no-warning`