[clang] [clang-tools-extra] [clang-tidy] bugprone-implicit-widening ignores unsigned consts (PR #101073)
Chris Warner via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 29 12:45:55 PDT 2024
https://github.com/cwarner-8702 created https://github.com/llvm/llvm-project/pull/101073
Expanding on [the previous PR](https://github.com/llvm/llvm-project/pull/98352), this is an attempt to remove the limitation that the option can only apply to signed integer types. Because unsigned types have well-defined behavior when they overflow (wrap around to 0), this wasn't something the Integer Constant Expression code is checking for.
The change was to add a flag to `EvalInfo` on whether to check for unsigned overflow or not, which is bubbled up through the `Expr` API and only set to `true` (at least so far) by the implicit-widening check. This should prevent changes in behavior anywhere else.
cc: @PiotrZSL
>From 39208a7b73893d1eef2078b3d212c92eb58e4457 Mon Sep 17 00:00:00 2001
From: Chris Warner <cwarner at esri.com>
Date: Wed, 17 Jul 2024 11:22:39 -0700
Subject: [PATCH] [clang-tidy] bugprone-implicit-widening ignores unsigned
consts
---
...citWideningOfMultiplicationResultCheck.cpp | 12 +++---
clang-tools-extra/docs/ReleaseNotes.rst | 5 +++
...icit-widening-of-multiplication-result.rst | 3 +-
...ing-of-multiplication-result-constants.cpp | 15 +++++--
clang/include/clang/AST/Expr.h | 11 +++--
clang/lib/AST/ExprConstant.cpp | 40 +++++++++++++------
6 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
index 46bf20e34ce04..05c37acda1191 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
@@ -88,13 +88,15 @@ void ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
// Is the expression a compile-time constexpr that we know can fit in the
// source type?
- if (IgnoreConstantIntExpr && ETy->isIntegerType() &&
- !ETy->isUnsignedIntegerType()) {
- if (const auto ConstExprResult = E->getIntegerConstantExpr(*Context)) {
+ if (IgnoreConstantIntExpr && ETy->isIntegerType()) {
+ if (const auto ConstExprResult =
+ E->getIntegerConstantExpr(*Context, nullptr, true)) {
const auto TypeSize = Context->getTypeSize(ETy);
+ const auto Unsigned = ETy->isUnsignedIntegerType();
+
llvm::APSInt WidenedResult = ConstExprResult->extOrTrunc(TypeSize);
- if (WidenedResult <= llvm::APSInt::getMaxValue(TypeSize, false) &&
- WidenedResult >= llvm::APSInt::getMinValue(TypeSize, false))
+ if (WidenedResult <= llvm::APSInt::getMaxValue(TypeSize, Unsigned) &&
+ WidenedResult >= llvm::APSInt::getMinValue(TypeSize, Unsigned))
return;
}
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 642ad39cc0c1c..8b369357d36b2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,11 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Improved :doc:`bugprone-implicit-widening-of-multiplication-result
+ <clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result>` check
+ by allowing the `IgnoreConstantIntExpr` option to apply to both signed and
+ unsigned integer types.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
index 117310d404f6f..6d72e970d53fb 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
@@ -49,8 +49,7 @@ Options
If the multiplication operands are compile-time constants (like literals or
are ``constexpr``) and fit within the source expression type, do not emit a
- diagnostic or suggested fix. Only considers expressions where the source
- expression is a signed integer type. Defaults to ``false``.
+ diagnostic or suggested fix. Defaults to ``false``.
Examples:
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp
index d7ab8a7a44fe6..b337b22600135 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp
@@ -40,6 +40,11 @@ long t5() {
return 1 * min_int;
}
+long t6() {
+ const unsigned int a = 4294967295u; // unsigned int max
+ return a * 1u;
+}
+
unsigned long n0() {
const int a = 1073741824; // 1/2 of int32_t max
const int b = 3;
@@ -50,7 +55,11 @@ unsigned long n0() {
// CHECK-MESSAGES: :[[@LINE-4]]:10: note: perform multiplication in a wider type
}
-double n1() {
- const long a = 100000000;
- return a * 400;
+long n1() {
+ const unsigned int a = 4294967295u; // unsigned int max
+ return a * 3u;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'unsigned int'
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+ // CHECK-MESSAGES: static_cast<long>( )
+ // CHECK-MESSAGES: :[[@LINE-4]]:10: note: perform multiplication in a wider type
}
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 5b813bfc2faf9..9191ce93fce85 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -551,11 +551,15 @@ class Expr : public ValueStmt {
/// and fill in Loc (if specified) with the location of the invalid
/// expression.
///
+ /// If CheckUnsignedOverflow is true, the i-c-e is of an unsigned type, and
+ /// the i-c-e result cannot fit into the expression type without overflowing,
+ /// std::nullopt is returned.
+ ///
/// Note: This does not perform the implicit conversions required by C++11
/// [expr.const]p5.
std::optional<llvm::APSInt>
- getIntegerConstantExpr(const ASTContext &Ctx,
- SourceLocation *Loc = nullptr) const;
+ getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc = nullptr,
+ bool CheckUnsignedOverflow = false) const;
bool isIntegerConstantExpr(const ASTContext &Ctx,
SourceLocation *Loc = nullptr) const;
@@ -569,7 +573,8 @@ class Expr : public ValueStmt {
/// Note: This does not perform the implicit conversions required by C++11
/// [expr.const]p5.
bool isCXX11ConstantExpr(const ASTContext &Ctx, APValue *Result = nullptr,
- SourceLocation *Loc = nullptr) const;
+ SourceLocation *Loc = nullptr,
+ bool CheckUnsignedOverflow = false) const;
/// isPotentialConstantExpr - Return true if this function's definition
/// might be usable in a constant expression in C++11, if it were marked
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 558e20ed3e423..3349c7d88ca48 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -966,6 +966,12 @@ namespace {
/// is set; this is used when evaluating ICEs in C.
bool CheckingForUndefinedBehavior = false;
+ /// Whether we're checking for an expression that causing the unsigned
+ /// integer expression type to overflow and wrap-around back to 0 when
+ /// evaluating an ICE. If set to true, the ICE evaluation will fail as
+ /// though the expression could not be calculated at compile time
+ bool CheckingForUnsignedIntegerOverflow = false;
+
enum EvaluationMode {
/// Evaluate as a constant expression. Stop if we find that the expression
/// is not a constant expression.
@@ -2772,24 +2778,28 @@ static bool truncateBitfieldValue(EvalInfo &Info, const Expr *E,
/// Perform the given integer operation, which is known to need at most BitWidth
/// bits, and check for overflow in the original type (if that type was not an
-/// unsigned type).
+/// unsigned type or EvalInfo.CheckingForUnsignedIntegerOverflow is true).
template<typename Operation>
static bool CheckedIntArithmetic(EvalInfo &Info, const Expr *E,
const APSInt &LHS, const APSInt &RHS,
unsigned BitWidth, Operation Op,
APSInt &Result) {
- if (LHS.isUnsigned()) {
+ if (LHS.isUnsigned() && !Info.CheckingForUnsignedIntegerOverflow) {
Result = Op(LHS, RHS);
return true;
}
- APSInt Value(Op(LHS.extend(BitWidth), RHS.extend(BitWidth)), false);
+ APSInt Value(Op(LHS.extend(BitWidth), RHS.extend(BitWidth)),
+ LHS.isUnsigned());
Result = Value.trunc(LHS.getBitWidth());
if (Result.extend(BitWidth) != Value) {
+ if (Result.isUnsigned())
+ return false;
if (Info.checkingForUndefinedBehavior())
Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
diag::warn_integer_constant_overflow)
- << toString(Result, 10, Result.isSigned(), /*formatAsCLiteral=*/false,
+ << toString(Result, 10, Result.isSigned(),
+ /*formatAsCLiteral=*/false,
/*UpperCase=*/true, /*InsertSeparators=*/true)
<< E->getType() << E->getSourceRange();
return HandleOverflow(Info, E, Value, E->getType());
@@ -16776,17 +16786,16 @@ static ICEDiag CheckICE(const Expr* E, const ASTContext &Ctx) {
}
/// Evaluate an expression as a C++11 integral constant expression.
-static bool EvaluateCPlusPlus11IntegralConstantExpr(const ASTContext &Ctx,
- const Expr *E,
- llvm::APSInt *Value,
- SourceLocation *Loc) {
+static bool EvaluateCPlusPlus11IntegralConstantExpr(
+ const ASTContext &Ctx, const Expr *E, llvm::APSInt *Value,
+ SourceLocation *Loc, bool CheckUnsignedOverflow) {
if (!E->getType()->isIntegralOrUnscopedEnumerationType()) {
if (Loc) *Loc = E->getExprLoc();
return false;
}
APValue Result;
- if (!E->isCXX11ConstantExpr(Ctx, &Result, Loc))
+ if (!E->isCXX11ConstantExpr(Ctx, &Result, Loc, CheckUnsignedOverflow))
return false;
if (!Result.isInt()) {
@@ -16806,7 +16815,8 @@ bool Expr::isIntegerConstantExpr(const ASTContext &Ctx,
ExprTimeTraceScope TimeScope(this, Ctx, "isIntegerConstantExpr");
if (Ctx.getLangOpts().CPlusPlus11)
- return EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, nullptr, Loc);
+ return EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, nullptr, Loc,
+ false);
ICEDiag D = CheckICE(this, Ctx);
if (D.Kind != IK_ICE) {
@@ -16817,7 +16827,8 @@ bool Expr::isIntegerConstantExpr(const ASTContext &Ctx,
}
std::optional<llvm::APSInt>
-Expr::getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc) const {
+Expr::getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc,
+ bool CheckUnsignedOverflow) const {
if (isValueDependent()) {
// Expression evaluator can't succeed on a dependent expression.
return std::nullopt;
@@ -16826,7 +16837,8 @@ Expr::getIntegerConstantExpr(const ASTContext &Ctx, SourceLocation *Loc) const {
APSInt Value;
if (Ctx.getLangOpts().CPlusPlus11) {
- if (EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, &Value, Loc))
+ if (EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, &Value, Loc,
+ CheckUnsignedOverflow))
return Value;
return std::nullopt;
}
@@ -16857,7 +16869,8 @@ bool Expr::isCXX98IntegralConstantExpr(const ASTContext &Ctx) const {
}
bool Expr::isCXX11ConstantExpr(const ASTContext &Ctx, APValue *Result,
- SourceLocation *Loc) const {
+ SourceLocation *Loc,
+ bool CheckUnsignedOverflow) const {
assert(!isValueDependent() &&
"Expression evaluator can't be called on a dependent expression.");
@@ -16870,6 +16883,7 @@ bool Expr::isCXX11ConstantExpr(const ASTContext &Ctx, APValue *Result,
SmallVector<PartialDiagnosticAt, 8> Diags;
Status.Diag = &Diags;
EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantExpression);
+ Info.CheckingForUnsignedIntegerOverflow = CheckUnsignedOverflow;
APValue Scratch;
bool IsConstExpr =
More information about the cfe-commits
mailing list