From: Hans Wennborg Date: Fri, 8 Dec 2017 05:19:12 +0000 (+0000) Subject: Revert r320124 "Fold together the in-range and out-of-range portions of -Wtautologica... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ce59979560afc7f4cb497bf4b93146e56e607d65;p=clang Revert r320124 "Fold together the in-range and out-of-range portions of -Wtautological-compare." This broke Chromium: ../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant 64 with expression of type 'unsigned int' is always true [-Werror,-Wtautological-constant-out-of-range-compare] DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize); ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The 'unsigned int' is really a 6-bit bitfield, which is why it's always less than 63. Did this use to fall under the "in-range" case before? I thought we didn't use to warn when comparing against the boundaries of a type. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@320133 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index 6e7978c9a1..c5e4f26164 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/lib/Sema/SemaChecking.cpp @@ -8801,7 +8801,12 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E, Expr *Constant, Expr *Other, const llvm::APSInt &Value, bool RhsConstant) { - if (S.inTemplateInstantiation()) + // Disable warning in template instantiations + // and only analyze <, >, <= and >= operations. + if (S.inTemplateInstantiation() || !E->isRelationalOp()) + return false; + + if (IsEnumConstOrFromMacro(S, Constant)) return false; Expr *OriginalOther = Other; @@ -8828,23 +8833,94 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E, OtherRange.Width = std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width); - // Determine the promoted range of the other type and see if a comparison of - // the constant against that range is tautological. + // Check whether the constant value can be represented in OtherRange. Bail + // out if so; this isn't an out-of-range comparison. PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(), Value.isUnsigned()); + auto Cmp = OtherPromotedRange.compare(Value); + if (Cmp != PromotedRange::Min && Cmp != PromotedRange::Max && + Cmp != PromotedRange::OnlyValue) + return false; + auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, RhsConstant); if (!Result) return false; - // Suppress the diagnostic for an in-range comparison if the constant comes - // from a macro or enumerator. We don't want to diagnose - // - // some_long_value <= INT_MAX - // - // when sizeof(int) == sizeof(long). - bool InRange = Cmp & PromotedRange::InRangeFlag; - if (InRange && IsEnumConstOrFromMacro(S, Constant)) + // Should be enough for uint128 (39 decimal digits) + SmallString<64> PrettySourceValue; + llvm::raw_svector_ostream OS(PrettySourceValue); + OS << Value; + + // FIXME: We use a somewhat different formatting for the cases involving + // boolean values for historical reasons. We should pick a consistent way + // of presenting these diagnostics. + if (Other->isKnownToHaveBooleanValue()) { + S.DiagRuntimeBehavior( + E->getOperatorLoc(), E, + S.PDiag(diag::warn_tautological_bool_compare) + << OS.str() << classifyConstantValue(Constant) + << OtherT << !OtherT->isBooleanType() << *Result + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); + return true; + } + + unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0) + ? (HasEnumType(OriginalOther) + ? diag::warn_unsigned_enum_always_true_comparison + : diag::warn_unsigned_always_true_comparison) + : diag::warn_tautological_constant_compare; + + S.Diag(E->getOperatorLoc(), Diag) + << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); + + return true; +} + +static bool DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E, + Expr *Constant, Expr *Other, + const llvm::APSInt &Value, + bool RhsConstant) { + // Disable warning in template instantiations. + if (S.inTemplateInstantiation()) + return false; + + Constant = Constant->IgnoreParenImpCasts(); + Other = Other->IgnoreParenImpCasts(); + + // TODO: Investigate using GetExprRange() to get tighter bounds + // on the bit ranges. + QualType OtherT = Other->getType(); + if (const auto *AT = OtherT->getAs()) + OtherT = AT->getValueType(); + IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT); + + // Whether we're treating Other as being a bool because of the form of + // expression despite it having another type (typically 'int' in C). + bool OtherIsBooleanDespiteType = + !OtherT->isBooleanType() && Other->isKnownToHaveBooleanValue(); + if (OtherIsBooleanDespiteType) + OtherRange = IntRange::forBoolType(); + + if (FieldDecl *Bitfield = Other->getSourceBitField()) + if (!Bitfield->getBitWidth()->isValueDependent()) + OtherRange.Width = + std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width); + + // Check whether the constant value can be represented in OtherRange. Bail + // out if so; this isn't an out-of-range comparison. + PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(), + Value.isUnsigned()); + auto Cmp = OtherPromotedRange.compare(Value); + + // If Value is in the range of possible Other values, this comparison is not + // tautological. + if (Cmp & PromotedRange::InRangeFlag) + return false; + + auto IsTrue = PromotedRange::constantValue(E->getOpcode(), Cmp, RhsConstant); + if (!IsTrue) return false; // If this is a comparison to an enum constant, include that @@ -8853,7 +8929,6 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E, if (const DeclRefExpr *DR = dyn_cast(Constant)) ED = dyn_cast(DR->getDecl()); - // Should be enough for uint128 (39 decimal digits) SmallString<64> PrettySourceValue; llvm::raw_svector_ostream OS(PrettySourceValue); if (ED) @@ -8861,30 +8936,14 @@ static bool CheckTautologicalComparison(Sema &S, BinaryOperator *E, else OS << Value; - // FIXME: We use a somewhat different formatting for the in-range cases and - // cases involving boolean values for historical reasons. We should pick a - // consistent way of presenting these diagnostics. - if (!InRange || Other->isKnownToHaveBooleanValue()) { - S.DiagRuntimeBehavior( - E->getOperatorLoc(), E, - S.PDiag(!InRange ? diag::warn_out_of_range_compare - : diag::warn_tautological_bool_compare) - << OS.str() << classifyConstantValue(Constant) - << OtherT << OtherIsBooleanDespiteType << *Result - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); - } else { - unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0) - ? (HasEnumType(OriginalOther) - ? diag::warn_unsigned_enum_always_true_comparison - : diag::warn_unsigned_always_true_comparison) - : diag::warn_tautological_constant_compare; - - S.Diag(E->getOperatorLoc(), Diag) - << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result - << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange(); - } + S.DiagRuntimeBehavior( + E->getOperatorLoc(), E, + S.PDiag(diag::warn_out_of_range_compare) + << OS.str() << classifyConstantValue(Constant) + << OtherT << OtherIsBooleanDespiteType << *IsTrue + << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange()); - return true; + return true; } /// Analyze the operands of the given comparison. Implements the @@ -8934,8 +8993,12 @@ static void AnalyzeComparison(Sema &S, BinaryOperator *E) { // Check whether an integer constant comparison results in a value // of 'true' or 'false'. + if (CheckTautologicalComparison(S, E, Const, Other, Value, RhsConstant)) return AnalyzeImpConvsInComparison(S, E); + + if (DiagnoseOutOfRangeComparison(S, E, Const, Other, Value, RhsConstant)) + return AnalyzeImpConvsInComparison(S, E); } }