Page MenuHomePhabricator

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

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

Diff Detail

Repository
rL LLVM

Event Timeline

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.Oct 3 2017, 5:19 PM
lib/Sema/SemaChecking.cpp
8570–8573

Use /// for documentation comments.

8573

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.

8575

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

8619–8620

This comment adds nothing to the following code; remove?

8703–8751

Please try to reduce/factor out the duplication here.

8779

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.Oct 4 2017, 12:31 PM
lib/Sema/SemaChecking.cpp
8704

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

rsmith edited edge metadata.Oct 4 2017, 6:03 PM

Thanks for the refactoring :)

lib/Sema/SemaChecking.cpp
8704

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 :)

8777

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.Oct 5 2017, 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
8777

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.Oct 6 2017, 9:59 AM
lib/Sema/SemaChecking.cpp
8777

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.Oct 7 2017, 6:58 AM
lib/Sema/SemaChecking.cpp
8777

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.Oct 11 2017, 5:05 PM
lib/Sema/SemaChecking.cpp
8676–8688

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;
9027

Pass LHSValue and RHSValue in here rather than recomputing them.

9028

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.

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
8676–8688

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

9027

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

9028

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.

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

Thanks, this looks great.

lib/Sema/SemaChecking.cpp
9024

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

9028

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.

9034

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.Oct 12 2017, 12:19 PM
lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.
lib/Sema/SemaChecking.cpp
9034

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.Oct 12 2017, 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.Oct 12 2017, 3:11 PM
lebedev.ri added inline comments.Oct 13 2017, 11:32 AM
cfe/trunk/test/Sema/tautological-constant-compare.c
23 ↗(On Diff #118835)

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.Oct 19 2017, 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!