[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 Aug 26 10:59:50 PDT 2024
https://github.com/cwarner-8702 updated https://github.com/llvm/llvm-project/pull/101073
>From 24f52fbfb9117a6498769cebdc7b09ecbd7e019e 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 1/4] [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 46bf20e34ce041..05c37acda11912 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 1b025e8f90f7ba..b2d2a545b1ab5f 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.
+
- Improved :doc:`modernize-use-std-format
<clang-tidy/checks/modernize/use-std-format>` check to support replacing
member function calls too.
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 117310d404f6f4..6d72e970d53fb6 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 d7ab8a7a44fe68..b337b226001359 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 7bacf028192c65..d92df1da3134cc 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 d46f57521a97d3..6a704b9e91aeda 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -980,6 +980,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.
@@ -2791,24 +2797,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());
@@ -16943,17 +16953,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()) {
@@ -16973,7 +16982,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) {
@@ -16984,7 +16994,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;
@@ -16993,7 +17004,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;
}
@@ -17024,7 +17036,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.");
@@ -17037,6 +17050,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 =
>From 3f59abbd16a87ec9296cda453baee5a260ec1635 Mon Sep 17 00:00:00 2001
From: Chris Warner <cwarner at esri.com>
Date: Tue, 30 Jul 2024 08:55:28 -0700
Subject: [PATCH 2/4] CR: EugeneZelenko
---
.../bugprone/implicit-widening-of-multiplication-result.rst | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
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 6d72e970d53fb6..1e3a783eab2324 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
@@ -37,19 +37,19 @@ Options
.. option:: UseCXXStaticCastsInCppSources
When suggesting fix-its for C++ code, should C++-style ``static_cast<>()``'s
- be suggested, or C-style casts. Defaults to ``true``.
+ be suggested, or C-style casts. Defaults to `true`.
.. option:: UseCXXHeadersInCppSources
When suggesting to include the appropriate header in C++ code,
should ``<cstddef>`` header be suggested, or ``<stddef.h>``.
- Defaults to ``true``.
+ Defaults to `true`.
.. option:: IgnoreConstantIntExpr
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. Defaults to ``false``.
+ diagnostic or suggested fix. Defaults to `false`.
Examples:
>From ac27ca9624bb3da5db4b80c037fe8a1547fc3714 Mon Sep 17 00:00:00 2001
From: Chris Warner <cwarner at esri.com>
Date: Mon, 19 Aug 2024 15:30:35 -0700
Subject: [PATCH 3/4] Add test to unittests
---
clang/unittests/AST/ASTExprTest.cpp | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/clang/unittests/AST/ASTExprTest.cpp b/clang/unittests/AST/ASTExprTest.cpp
index 5ec6aea8edba38..d278364f154ef5 100644
--- a/clang/unittests/AST/ASTExprTest.cpp
+++ b/clang/unittests/AST/ASTExprTest.cpp
@@ -108,3 +108,19 @@ TEST(ASTExpr, InitListIsConstantInitialized) {
(void)FooInit->updateInit(Ctx, 2, Ref);
EXPECT_FALSE(FooInit->isConstantInitializer(Ctx, false));
}
+
+TEST(ASTExpr, CheckForUnsignedOverflowICE) {
+ auto AST = buildASTFromCode(R"cpp(
+ unsigned u = 1'000'000'000u * 8u;
+ )cpp");
+ ASTContext &Ctx = AST->getASTContext();
+ const VarDecl *Var = getVariableNode(AST.get(), "u");
+
+ const auto ICE = Var->getInit()->getIntegerConstantExpr(Ctx);
+ EXPECT_TRUE(ICE.has_value());
+ EXPECT_TRUE(ICE->isUnsigned());
+ EXPECT_EQ(3'705'032'704u, *ICE);
+
+ const auto ICEOverflowCheck = Var->getInit()->getIntegerConstantExpr(Ctx, nullptr, true);
+ EXPECT_FALSE(ICEOverflowCheck.has_value());
+}
>From 276e48b39864d2c91f97f501d9bc5ed59ca52ced Mon Sep 17 00:00:00 2001
From: Chris Warner <cwarner at esri.com>
Date: Tue, 20 Aug 2024 09:40:35 -0700
Subject: [PATCH 4/4] CR: another test
---
clang/unittests/AST/ASTExprTest.cpp | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/clang/unittests/AST/ASTExprTest.cpp b/clang/unittests/AST/ASTExprTest.cpp
index d278364f154ef5..f9d451d8c1927b 100644
--- a/clang/unittests/AST/ASTExprTest.cpp
+++ b/clang/unittests/AST/ASTExprTest.cpp
@@ -109,6 +109,24 @@ TEST(ASTExpr, InitListIsConstantInitialized) {
EXPECT_FALSE(FooInit->isConstantInitializer(Ctx, false));
}
+TEST(ASTExpr, NoUnsignedOverflowICE) {
+ auto AST = buildASTFromCode(R"cpp(
+ unsigned u = 1u * 8u;
+ )cpp");
+ ASTContext &Ctx = AST->getASTContext();
+ const VarDecl *Var = getVariableNode(AST.get(), "u");
+
+ const auto ICE = Var->getInit()->getIntegerConstantExpr(Ctx);
+ EXPECT_TRUE(ICE.has_value());
+ EXPECT_TRUE(ICE->isUnsigned());
+ EXPECT_EQ(8u, *ICE);
+
+ const auto ICEOverflowCheck = Var->getInit()->getIntegerConstantExpr(Ctx, nullptr, true);
+ EXPECT_TRUE(ICEOverflowCheck.has_value());
+ EXPECT_TRUE(ICEOverflowCheck->isUnsigned());
+ EXPECT_EQ(8u, *ICEOverflowCheck);
+}
+
TEST(ASTExpr, CheckForUnsignedOverflowICE) {
auto AST = buildASTFromCode(R"cpp(
unsigned u = 1'000'000'000u * 8u;
More information about the cfe-commits
mailing list