Page MenuHomePhabricator

[Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent
Needs ReviewPublic

Authored by lebedev.ri on Oct 31 2017, 10:44 AM.

Details

Summary

As brought up in D39149, and post-review mails for some other commits,
the current -Wtautological-constant-compare is dumb, much like
unreachable code diagnostic.

The common complaint is that it diagnoses the comparisons between an int and
long when compiling for a 32-bit target as tautological, but not when
compiling for 64-bit targets. The underlying problem is obvious: data model.
In most cases, 64-bit target is LP64 (int is 32-bit, long and pointer are
64-bit), and the 32-bit target is ILP32 (int, long, and pointer are 32-bit).

I.e. the common pattern is: (pseudocode)

#include <limits>
#include <cstdint>
int main() {
  using T1 = long;
  using T2 = int;

  T1 r;
  if (r < std::numeric_limits<T2>::min()) {}
  if (r > std::numeric_limits<T2>::max()) {}
}

The fix is simple: if the types of the values being compared are different, but
are of the same size, then we issue -Wmaybe-tautological-constant-compare
instead of -Wtautological-constant-compare.
That new diagnostic is *not* enabled by default/-Wall/-Wextra.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Oct 31 2017, 10:44 AM

I'm sure i have missed some corner case :)

...

That would also suppress warnings for cases like 'ch > CHAR_MAX', when ch is a char, but we could detect and re-enable the warning for such cases by, say, always warning if the constant side is an integer literal.

This sounds like a very promising direction. (That is: if the types of the values being compared are different, but are of the same size, then suppress the warning or move it to a different warning group that's not part of -Wtautological-compare.) That would also suppress warnings for cases like 'ch > CHAR_MAX', when ch is a char, but we could detect and re-enable the warning for such cases by, say, always warning if the constant side is an integer literal.

When you say "always warning if the constant side is an integer literal", do you mean if it's a straight-up integer literal, or if it's an expression which evaluates to an integer literal at compile time? For example, would there be a difference if you were comparing to numeric_limits<int>::max() vs. INT_MAX?

Please do note that if the integer literal / integral constant expression is originating from *any* macro, this entire tautologicalness diagnostic is silenced.
I very much don't like this, because it would warn on if(char > 255) but would not warn on assert(char <= 255);.

rjmccall edited edge metadata.Oct 31 2017, 11:08 AM

The actual choice of integer type is not stable across targets any more than the size is. In practice, people often don't use int and long, they use standard typedefs like size_t and uint64_t, but the actual type chosen for size_t is arbitrary when there are multiple types at that bit-width. So I'm concerned that you're suppressing more than you think here and still not going to satisfy people.

Also, "data model" is not a term in use. Use "target" or "target platform".

The actual choice of integer type is not stable across targets any more than the size is. In practice, people often don't use int and long, they use standard typedefs like size_t and uint64_t, but the actual type chosen for size_t is arbitrary when there are multiple types at that bit-width.

The internal canonical types are compared, so all this typedef sugar will be silently ignored.

So I'm concerned that you're suppressing more than you think here and still not going to satisfy people.

Possibly...
But it will at least make it easier to deal with the most common pattern "that obviously should not warn".
I'd love to hear better ideas

Also, "data model" is not a term in use. Use "target" or "target platform".

Changed to for the current target platform, ...

This solves the issue in D39149, at least.

The internal canonical types are compared, so all this typedef sugar will be silently ignored.

Yes, and that's precisely the problem. On many 32-bit platforms, both "size_t" and "uint32_t" are typedefs for "unsigned int"; on some 32-bit platforms, "size_t" is an "unsigned long". So whether this patch suppresses the warning for a comparison between size_t and uint32_t depends on the target ABI.

phosek added a subscriber: phosek.Nov 3 2017, 3:41 PM
lebedev.ri updated this revision to Diff 121743.Nov 6 2017, 8:43 AM
lebedev.ri edited the summary of this revision. (Show Details)

The actual choice of integer type is not stable across targets any more than the size is. In practice, people often don't use int and long, they use standard typedefs like size_t and uint64_t, but the actual type chosen for size_t is arbitrary when there are multiple types at that bit-width.

The internal canonical types are compared, so all this typedef sugar will be silently ignored.

Yes, and that's precisely the problem. On many 32-bit platforms, both "size_t" and "uint32_t" are typedefs for "unsigned int"; on some 32-bit platforms, "size_t" is an "unsigned long". So whether this patch suppresses the warning for a comparison between size_t and uint32_t depends on the target ABI.

Okay, changed the code to compare not the internal canonical types, but just the types.
Added tests that show that it does detect&complain about comparing two different types, that point to the same type.

Also. should it really not be in -Wextra?

So, that change makes this very interesting, because I think the right way of looking at it is as the first in a larger family of warnings that attempt to treat typedefs as if they were a much stronger type-system feature, i.e. that warn about all sorts of conversions between different typedef types. That should be good enough to serve as a basic rule for a stronger portability warning, as well as generally pointing out all sorts of potential logical errors like passing a bit_offset_t off as a byte_offset_t.

Such a warning really needs more exceptions than a simple exact-type-spelling rule would give you. There are several language features that add type sugar which should really be ignored for the purposes of the warning, such as typeof and decltype; and conversely, there are several features that remove (or just never add) type sugar that also shouldn't cause problems, like literals or C++ templates.

I think that feature could be really useful as a major new diagnostic, but I do want to warn you that it's probably a pretty large project, somewhat on the scale of implementing -Wconversion in the first place. Also, yeah, my first thought is that it's probably outside of a reasonable rubric for even -Wextra, especially while it's being actively developed.

So, that change makes this very interesting, because I think the right way of looking at it is as the first in a larger family of warnings that attempt to treat typedefs as if they were a much stronger type-system feature, i.e. that warn about all sorts of conversions between different typedef types. That should be good enough to serve as a basic rule for a stronger portability warning, as well as generally pointing out all sorts of potential logical errors like passing a bit_offset_t off as a byte_offset_t.

Such a warning really needs more exceptions than a simple exact-type-spelling rule would give you. There are several language features that add type sugar which should really be ignored for the purposes of the warning, such as typeof and decltype; and conversely, there are several features that remove (or just never add) type sugar that also shouldn't cause problems, like literals or C++ templates.

I think that feature could be really useful as a major new diagnostic

That is all very cool and shiny, but could we please go back to the real world, please? :)

but I do want to warn you that it's probably a pretty large project, somewhat on the scale of implementing -Wconversion in the first place.

Exactly. My expirience shows that unless i'm actually interested, i will either fail, or it will work poorly.
And i can tell you that i'm not quite interested in implementing what you seem to suggest to implement.

What i would like to do, is to finish *this* differential, that would make D38101 less noisy for some questionable edge-cases,
without

a pretty large project, somewhat on the scale of implementing -Wconversion in the first place.

Can that happen? :)

