[clang-tidy] bugprone-implicit-widening ignores unsigned consts #101073
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-tools-extra Author: Chris Warner (cwarner-8702) ChangesExpanding on the previous PR, 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 cc: @PiotrZSL Full diff: https://github.com/llvm/llvm-project/pull/101073.diff 6 Files Affected:
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 =
|
EugeneZelenko
requested review from
PiotrZSL,
SimplyDanny,
njames93,
5chmidti and
HerrCai0907
...g-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst
Outdated
Show resolved
Hide resolved
|
@PiotrZSL Ok. I was hoping that since this new option is only used by the I need some help figuring out how to go about testing the change to the What would be easiest if if there was a test context like Gtest which calls into the code itself from a test, rather than The only other thing I can think of is adding a new diagnostic that can be detected by Is there some other novel way of testing |
|
Thats a good question. |
| @@ -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; | |||
There was a problem hiding this comment.
Given that unsigned overflow is well defined and allowed in constexpr, having an argument to disallow it seems a little off. From what I can tell the implementation here can just hard code the value internally to false and the public API for this function remains unchanged
There was a problem hiding this comment.
Given that unsigned overflow is well defined and allowed in constexpr, having an argument to disallow it seems a little off.
I agree from the compiler's perspective, but it seems in-line with the purpose of a bugprone clang tidy check: it's a hint that something may warrant investigation to make sure it is intentional.
From what I can tell the implementation here can just hard code the value internally to false and the public API for this function remains unchanged
I'm not seeing how, since this function sets up the EvalInfo that communications the option to the lower-level CheckedIntArithmetic function. It's not just checking if the expression is constant, it's also calculating the result, and the overflow is detected during that calculation.
There should definitely be a test in the clang side of things to ensure correct handling of the |
100% agree. What would be a good way to verify handling of it when the flag shouldn't the compiled output? |
cwarner-8702
force-pushed
the
users/cwarner-8702/implicit-widening-ignore-constexpr-unsigned
branch
from
2f8a23e to
276e48b
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…constexpr-unsigned
…constexpr-unsigned
|
@EugeneZelenko @PiotrZSL @njames93 @SimplyDanny @5chmidti @HerrCai0907 @AaronBallman Sorry for mass tagging folks, but this PR has been open for nearly 8 weeks with barely any feedback, and is starting to bit rot. I'd really appreciate some feedback, even if it's "this is terrible and should never be merged". Please take a look when you have some time. |
clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
Outdated
Show resolved
Hide resolved
|
Ping |
|
Looking at the clang-tidy changes only, this looks good. Though, I don't know if the changes to Clang are okay with Clang maintainers, but maybe they have an untapped use for this as well? Either way, a Clang maintainer needs to sign off on those changes before merging. Maybe there are also different ideas on how to detect overflow |
|
@5chmidti I agree. Do you know how I can get the attention of someone more familiar with clang internals, or who that might be? |
There was a problem hiding this comment.
Thank you for this, I think the clang-tidy portions make a lot of sense. However, I do have concerns about the changes to Clang. I don't think we want constant expression evaluation to fail based on an input parameter. For one, it's a bit odd because the constant expression is valid. But also, I don't think it will scale well if we want to add additional constraints on the evaluation in the future.
I think a better approach would be a new interface which doesn't return the APValue/APSInt directly, but instead returns something more like:
struct EvalResult {
APValue Value;
unsigned UnsignedWraparoundOccurred : 1;
unsigned VLAWasUsed : 1; // Just an example, not a real suggestion
unsigned DidSomeOtherQuestionableThing : 1; // Also an example :-)
};
This way, we can track whatever interesting details we want regarding the computation of the resulting value in a more extensible way, and we don't need to fail the constant expression evaluation itself.
CC @tbaederr who may also have opinions on constant expression evaluations or alternative ideas to consider
|
Thanks @AaronBallman!
It will already fail if the inputs as signed and expression overflows, so this seems like a case that all callers will have to deal with anyway.
I can work with that! Still open to other opinions or ideas too. |
Expanding on the previous PR, 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
EvalInfoon whether to check for unsigned overflow or not, which is bubbled up through theExprAPI and only set totrue(at least so far) by the implicit-widening check. This should prevent changes in behavior anywhere else.cc: @PiotrZSL