Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[clang][sema] Unary not boolean to int conversion
Needs ReviewPublic

Authored by AshleyRoll on Jun 3 2022, 5:41 AM.



I have modifyed GetExprRange() to capture the promotion
of boolean values to ints when applying unary not to ensure
-Wsign-compare identifies unsigned/signed comparisons as
identified in:

Diff Detail

Event Timeline

AshleyRoll created this revision.Jun 3 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 5:41 AM
AshleyRoll requested review of this revision.Jun 3 2022, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 5:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Rebased to remove clangd test failure

erichkeane added inline comments.Jun 10 2022, 6:27 AM

Richard mentions UO_PreInc, UO_PreDec, UO_Minus, and UO_Not. What about the other 3?


Can you trace what MaxWidth ends up being every time? Are we sure this will always just be int-width and not long long or something?

aaron.ballman added inline comments.Jun 10 2022, 7:52 AM

UO_Plus also.

Thank you for the patch! This is certainly an improvement but I think there are still some cases where we compute the wrong range for ~ with this patch applied.


Taking those in turn:

  • UO_PreInc and UO_PreDec can only produce values in the range of the lvalue operand; we'll never have a more precise range for an lvalue than the range from its type unless it's a bit-field, in which case we *do* want to take the bit-field's width into account. So I think the current code is actually correct for those two -- looking at the range of the operand gives the right result even for bit-fields, whereas looking at the type of the operand would give a less precise result in that case.
  • UO_Minus is not giving the right answer. I think we should model -expr in exactly the same way we model 0 - expr -- see the code for BO_Sub above.
  • UO_Plus seems OK with the current code: the range of values of +expr is the same as the range of values of expr, even if a promotion happens.

In any case, I don't think we should have a default here that returns the width of the operand. Instead, I think the default should return the range for the type of E.


I don't think falling through and picking up the expression range of the operand is ever correct for ~. For example:

bool f(char c) {
  return ~c > 0x10000;

... produces a bogus tautological comparison warning. (This is not tautological: the ~ operator will map negative chars to ints in the range [2^31 - 2^7, 2^31) and non-negative chars to ints in the range [-2^31, -2^31+2^7), so this is equivalent to c < 0.)

Also, using MaxWidth here is conservatively correct but isn't precise; for example, we should still warn on:

bool f(bool b) {
  return ~b > 0x1'0000'0000LL;

... because ~b always fits in an int, but I think MaxWidth here will be 64 so we won't warn. (It'd be nice to warn even on ~b > 0 but I don't think we can do that without some major changes to how IntRange is represented.)

The true result range of ~n will be something like [-2^N, -2^N + 2^M) u [2^N - 2^M, 2^N) if the input is signed. IntRange can't represent a range with a hole in the middle like that, but it can represent [-2^N, 2^N), which seems like the least wrong answer that we can give -- that is notably also the entire range of the type of the ~ expression. We get a contiguous range [-2^N, -2^N + 2^M) if the input is known non-negative, but IntRange still can't represent that, and so that doesn't help us make our result any more precise than using the entire range of the type of E, unfortunately. So I think UO_Not should never look at its subexpression -- the best result we can give is IntRange::forValueOfType(C, GetExprType(E)), and that's what we should use.

@AshleyRoll are you still working on this patch?

I've not been able to get the time for a while, I hope to be able to spend some more time on it, but I'd probably do that through a GitHub PR now. If someone else wants to take it on, I'd be fine with that.

If someone else wants to take it on, I'd be fine with that.

You can resume working with this patch, if you want. It's just a reminder. )