[Sema] Diagnose tautological comparison with type's min/max values
ClosedPublic

Authored by lebedev.ri on Sep 20 2017, 2:38 PM.

Details

Summary

Currently, clang only diagnoses completely out-of-range comparisons (e.g. char and constant 300),
and comparisons of unsigned and 0. But gcc also does diagnose the comparisons with the
std::numeric_limits<>::max() / std::numeric_limits<>::min() so to speak

Finally Fixes https://bugs.llvm.org/show_bug.cgi?id=34147
Continuation of https://reviews.llvm.org/D37565

Diff Detail

Repository
rL LLVM
lebedev.ri created this revision.Sep 20 2017, 2:38 PM

Tried stage2 build, so far only one warning.
But found out that ubsan_value.h's typedef s128 SIntMax; crashes this code because

class LLVM_NODISCARD APSInt : public APInt {
...
  /// \brief Get the correctly-extended \c int64_t value.
  int64_t getExtValue() const {
    assert(getMinSignedBits() <= 64 && "Too many bits for int64_t");
    return isSigned() ? getSExtValue() : getZExtValue();
  }
  • Fixed handling of types > 64 bit (e.g. __int128), testcase added
  • Stage-2 build now fully passes (only one new warning, D38132)
  • Updated ReleaseNotes.rst

Rebased, ping.

Vanilla stage-2 build is now warning-clean, the only previous warning was fixed.

Ping :)
Does this need more, different tests?
Should i rewrite that lengthy if-else-if chain somehow differently?

