[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov added a comment. @NoQ > Even though NFC commits don't require tests, this doesn't mean they shouldn't > add them! While developing this, I wasn't able to reproduce any regression or unpassed cases on the projects from CodeChecker list (//Bitcoin_v0.20.1, Curl_curl-7_66_0,

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

NoQ added a comment. I'm catching up and these changes look great. In D90157#2433657 , @steakhal wrote: > I've run the baseline and your patch as well on 15 projects, both with and > without Z3 refutation enabled. > (...) > All in all, I'm still in

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

This revision was automatically updated to reflect the committed changes. Closed by commit rG13f4448ae7db: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way (authored by ASDenysPetrov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov added a comment. Hi, guys! Does anyone can review this item, except @steakhal (thanks him)? Please, look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90157/new/ https://reviews.llvm.org/D90157 ___ cfe-commits mailing list

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

steakhal added a comment. In D90157#2519800 , @ASDenysPetrov wrote: > Updated. Naming fixes. Member access changes. Looks great! IMO we are sort-of late to commit this into clang-12 with confidence. We should land it after we tag that release. What do

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov updated this revision to Diff 318991. ASDenysPetrov added a comment. Updated. Naming fixes. Member access changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90157/new/ https://reviews.llvm.org/D90157 Files:

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:647-650 + // Array to pointer. + if (isa(OriginalTy)) +if (CastTy->isPointerType() || CastTy->isReferenceType()) return UnknownVal(); ASDenysPetrov wrote: >

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov added a comment. @steakhal > Why don't you use the `SValVisitor` instead? I simply didn't know of its exsistence. We can try to transform this patch using `SValVisitor` in the next revision to make the review easier and avoid additional complexity. Comment

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

steakhal added a comment. What you are basically doing is a visitation on the kind of the `SVal`. Why don't you use the `SValVisitor` instead? That way you could focus on the leaf nodes in the hierarchy, leaving you an even cleaner solution. class CastEvaluator : public SValVisitor {

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov updated this revision to Diff 317351. ASDenysPetrov added a comment. @steakhal Oh, sure. Updated. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90157/new/ https://reviews.llvm.org/D90157 Files: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

steakhal added a comment. Please @ASDenysPetrov, give full context with the option `-U` when you export the diff from git. I would like to quickly swipe through the code before I accept this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90157/new/

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov updated this revision to Diff 317342. ASDenysPetrov added a comment. Unfortunately I wasn't able to reproduce the singe difference in PostgreSQL project. So I revised the patch in a part of the difference nature. This update passed all of CodeChecker's tests without any difference.

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov added a comment. Thank to @steakhal for the new tests. //All// previous result differences have **gone** in `FFMPEG` and `twin` but a new //one// **appeared** in `PostgreSQL`. /src/common/d2s.c Line 895 Memory copy function overflows the destination buffer

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov updated this revision to Diff 313114. ASDenysPetrov added a comment. @steakhal I've precisely inspected your reports and find the bug. I've fixed it. I also verified all the rest and find some other vulnerabilities and fixed them as well. Thank you for your testing, and I would

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov added a comment. @steakhal Thank you for your invaluable analysis! > What do you think @ASDenysPetrov ? I think if there is any difference then I have to inspect the changes deeper. I tryed to make this changes in a way of not changing any behaviour. Let me back to this patch a

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

steakhal added a comment. In D90157#2361773 , @ASDenysPetrov wrote: > Who can confirm if this is correct or somewhere it needs fixes? Here is a > generated result of `evalCast` from the origin branch(before the patch): > > void foo(int* x, //

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov added a comment. Who can confirm if this is correct or somewhere it needs fixes? Here is a generated result of `evalCast` from the origin branch(before the patch): void foo(int* x, // {reg_$0} int** y, // {reg_$0} int***z) // {reg_$0} { (void*)x;

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

steakhal added a comment. In D90157#2356170 , @martong wrote: >> Now I'm going to make unittests for SValBuilder::evalCast. Let me ask you to >> offer me some examples of casts to check. As many cases we can collect as >> robust the test would be. E.g.

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

martong added a comment. > Now I'm going to make unittests for SValBuilder::evalCast. Let me ask you to > offer me some examples of casts to check. As many cases we can collect as > robust the test would be. E.g. This is not going to be easy. I guess we should cover both implicit and explicit

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

ASDenysPetrov created this revision. ASDenysPetrov added reviewers: NoQ, steakhal, krememek, xazax.hun. ASDenysPetrov added a project: clang. Herald added subscribers: cfe-commits, martong, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.