Fix 11203: false positive: knownConditionTrueFalse 'always false' when comparing integer with floating-point#4350
Conversation
…n comparing integer with floating-point
| if(ANALYZE_UNDEFINED) | ||
| # TODO: enable signed-integer-overflow | ||
| add_compile_options(-fsanitize=undefined -fsanitize=nullability -fno-sanitize=signed-integer-overflow) | ||
| add_compile_options(-fsanitize=undefined -fsanitize=nullability -fno-sanitize=signed-integer-overflow -fno-sanitize=float-cast-overflow) |
There was a problem hiding this comment.
Please provide the UBSAN warning which is being reported here.
There was a problem hiding this comment.
is float-cast-overflow generated from the provided test case ? spontanously that sounds weird to me.
There was a problem hiding this comment.
From https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html:
-fsanitize=float-cast-overflow: Conversion to, from, or between floating-point types which would overflow the destination. Because the range of representable values for all floating-point types supported by Clang is [-inf, +inf], the only cases detected are conversions from floating point to integer types.
There was a problem hiding this comment.
yeah. So we get a overflow when cppcheck analyze this code:
check("void f() {\n"
" int i = 10;\n"
" if(i > 9.9){}\n"
" float f = 9.9f;\n"
" if(f < 10) {}\n"
"}\n");
And therefore you have to disable that sanitizer? spontanously I am confused. Shouldn't our analysis be tweaked so the cast overflow is avoided?
There was a problem hiding this comment.
I do wonder why the data flow thinks that some value is that large/small.. maybe it's some "impossible" value that needs to be casted better?
There was a problem hiding this comment.
So we get a overflow when cppcheck analyze this code
We dont get an overflow for that case. We get incorrect rounding. We get the overflow for this case:
void foo(double recoveredX) {
for (double x = 1e-18; x < 1e40; x *= 1.9) {
double relativeError = (x - recoveredX) / x;
}
}
|
If this actually overflows should the overflow be more "controlled" so the result does not flip? Is this a case like https://trac.cppcheck.net/ticket/9994 where the value actually should be 128-bit? Or even being reported as a finding? |
| const double floatValue1 = value1.isFloatValue() ? value1.floatValue : value1.intvalue; | ||
| const double floatValue2 = value2.isFloatValue() ? value2.floatValue : value2.intvalue; | ||
| const MathLib::bigint intValue1 = | ||
| value1.isFloatValue() ? std::llround(value1.floatValue) : value1.intvalue; |
There was a problem hiding this comment.
Maybe mention that UBSAN warning in comment that it is intentional.
There was a problem hiding this comment.
I doubt that the ubsan warning is intentional. if value1.floatValue is too large and we try to cast it to bigint.. what is the result? we don't want a random value for it right?
There was a problem hiding this comment.
if value1.floatValue is too large and we try to cast it to bigint.. what is the result? we don't want a random value for it right?
We dont actually use the value since one of the values is already float. I think we could make this lazy.
|
I am no longer disabling UBSAN for this case. |
|
Anymow feedback on this? |
3 participants
No description provided.