Also, yeah, my first thought is that it's probably outside of a reasonable rubric for even -Wextra, especially while it's being actively developed.

For that other diagnostic you suggest - sure.


I think the general direction is correct, but there are unhandled cases yet, e.g , so I'm somewhat lost here, and would love to hear some actual feedback for the suggested code.

I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives. I sketched the basics of a design that I think I could accept; if you don't want to implement it, that's your right, but that doesn't make me more likely to accept what you're willing to implement.

I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives. I sketched the basics of a design that I think I could accept; if you don't want to implement it, that's your right, but that doesn't make me more likely to accept what you're willing to implement.

Just to reiterate that we are talking about the same thing here:

  • D38101 is already merged. -Wtautological-constant-compare is here.
  • There are cases when it warns for some target platform, but not the other, as complained in D39149, and post-review mails for D38101
  • So far it seems all the cases reduce to
#include <limits>
#include <cstdint>
int main() {
  using T1 = long;
  using T2 = int;

  T1 r;
  if (r < std::numeric_limits<T2>::min()) {}
  if (r > std::numeric_limits<T2>::max()) {}
}
  • *This* differential (D39462) would find such cases, and issue them under different diagnostic, thus reducing the "false-positive" (it is an open question whether they are actual false-positives or not) rate of -Wtautological-constant-compare.

Are you suggesting me to drop this, and implement some other huge new diagnostic that may catch such cases before -Wtautological-constant-compare, thus preventing -Wtautological-constant-compare from triggering on that completely?

I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives. I sketched the basics of a design that I think I could accept; if you don't want to implement it, that's your right, but that doesn't make me more likely to accept what you're willing to implement.

