NoQ created this revision. NoQ added reviewers: pgousseau, zaks.anna, dcoughlin, xazax.hun. NoQ added a subscriber: cfe-commits.
Sorry for being a bit slow, i should have had a look at the review earlier; i noticed some stuff after the recent patch by Pierre Gousseau was committed. 1. There's an off-by-one in the comparison `evalBinOp` in `evalIntegralCast`, the patch attached to this revision fixes it, and tests it via bool-assignment checker. 2. There's also a FIXME test for the same checker, which is a regression after this patch (used to throw a warning, now it doesn't); seems not as bad as the crash, so it can probably be addressed later. 3. I noticed that it's still possible to trigger the crash with the following test, not sure what to do with it: ``` void f1(long foo) { unsigned index = -1; if (index < foo - 1) index = foo; if (index + 1 == 0) {} } ``` So my best idea is to push this quick fix for off-by-one and then think where to go further. Probably we need to teach `RangeConstraintManager` to work with `SymbolCast`'s correctly (eg. understand how their range is related to a parent symbol's range). Eventually, once it gets better, we'd probably want to change the following in `evalIntegralCast`: ``` std::tie(IsNotTruncated, IsTruncated) = state->assume(CompVal); - if (!IsNotTruncated && IsTruncated) { + if (IsTruncated) { ``` but right now it breaks tests (not only bool-assignment, but also other checkers that start failing can probably be shown to loose warnings after this patch, but i didn't have a look at this yet, so not sure, just hand-waving). Anyway, generally, overally, i think r257464 is a step in the very right direction if we want to work with casts better, and i was actually very happy to see it went that way :) http://reviews.llvm.org/D16178 Files: lib/StaticAnalyzer/Core/SValBuilder.cpp test/Analysis/bool-assignment.c Index: test/Analysis/bool-assignment.c =================================================================== --- test/Analysis/bool-assignment.c +++ test/Analysis/bool-assignment.c @@ -42,6 +42,15 @@ BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}} return; } + if (y > 200 && y < 250) { + // FIXME: Currently we are loosing this warning due to a SymbolCast in RHS. + BOOL x = y; // no-warning + return; + } + if (y >= 127 && y < 150) { + BOOL x = y; // expected-warning{{Assignment of a non-Boolean value}} + return; + } if (y > 1) { BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}} return; Index: lib/StaticAnalyzer/Core/SValBuilder.cpp =================================================================== --- lib/StaticAnalyzer/Core/SValBuilder.cpp +++ lib/StaticAnalyzer/Core/SValBuilder.cpp @@ -451,7 +451,7 @@ NonLoc FromVal = val.castAs<NonLoc>(); QualType CmpTy = getConditionType(); NonLoc CompVal = - evalBinOpNN(state, BO_LT, FromVal, ToTypeMaxVal, CmpTy).castAs<NonLoc>(); + evalBinOpNN(state, BO_LE, FromVal, ToTypeMaxVal, CmpTy).castAs<NonLoc>(); ProgramStateRef IsNotTruncated, IsTruncated; std::tie(IsNotTruncated, IsTruncated) = state->assume(CompVal); if (!IsNotTruncated && IsTruncated) {
Index: test/Analysis/bool-assignment.c =================================================================== --- test/Analysis/bool-assignment.c +++ test/Analysis/bool-assignment.c @@ -42,6 +42,15 @@ BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}} return; } + if (y > 200 && y < 250) { + // FIXME: Currently we are loosing this warning due to a SymbolCast in RHS. + BOOL x = y; // no-warning + return; + } + if (y >= 127 && y < 150) { + BOOL x = y; // expected-warning{{Assignment of a non-Boolean value}} + return; + } if (y > 1) { BOOL x = y; // expected-warning {{Assignment of a non-Boolean value}} return; Index: lib/StaticAnalyzer/Core/SValBuilder.cpp =================================================================== --- lib/StaticAnalyzer/Core/SValBuilder.cpp +++ lib/StaticAnalyzer/Core/SValBuilder.cpp @@ -451,7 +451,7 @@ NonLoc FromVal = val.castAs<NonLoc>(); QualType CmpTy = getConditionType(); NonLoc CompVal = - evalBinOpNN(state, BO_LT, FromVal, ToTypeMaxVal, CmpTy).castAs<NonLoc>(); + evalBinOpNN(state, BO_LE, FromVal, ToTypeMaxVal, CmpTy).castAs<NonLoc>(); ProgramStateRef IsNotTruncated, IsTruncated; std::tie(IsNotTruncated, IsTruncated) = state->assume(CompVal); if (!IsNotTruncated && IsTruncated) {
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits