[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

chrish_ericsson_atx added a comment. I believe this exposed another odd issue where a true positive (enabled by this commit) disappears when unrelated code is not present. Bug filed as: https://bugs.llvm.org/show_bug.cgi?id=51950. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

chrish_ericsson_atx added a comment. I believe this commit exposed a new false-positive bug in [core.DivideZero]. I've filed the report here: https://bugs.llvm.org/show_bug.cgi?id=51940 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2838065 , @martong wrote: > In D103314#2837907 , @uabelho wrote: > >> Hi, >> >> Another failed assertion that started appearing with this patch: >> >> clang --analyze

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added a comment. In D103314#2838065 , @martong wrote: > In D103314#2837907 , @uabelho wrote: > >> Hi, >> >> Another failed assertion that started appearing with this patch: >> >> clang --analyze

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2837907 , @uabelho wrote: > Hi, > > Another failed assertion that started appearing with this patch: > > clang --analyze bbi-57589.c > > which results in: > > clang: ../lib/Support/APInt.cpp:284: int

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

uabelho added a comment. Hi, Another failed assertion that started appearing with this patch: clang --analyze bbi-57589.c which results in: clang: ../lib/Support/APInt.cpp:284: int llvm::APInt::compareSigned(const llvm::APInt &) const: Assertion `BitWidth == RHS.BitWidth && "Bit widths

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2829806 , @uabelho wrote: > Hi, > > I'm seeing a failed assertion with this patch. > Reproduce with > > clang --analyze bbi-57338.c > > Result: > > clang:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added a comment. In D103314#2829806 , @uabelho wrote: > Hi, > > I'm seeing a failed assertion with this patch. > Reproduce with > > clang --analyze bbi-57338.c > > Result: > > clang:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

uabelho added a comment. Hi, I'm seeing a failed assertion with this patch. Reproduce with clang --analyze bbi-57338.c Result: clang: /repo/uabelho/master-github/llvm/include/llvm/ADT/APSInt.h:148: bool llvm::APSInt::operator<(const llvm::APSInt &) const: Assertion `IsUnsigned ==

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. > This patch is the first step of a sequence of patches, and not intended to be > commited as a standalone change. Although I planned to commit this in a lock-step when subsequent patches are also accepted, it makes sense to commit now since it's an obvious improvement

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8ddbb442b6e8: [Analyzer][solver] Simplify existing eq classes and constraints when a new… (authored by martong). Changed prior to commit:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added a comment. In D103314#2810795 , @martong wrote: > I have the first measurements results in the attached zip file. The file > contains the html file generated by csa-testbench. It's name contains `CTU` > but actually it was a regular

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked an inline comment as done. martong added a comment. I have the first measurements results in the attached zip file. The file contains the html file generated by csa-testbench. It's name contains `CTU` but actually it was a regular non-CTU analysis. The most interesting is

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1789 + if (DisequalToOther.contains(*this)) +return nullptr; if (!DisequalToOther.isEmpty()) { vsavchenko wrote:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 350966. martong marked an inline comment as done. martong added a comment. - Remove isEqualTo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm.org/D103314 Files:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. > OK, we definitely need to know about performance. Couldn't agree more. I am in the middle of a performance measurement that I do with csa-testbench (on memchached,tmux,curl,twin,redis,vim,openssl,sqlite,ffmpeg,postgresql,tinyxml2,libwebm,xerces,bitcoin,protobuf).

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 2 inline comments as done. martong added inline comments. Comment at: clang/test/Analysis/find-binop-constraints.cpp:155-158 +// Here, e1 is still bound to (reg_$0) - (reg_$1) but we +// should be able to simplify it to (reg_$0) - 2 and thus realize +

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 2 inline comments as done. martong added a comment. > I have one thought here. If the lack of simplification indeed caused the > crash, we are in trouble with this patch. IMO simplification in just one > place should make it better, but shouldn't produce infeasible states for us.

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added a comment. OK, we definitely need to know about performance. Plus, I'm still curious about the crash. I didn't get how simplification helped/caused that crash. I have one thought here. If the lack of simplification indeed caused the crash, we are in trouble with this patch.

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2798968 , @martong wrote: > In D103314#2790868 , @vsavchenko > wrote: > >> Awesome! >> I know, I said that we are ready to land, but I think I was too excited >> about this

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 350898. martong added a comment. - Simplify the symbol before eq tracking as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm.org/D103314 Files:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2790868 , @vsavchenko wrote: > Awesome! > I know, I said that we are ready to land, but I think I was too excited about > this change. We probably should have some data on how it performs on > real-life codebases.

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 349352. martong added a comment. I am terribly sorry, but I uploaded an unfinished Diff previously, please disregard that. So these are the changes: - Add isEqualTo and simplify members to EquivalenceClass Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 349261. martong marked 3 inline comments as done. martong added a comment. - Add isEqualTo and simplify members to EquivalenceClass Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 3 inline comments as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1559 + // absolute minimum. + LLVM_NODISCARD ProgramStateRef simplifyEquivalenceClass( + ProgramStateRef State, EquivalenceClass

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added a comment. Awesome! I know, I said that we are ready to land, but I think I was too excited about this change. We probably should have some data on how it performs on real-life codebases. Comment at:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 2 inline comments as done. martong added a comment. I was wondering if there is a direct way to check the equivalence classes? I am thinking about to add a `clang_annalyzer_dump_equivalence_classes` function to the ExprInspection checker. Repository: rG LLVM Github Monorepo

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 348945. martong marked 2 inline comments as done. martong added a comment. - Simplify equivalence classes when iterate over ClassMap, simplify constraints by iterating over the ConstraintsMap Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 5 inline comments as done. martong added a comment. In D103314#2789754 , @vsavchenko wrote: > I had another thought, `merge` is usually called in situations when we found > out that two symbols should be marked equal (and checked that

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added a comment. I had another thought, `merge` is usually called in situations when we found out that two symbols should be marked equal (and checked that it's possible to begin with), which is not true in your case. If we update my case from before, we can get: `a + b == c` and `a

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1576-1577 + // Add the newly simplified symbol to the equivalence class. + State = + Class.merge(this->getBasicVals(), F, State, +

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added a comment. That's awesome, just a few stylistic tweaks and tests and we are to land! Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1561-1562 + +if (!Constraint.getConcreteValue()) + return State; + I think we

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 348808. martong marked an inline comment as done. martong added a comment. - Merge the simplified symbol to the old class Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1561 + return nullptr; + +ConstraintMap CM = getConstraintMap(State); vsavchenko wrote: > Also I think we can

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1561 + return nullptr; + +ConstraintMap CM = getConstraintMap(State); Also I think we can introduce a simple, but efficient optimization of kicking

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. Thanks Valeriy for the quick review and guidance! I am planning to do the changes and continue next week :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm.org/D103314

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

vsavchenko requested changes to this revision. vsavchenko added a comment. This revision now requires changes to proceed. Hey, great job! This is really something that we need, but it's implemented not really correctly. I tried to cover it in the inline comment. Comment at:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

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