Just to reiterate that we are talking about the same thing here:

  • D38101 is already merged. -Wtautological-constant-compare is here.
  • There are cases when it warns for some target platform, but not the other, as complained in D39149, and post-review mails for D38101
  • So far it seems all the cases reduce to ` #include <limits> #include <cstdint> int main() { using T1 = long; using T2 = int;

    T1 r; if (r < std::numeric_limits<T2>::min()) {} if (r > std::numeric_limits<T2>::max()) {} } `
  • *This* differential (D39462) would find such cases, and issue them under different diagnostic, thus reducing the "false-positive" (it is an open question whether they are actual false-positives or not) rate of -Wtautological-constant-compare.

I think you might have this backwards. We think of the check for the diagnostic as being the test, so a false positive is when we warn when we shouldn't. What you've identified here is a false *negative*, i.e. a case where you believe it should warn (because it would warn on a different target) that it currently does not.

Are you suggesting me to drop this, and implement some other huge new diagnostic that may catch such cases before -Wtautological-constant-compare, thus preventing -Wtautological-constant-compare from triggering on that completely?

"This warning triggers on some targets and not on others" is a far more widespread problem than just -Wtautological-constant-compare. I don't think your patch reasonably solves that problem, even for just this diagnostic. I think a "strong typedef" analysis would address it, but that's going to require a significant amount of work, even if you literally only apply it to this warning, because you're going to have to implement a bunch of more careful logic about inferring when to propagate typedefs through e.g. templates, as well as when to consider a template to have "propagated" through arithmetic promotion in the same way we propagate integer ranges.

smeenai added a comment.EditedNov 12 2017, 12:05 PM

I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives. I sketched the basics of a design that I think I could accept; if you don't want to implement it, that's your right, but that doesn't make me more likely to accept what you're willing to implement.

Just to reiterate that we are talking about the same thing here:

  • D38101 is already merged. -Wtautological-constant-compare is here.
  • There are cases when it warns for some target platform, but not the other, as complained in D39149, and post-review mails for D38101
  • So far it seems all the cases reduce to ` #include <limits> #include <cstdint> int main() { using T1 = long; using T2 = int;

    T1 r; if (r < std::numeric_limits<T2>::min()) {} if (r > std::numeric_limits<T2>::max()) {} } `
  • *This* differential (D39462) would find such cases, and issue them under different diagnostic, thus reducing the "false-positive" (it is an open question whether they are actual false-positives or not) rate of -Wtautological-constant-compare.

I think you might have this backwards. We think of the check for the diagnostic as being the test, so a false positive is when we warn when we shouldn't. What you've identified here is a false *negative*, i.e. a case where you believe it should warn (because it would warn on a different target) that it currently does not.

I guess you can think of it both ways, but the concern for D39149 was definitely that of a false positive. More precisely, a long variable was being compared against the limits for int. On a 64-bit platform, you would have no warning. On a 32-bit platform, you would have a warning, by virtue of int and long being the same size. I consider the warning on the 32-bit platform to be a false positive, since the comparison is serving a purpose, but the compiler is still flagging it as tautological. In general, I would want -Wtautological-constant-compare to only fire in cases where the comparison is guaranteed to be tautological, and would consider anything else to be a false positive.

I see. The problem now, though, is that things involving, say, a size_t and a numeric_limits<size_t> will never warn.

I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives. I sketched the basics of a design that I think I could accept; if you don't want to implement it, that's your right, but that doesn't make me more likely to accept what you're willing to implement.

Just to reiterate that we are talking about the same thing here:

  • D38101 is already merged. -Wtautological-constant-compare is here.
  • There are cases when it warns for some target platform, but not the other, as complained in D39149, and post-review mails for D38101
  • So far it seems all the cases reduce to ` #include <limits> #include <cstdint> int main() { using T1 = long; using T2 = int;

    T1 r; if (r < std::numeric_limits<T2>::min()) {} if (r > std::numeric_limits<T2>::max()) {} } `
  • *This* differential (D39462) would find such cases, and issue them under different diagnostic, thus reducing the "false-positive" (it is an open question whether they are actual false-positives or not) rate of -Wtautological-constant-compare.

I think you might have this backwards.

The views here are clearly polarized.

We think of the check for the diagnostic as being the test, so a false positive is when we warn when we shouldn't.

Yes.

What you've identified here is a false *negative*, i.e. a case where you believe it should warn (because it would warn on a different target) that it currently does not.

I'm sorry, but where are you getting this from? I don't believe these warnings should be elevated to always warn even if it is not tautological for the current target platform. I don't think i have said that?

Are you suggesting me to drop this, and implement some other huge new diagnostic that may catch such cases before -Wtautological-constant-compare, thus preventing -Wtautological-constant-compare from triggering on that completely?

"This warning triggers on some targets and not on others" is a far more widespread problem than just -Wtautological-constant-compare. I don't think your patch reasonably solves that problem, even for just this diagnostic. I think a "strong typedef" analysis would address it, but that's going to require a significant amount of work, even if you literally only apply it to this warning, because you're going to have to implement a bunch of more careful logic about inferring when to propagate typedefs through e.g. templates, as well as when to consider a template to have "propagated" through arithmetic promotion in the same way we propagate integer ranges.

First and foremost, i do admit that i don't have the full picture in mind.
If other reviewers agree with Your view, i will abandon this differential. Hopefully someone with clearer understanding will come up with a better solution.

I see. The problem now, though, is that things involving, say, a size_t and a numeric_limits<size_t> will never warn.

The same type (size_t) will be on the both sides, so i think it should still warn. (I do see that the test disagrees with me.)

That's the template type-propagation problem: the template was invoked with a size_t, but that's erased down to the canonical type by the template machinery. That can't be fixed within the template, but at use sites, we could theoretically recognize that e.g. the result of the callee was originally spelled T and then look for how T was determined; here it comes from an explicit template argument, whose spelling can just be propagated into the sugared type of the call expression. But that's the sort of analysis I think you'd need to do a good job with in order to make this adjustment successful.

I'd like to understand/resurrect this change, so I'll try to summarize. Please correct this as appropriate:

  1. We got here because libc++ has code that triggers a warning for some targets (those whose int and long have the same size).
  2. This change would "move" the existing logic for -Wtautological-constant-compare to -Wmaybe-tautological-constant-compare and replace -Wtautological-constant-compare logic with one less likely to report that the code in libc++ is wrong.
  3. A superior checker could be defined that would thread the needle between these cases: warning only when it should and not when it shouldn't. This would be preferred because it avoids the creation of similar but slightly distinct warnings.

If this summary is really the case, what's the best way to break this stalemate? Could we implement this change for now and improve the warnings later? If the answer is 'no', then let's please restore D39149.

AFAICT Marshall and John have strong feelings on each of these proposed libc++ and clang changes -- it would be valuable if each of you could weigh in.

lebedev.ri added a comment.EditedNov 27 2017, 12:59 PM

I'd like to understand/resurrect this change, so I'll try to summarize. Please correct this as appropriate:

  1. We got here because libc++ has code that triggers a warning for some targets (those whose int and long have the same size).
  2. This change would "move" the existing logic for -Wtautological-constant-compare to -Wmaybe-tautological-constant-compare and replace -Wtautological-constant-compare logic with one less likely to report that the code in libc++ is wrong.
  3. A superior checker could be defined that would thread the needle between these cases: warning only when it should and not when it shouldn't. This would be preferred because it avoids the creation of similar but slightly distinct warnings.

    If this summary is really the case, what's the best way to break this stalemate? Could we implement this change for now and improve the warnings later? If the answer is 'no', then let's please restore D39149.

    AFAICT Marshall and John have strong feelings on each of these proposed libc++ and clang changes -- it would be valuable if each of you could weigh in.

After more thought i agree with @rjmccall, this is incomplete at best.
The current diff will basically kill the -Wtautological-constant-compare, introduce a lot of false-negatives, which is no-go.
On the other hand, i'm not sure that at the moment i'm able to implement what @rjmccall has outlined.
So if someone wants to take this over, let me know.

jakehehrlich removed a subscriber: jakehehrlich.
jakehehrlich added a subscriber: jakehehrlich.

FWIW we've already rolled Clang that contains -Wtautological-constant-compare to our codebase and we had to set -Wno-tautological-constant-compare globally because we were getting bogus warnings in many places, similarly to D41368. If that warning ships as part of Clang 6, it's going to be be a major pain for many users so we really need some solution.

FWIW we've already rolled Clang that contains -Wtautological-constant-compare to our codebase and we had to set -Wno-tautological-constant-compare globally because we were getting bogus warnings in many places, similarly to D41368. If that warning ships as part of Clang 6, it's going to be be a major pain for many users so we really need some solution.

Let's escalate this discussion to llvm-dev, then? If we don't do anything soon, it will ship w/clang 6.