rsmith added inline comments.Tue, Oct 3, 5:19 PM
lib/Sema/SemaChecking.cpp
8583–8586 ↗(On Diff #116857)

Use /// for documentation comments.

8586 ↗(On Diff #116857)

Is there a reason to separate the checks for 0 from the other checks for min/max values? It looks like the code below would be slightly simpler if the two checks were combined.

8588 ↗(On Diff #116857)

Value can't be null; pass it by reference instead.

8636–8637 ↗(On Diff #116857)

This comment adds nothing to the following code; remove?

8720–8768 ↗(On Diff #116857)

Please try to reduce/factor out the duplication here.

8796 ↗(On Diff #116857)

Comments should start with a capital letter.

lebedev.ri marked 6 inline comments as done.
lebedev.ri edited the summary of this revision. (Show Details)

@rsmith thank you for the review!

Address review notes:

  1. Merge CheckTautologicalComparisonWithZero() and CheckTautologicalComparisonWithMinMax()
  2. Significantly deduplicate the code, hopefully not at the cost of readability.
  3. Slightly cleanup/move around/update relevant tests for these three types of tautological comparisons.
lebedev.ri added inline comments.Wed, Oct 4, 12:31 PM
lib/Sema/SemaChecking.cpp
8697 ↗(On Diff #117710)

If the diagnostic we are about to output is disabled, should we still return true; and suppress potential -Wsign-compare warning?

rsmith added a comment.Wed, Oct 4, 6:03 PM

Thanks for the refactoring :)

lib/Sema/SemaChecking.cpp
8697 ↗(On Diff #117710)

The general principle is to behave as if we produced all diagnostics, and then filtered out the ones that aren't enabled. So if a more specialized (eg tautological comparison) diagnostic is disabled, the more general (eg sign compare) diagnostic should not appear. In short, this is fine :)

8719 ↗(On Diff #117710)

Is this necessary? (Aren't the type limit values always within the range of the type in question?)

Can we avoid evaluating Constant a extra time here? (We already have its value in Value.)

lebedev.ri updated this revision to Diff 117802.Thu, Oct 5, 6:16 AM
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a subscriber: rtrieu.
  • Address review notes
  • Avoid re-evaluating ICE when called from DiagnoseOutOfRangeComparison() which already knows the evaluated value

    Don't even check if this is a Type Limit in DiagnoseOutOfRangeComparison(). Perhaps @rtrieu can shed some light on this.
  • Enhance test coverage a little (add a test with macro, with template)
lib/Sema/SemaChecking.cpp
8719 ↗(On Diff #117710)

Uhm, good question :)
If i remove this, check-clang-sema and check-clang-semacxx still pass.
I agree that it does not make much sense. Initially it was only checking for Value == 0.
Git suggests that initially this branch was added by @rtrieu, maybe can help.

lebedev.ri added inline comments.Fri, Oct 6, 9:59 AM
lib/Sema/SemaChecking.cpp
8719 ↗(On Diff #117710)

The most original version of this code
After some though i think the initial check Value == 0 was simply to quickly bail out
out of DiagnoseOutOfRangeComparison(), and not waste any time for the obvious case
(0), which can't be out-of-range, ever. So i think the right behaviour could be:

  1. Either simply restore the original check:
// 0 values are handled later by CheckTautologicalComparison().
if ((Value == 0) && (!OtherIsBooleanType))
  return;

And add a comment there about it

  1. Or, drop it completely
  2. Or, perhaps refactor CheckTautologicalComparison(), and more or less call it from function AnalyzeComparison(), before calling DiagnoseOutOfRangeComparison(), thus completely avoiding the need to re-evaluate Constant there later on, and simplify the logic in the process.

I personally think the 3. *might* be best.
WDYT?

lebedev.ri added inline comments.Sat, Oct 7, 6:58 AM
lib/Sema/SemaChecking.cpp
8719 ↗(On Diff #117710)

Tried implementing 3..
It won't work, because DiagnoseOutOfRangeComparison() needs the {L,R}HS
after IgnoreParenImpCasts(), and CheckTautologicalComparison() is not ok with that.
It seems that at most, i could re-use the detection of RhsConstant.

So, new options:

  1. Either simply restore the original check, and add a comment there about the logic behind it
  2. Or, drop the check completely
  3. Or, move the CheckTautologicalComparison() call before DiagnoseOutOfRangeComparison() And if DiagnoseOutOfRangeComparison() has already emitted diagnostic, return. Much like what CheckTautologicalComparison() already does.

So i think 3. is still the best option :)
(tried implementing it, appears to work)

Rework AnalyzeComparison() and DiagnoseOutOfRangeComparison() - avoid that fast-return path in DiagnoseOutOfRangeComparison()
AnalyzeComparison() is now a bit smarter, and it

  1. calls CheckTautologicalComparison()
  2. returns if it diagnosed anything
  3. else, calls DiagnoseOutOfRangeComparison()
  4. returns if it diagnosed anything.

check-clang-sema and check-clang-semacxx are good.
Stage-2 build still good.

rsmith added inline comments.Wed, Oct 11, 5:05 PM
lib/Sema/SemaChecking.cpp
8667–8679 ↗(On Diff #118143)

The exhaustive case analysis is a little hard to verify. Perhaps something like this would be clearer:

bool ConstIsLowerBound = (Op == BO_LT || Op == BO_LE) ^ ConstOnRight;
bool ResultWhenConstEqualsOther = (Op == BO_LE || Op == BO_GE);
bool ResultWhenConstNeOther = ConstIsLowerBound ^ ValueType == LimitType::Max;
if (ResultWhenConstEqualsOther != ResultWhenConstNeOther)
  return false;
8940 ↗(On Diff #118143)

It seems suboptimal to evaluate both sides of the comparison and then evaluate the entire comparison again in this case. Presumably the point is to catch comparisons whose operands are not of integral type (eg, floating point), but we could get that benefit and also catch more integral cases by switching from isIntegerConstantExpr to the more appropriate EvaluateAsRValue.

I'm fine with a cleanup to avoid the repeated evaluation here being deferred to a later patch.

8949 ↗(On Diff #118143)

Pass LHSValue and RHSValue in here rather than recomputing them.

test/Sema/tautological-constant-compare.c
1–4 ↗(On Diff #118143)

This test makes assumptions about the valid ranges of certain built-in types, so should specify a target triple. (Eg, -triple x86_64-linux-gnu)

lebedev.ri marked 4 inline comments as done.

@rsmith, thank you for the review!

Rebased, address review notes, simplified all the things even further.

Testing:

  • $ ninja check-clang-sema check-clang-semacxx
  • Stage 2 build (still good, no issues/warnings)
lib/Sema/SemaChecking.cpp
8667–8679 ↗(On Diff #118143)

Oh, that looks better indeed :)
I did consider doing something like that, but my variant ended up looking worse than even the current if()'s

8940 ↗(On Diff #118143)

If we look at this code closely, if hasUnsignedIntegerRepresentation() == false, we always do return AnalyzeImpConvsInComparison(S, E);, regardless of E->isIntegerConstantExpr(S.Context).
So if i move more stuff into if (T->isIntegralType(S.Context)), then E->isIntegerConstantExpr(S.Context); is simply gone.
At least the current tests say so.

8949 ↗(On Diff #118143)

We can even go one step further, and pass the bool IsConstLiteralOnRHS

rsmith accepted this revision.Thu, Oct 12, 12:19 PM

Thanks, this looks great.

lib/Sema/SemaChecking.cpp
8940 ↗(On Diff #118143)

It looks like the old behavior was to suppress the CheckTautologicalComparisonWithZero diagnostics when:

  • the comparison has a constant value, and
  • the types being compared are not integral types, and
  • the types being compared do not have an unsigned integer representation

However, CheckTautologicalComparisonWithZero only emits diagnostics for comparisons with integral types. So I think you're right that the old codepath that evaluated E->isIntegerConstantExpr(S.Context) was pointless.

8924 ↗(On Diff #118793)

I don't think this variable pulls its weight any more, especially given the adjacent comment. Inline its initializer into the if condition, maybe?

8930 ↗(On Diff #118793)

It would probably be more obvious to use || here, since you already returned in the case where both sides are constant.

This revision is now accepted and ready to land.Thu, Oct 12, 12:19 PM
lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.
lib/Sema/SemaChecking.cpp
8930 ↗(On Diff #118793)

And what if neither of them is constant?
I *think* we could reach this point in that case.
At least it is not immediately obvious to me why it can't happen.

Getting really weird problems when trying to use arc patch, maybe re-updating the differential helps

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Thu, Oct 12, 3:11 PM

Reverted due to http://bb9.pgr.jp/#/builders/20/builds/59 that i don't currently know how to deal with.
It is really sad that i failed to encounter it during testing.

This revision is now accepted and ready to land.Thu, Oct 12, 3:11 PM
lebedev.ri added inline comments.Fri, Oct 13, 11:32 AM
cfe/trunk/test/Sema/tautological-constant-compare.c
24

I'm having second thoughts about macro handling.
Right now we completely ignore the comparisons when the constant is anyhow involved with macros.
I.e.

unsigned char c;
if (c > 255) // expected-warning {{comparison 'unsigned char' > 255 is always false}}
  return;  // would be diagnosed correctly
// but
assert (c <= 255); // would be completely ignored.

Perhaps we should be a bit more strict, and should not bailout in such cases?

But i *guess* this case we still should ignore

#define macro(val) (val > 255)
if (macro(c))
  return;

(Even though gcc warns in the last variant too)

Forgot to add, i really noticed/though about it just now, in https://reviews.llvm.org/D38871, because i did not encountered any warnings in that code in stage-2 builds.

materi added a subscriber: materi.Thu, Oct 19, 10:02 AM

Hi!

After this patch I started to see warnings:

e.c:8:23: warning: integer constant not in range of enumerated type 'enum E' [-Wassign-enum]
  enum E {a = 7,} e = 1000;
                      ^
e.c:10:12: warning: comparison 'enum E' > 7 is always false [-Wtautological-constant-compare]
  return e > 7;

Isn't the the "always false" message misleading? It's only "always false" if e was initialized with an in-range value. Maybe the tautology check should be on the enum's underlying type instead?

Hi!

Hi.

After this patch I started to see warnings:

Thank you for the report!

e.c:8:23: warning: integer constant not in range of enumerated type 'enum E' [-Wassign-enum]
  enum E {a = 7,} e = 1000;
                      ^
e.c:10:12: warning: comparison 'enum E' > 7 is always false [-Wtautological-constant-compare]
  return e > 7;

Isn't the the "always false" message misleading? It's only "always false" if e was initialized with an in-range value. Maybe the tautology check should be on the enum's underlying type instead?

I agree, please open a bug, i'll look into this.

Hi!

Hi.

After this patch I started to see warnings:

Thank you for the report!

e.c:8:23: warning: integer constant not in range of enumerated type 'enum E' [-Wassign-enum]
  enum E {a = 7,} e = 1000;
                      ^
e.c:10:12: warning: comparison 'enum E' > 7 is always false [-Wtautological-constant-compare]
  return e > 7;

Isn't the the "always false" message misleading? It's only "always false" if e was initialized with an in-range value. Maybe the tautology check should be on the enum's underlying type instead?

I agree, please open a bug, i'll look into this.

Here it is:
https://bugs.llvm.org/show_bug.cgi?id=35009

Thanks!