[PATCH] D12901: [Static Analyzer] Assertion "System is over constrained" after truncating 64 bits integers to 32 bits. (PR25078)

[pierre gousseau via cfe-commits]

pgousseau added a comment.

In http://reviews.llvm.org/D12901#320680, @zaks.anna wrote:

> > This patch also fixes a bug in 'RangeSet::pin' causing single value ranges to not be considered conventionally ordered.
>
>
> Can that fix be submitted as a separate patch? Is there a test for it?


Yes I will look at creating a separate review for it.
Tests at lines 81, 111, 131 fail without the fix to RangeSet::pin.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:351
@@ -351,1 +350,3 @@
       }
+      case CK_IntegralCast: {
+        // Delegate to SValBuilder to process.
----------------
zaks.anna wrote:
> SValBuilder::evalCast and SimpleSValBuilder::evalCastFromNonLoc perform a lot of special casing. I am not sure we are not loosing anything if we bypass them. For example, there is special handling of Booleans. We might want to add this smarter handling of the integral conversions inside SimpleSValBuilder::evalCastFromNonLoc, where you see the comment starting with "If the types are the same or both are integers, ignore the cast."
Yes I initially looked at making the change in 'evalCastFromNonLoc' but the problem is that the change requires access to the ProgramState, so to avoid changing the interface of evalCast (used in around 50 places) I made the change here.
Looking at evalCast it does not seem to add any special handling to casts of type 'CK_IntegralCast' before calling 'evalCastFromNonLoc'?
Booleans casts should be associated with another type of cast than 'CK_IntegralCast' and the new 'evalIntegralCast' will call 'evalCast' if it does not detect a truncation so it should be ok, what do you think?


http://reviews.llvm.org/D12901