[PATCH] D35109: [Analyzer] SValBuilder Comparison Rearrangement

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think you might also need to convert `APSInt`s to an appropriate type, as done above. Type of right-hand-side `APSInt`s do not necessarily coincide with the type of the left-hand-side symbol or of the whole expression. `APSInt` operations crash when signedness doesn't match (and in a few other cases).

In https://reviews.llvm.org/D35109#802123, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D35109#801921, @NoQ wrote:
>
> > Because integer promotion rules are tricky, could we, for now, avoid dealing with the situation when left-hand side and right-hand side and the result (all three) are not all of the same type? Or maybe we'd like to support substraction of unsigned values into a signed value of the same size, but still avoid the rest of the cases. Because it'd take an overwhelming amount of testing to ensure that we get all the promotion cases correctly.
>
>
> I think the best place to add and test integer promotion rules is the type system.


Currently we have broken symbolic casts, which means that we significantly ignore the type system, because symbolic expressions involving casts are much harder to simplify or deal with, while overflow cases are too often implicitly contracted out to make the analyzer good at understanding them. Which leads to everything having different type.



================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:397-403
           // We're looking for a type big enough to compare the two values.
           // FIXME: This is not correct. char + short will result in a promotion
           // to int. Unfortunately we have lost types by this point.
           APSIntType CompareType = std::max(APSIntType(LHSValue),
                                             APSIntType(RHSValue));
           CompareType.apply(LHSValue);
           CompareType.apply(RHSValue);
----------------
An example of how `APSInt` types are handled.


================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:521-522
               APSIntType IntType = BasicVals.getAPSIntType(resultTy);
               const llvm::APSInt &first = IntType.convert(symIntExpr->getRHS());
               const llvm::APSInt &second = IntType.convert(*RHSValue);
 
----------------
An example of how `APSInt` types are handled.


https://reviews.llvm.org/D35109