C++: Fix false positive in cpp/comparison-with-wider-type#5859
C++: Fix false positive in cpp/comparison-with-wider-type#5859MathiasVP merged 7 commits intogithub:mainfrom
cpp/comparison-with-wider-type#5859Conversation
…arge-expression side of the comparison.
| @@ -0,0 +1,3 @@ | |||
| void test_issue_5850(unsigned char small, unsigned int large1) { | |||
| for(; small < static_cast<unsigned char>(large1 - 1); small++) { } // GOOD | |||
There was a problem hiding this comment.
I've convinced myself I'm happy with this. There could still be a correctness issue if large1 - 1 overflows an unsigned char causing the loop to end at an unexpected point, but (1) the programmer has explicitly made the cast, suggesting they are happy it fits and (2) regardless the loop will terminate (which it might not without the cast), and I remember that being the main concern here.
There was a problem hiding this comment.
Good point. Would you be happier with the change if we only did large.getExplicitlyConverted() instead of large.getFullyConverted()? That would only include the conversions the programmer actually wrote.
There was a problem hiding this comment.
I think I'm happy with getFullyConverted ... LGTM diff to be sure: https://lgtm.com/query/3711128432146950375/
There was a problem hiding this comment.
^ almost all results are the same for all three versions; I found just three that were excluded by the conversion bit (either getFullyConverted or getExplicitlyConverted) and they all resembled the motivating case for this change.
So from a practical point of view I'm happy with this change as-is, but it sounds like @jbj has spotted a theoretical concern worthy of discussion.
There was a problem hiding this comment.
Yeah, I'm trying to see if I can produce a testcase that has the problem. Let's hold off with the merge until we have a conclusion on that.
There was a problem hiding this comment.
It makes sense that fully and explicitly converted are the same on large. The query is about a small type compared to a large type, so the implicit conversions will (always?) happen on the small side of the comparison.
There was a problem hiding this comment.
Oh, yes. That's a good point!
There was a problem hiding this comment.
Unlikely to be found like this in the wild, but maybe:
void test(unsigned char small, unsigned int large1) {
for(; small < (unsigned int)(unsigned char)(large1 - 1); small++) { } // GOOD
}
| large = rel.getGreaterOperand() and | ||
| rel = l.getCondition().getAChild*() and | ||
| upperBound(large).log2() > getComparisonSize(small) * 8 and | ||
| upperBound(large.getFullyConverted()).log2() > getComparisonSize(small) * 8 and |
There was a problem hiding this comment.
I worry that this change could lead to false positives when the conversion is the other way around, widening rather than narrowing. If the range analysis can't determine that a signed char x is non-negative, then (unsigned int)x will have an upper bound of INT_MAX. I might be wrong, so please add a test to confirm.
To be on the safe side, you could add the new line of code rather than replacing what was there.
There was a problem hiding this comment.
or
forall(Expr conv | conv = large.getConversion*() | upperBound(conv).log2() > getComparisonSize(small) * 8) and
There was a problem hiding this comment.
Yes, I have no principled argument for why the endpoints of the conversion should be special. But with both forall and getConversion* we'd have to check for performance regressions more carefully.
There was a problem hiding this comment.
I think I'm in favor of @geoffw0's more general solution as well. I'll add a problematic testcase first (the naive one I made didn't reveal any false positives, but I think I can tweak it so that it does the wrong thing), and see how the forall performs.
…xpression _and_ all of the conversions.
3 participants
Fixes #5850.
Prior to this PR, we ignored the conversions on the large expression when computing range analysis.