[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfd7efe33f1b2: [analyzer] Fix cast evaluation on scoped enums in ExprEngine (authored by steakhal). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM! Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85528/new/ https://reviews.llvm.org/D85528 ___ cfe-commits mailing list

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal updated this revision to Diff 424497. steakhal added a comment. Added the missing `-verify` flag for the new RUN line. Also, add an elaborate description in the test for explaining the situation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85528/new/

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal updated this revision to Diff 424470. steakhal added a comment. Added two new RUN lines, demonstrating the behavior of `support-symbolic-integer-casts={true,false}`. Also refined the match string to be less fuzzy. It helps to grasp the difference between the expectations. CHANGES

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

martong added a comment. I'd like to have a guarantee that if `Opts.ShouldSupportSymbolicIntegerCasts` is set to `true` then the `SymboCast` is produced both for the scoped and the unscoped enums. Could you please have an additional lit test for that? At some point we'd like to make

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal updated this revision to Diff 423624. steakhal edited the summary of this revision. steakhal added a comment. Herald added a subscriber: manas. Herald added a project: All. rebased; I'm still interested in this. WDYT @martong BTW we have this downstream for about two years now without

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. I really want to move this forward so I made a further evaluation on this, on the MongoDB project. The analysis took approx. 22 and half hours on 24 cores for the baseline and for this revision as well. There were 7116 common reports, 5 disappeared and new 34 were

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. The patch doesn't seem to affect the reports. It only introduced 3 new reports during the analysis of clang + clang-tidy. The analysis times indeed increased slightly, ~~ +3%. Overall, I think it's a valuable patch, which resolves crashes when the Z3 refutation

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

NoQ added a comment. With Z3 //constraint// manager you absolutely want your constraints to be as precise as possible. The only reason we don't add these casts is because it confuses the constraint manager a lot. With a better constraint manager we would have spared ourselves a lot of

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. In D85528#2307785 , @NoQ wrote: > Aha, ok, thanks, I now understand what the problem is because I was able to > run the test before the patch and see how the patch changes the behavior. > > What do you think about flattening the

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

NoQ added a comment. Aha, ok, thanks, I now understand what the problem is because I was able to run the test before the patch and see how the patch changes the behavior. What do you think about flattening the enum type out entirely? I.e., instead of `(unsigned char) conj_$2{enum

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. In D85528#2232074 , @xazax.hun wrote: > I'm not opposed to landing this to master, as we will have more time there to > see whether there are any unwanted side effects in practice. I made some experiments on the following

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

xazax.hun added a comment. In D85528#2229309 , @steakhal wrote: > It would be nice to have this fix in clang11. > Do you think it's qualified for it? Unfortunately, this is not a fix only change. This is a fix for refutation, but also a behavioral

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. It would be nice to have this fix in clang11. Do you think it's qualified for it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85528/new/ https://reviews.llvm.org/D85528 ___

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal updated this revision to Diff 286209. steakhal marked an inline comment as done. steakhal added a comment. Add an extra `RUN` line without //refutation//. The expected result is the same as with refutation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

xazax.hun added a comment. In D85528#2205799 , @NoQ wrote: > I expect at least one LIT test //without// `-analyzer-config > crosscheck-with-z3=true` Agreed. I think the code snippet I proposed would be a great test to include with this revision.

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

whisperity added inline comments. Comment at: clang/test/Analysis/z3-refute-enum-crash.cpp:5 +// +// Requires z3 only for refutation. Works with both constraint managers. + You can have multiple `RUN` lines in the same test file, which will result in the tests

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

NoQ added a comment. In D85528#2204664 , @steakhal wrote: > In D85528#2203325 , @NoQ wrote: > >> Because this patch changes the behavior of regular analysis (without Z3), i >> expect tests to reflect that. > >

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. I mean, this shouldn't be an issue. Since we already omitted the 'unnecessary' cast expressions... That decision is the root cause of this, we should have expected that. IMO we do the right thing here. If we want to treat sym and sym2 to refer to the same symbol, we

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. In D85528#2205094 , @xazax.hun wrote: > Looks reasonable to me, but I am not very familiar with the impacts of the > additional casts. Do we lose some modeling power when we are using the > regular constraint solver? > > For

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

xazax.hun added a comment. Looks reasonable to me, but I am not very familiar with the impacts of the additional casts. Do we lose some modeling power when we are using the regular constraint solver? For example, when we have constraints both on the original and the cased symbol can the

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. In D85528#2203325 , @NoQ wrote: > Aha, ok, sounds like the right thing to do. Like, for Z3 it's actually the > wrong thing to do (you'd prefer to evaluate the cast perfectly by adding > `SymbolCast`) but for pure

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal updated this revision to Diff 284126. steakhal marked 3 inline comments as done. steakhal edited the summary of this revision. steakhal added a comment. - Using `dump`s instead of `reaching` in tests. - Not requiring complete enums anymore //(unlike we did before the patch)//.

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

NoQ added a comment. Aha, ok, sounds like the right thing to do. Like, for Z3 it's actually the wrong thing to do (you'd prefer to evaluate the cast perfectly by adding `SymbolCast`) but for pure RangeConstraintManager this is the lesser of two evils. Because this patch changes the behavior

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal updated this revision to Diff 283936. steakhal added a comment. - Moved the FIXME closer to the subject. - Added tests for covering incomplete enums as well. - Added `REQUIRES: z3`. This will mark the test case `unsupported` on every buildbots. See my notes about this behavior at D83677

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

whisperity added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:98-104 +const auto IsIntegralOrUnscopedCompleteEnumerationType = [](QualType Ty) { + const Type *CanonicalType = Ty.getCanonicalType().getTypePtr(); +

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

whisperity added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:98-104 +const auto IsIntegralOrUnscopedCompleteEnumerationType = [](QualType Ty) { + const Type *CanonicalType = Ty.getCanonicalType().getTypePtr(); +

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal planned changes to this revision. steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:98-104 +const auto IsIntegralOrUnscopedCompleteEnumerationType = [](QualType Ty) { + const Type *CanonicalType =

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

vsavchenko added a comment. Good catch! Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:98-104 +const auto IsIntegralOrUnscopedCompleteEnumerationType = [](QualType Ty) { + const Type *CanonicalType =

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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