[clang-tidy] bugprone-implicit-widening ignores const exprs that fit #98352

cwarner-8702 · 2024-07-10T17:13:54Z

Add an option to the bugprone-implicit-widening-of-multiplication-result clang-tidy checker to suppress warnings when the expression is made up of all compile-time constants (literals, constexpr values or results, etc.) and the result of the multiplication is guaranteed to fit in both the source and destination types.

This currently only works for signed integer types:

For unsigned integers, the well-defined rollover behavior on overflow prevents the checker from detecting that the expression does not actually fit in the source expr type, and would produce false negatives. I have a somewhat-hacky stab at addressing this I'd like to submit as a follow-on PR
For floating-point types, there's probably a little additional work to be done to Expr to calculate the result at compile time

Even still, this seems like a helpful addition just for signed types, and additional types can be added.

llvmbot · 2024-07-10T17:14:22Z

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Chris Warner (cwarner-8702)

Changes

Add an option to the bugprone-implicit-widening-of-multiplication-result clang-tidy checker to suppress warnings when the expression is made up of all compile-time constants (literals, constexpr values or results, etc.) and the result of the multiplication is guaranteed to fit in both the source and destination types.

This currently only works for signed integer types:

For unsigned integers, the well-defined rollover behavior on overflow prevents the checker from detecting that the expression does not actually fit in the source expr type, and would produce false negatives. I have a somewhat-tacky stab at addressing this I'd like to submit as a follow-on PR
For floating-point types, there's probably a little additional work to be done to Expr to calculate the result at compile time

Even still, this seems like a helpful addition just for signed types, and additional types can be added.

Full diff: https://github.com/llvm/llvm-project/pull/98352.diff

4 Files Affected:

(modified) clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp (+15)
(modified) clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h (+1)
(modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst (+6)
(added) clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp (+81)

diff --git a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
index f99beac668ce7..46bf20e34ce04 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp
@@ -42,6 +42,7 @@ ImplicitWideningOfMultiplicationResultCheck::
       UseCXXStaticCastsInCppSources(
           Options.get("UseCXXStaticCastsInCppSources", true)),
       UseCXXHeadersInCppSources(Options.get("UseCXXHeadersInCppSources", true)),
+      IgnoreConstantIntExpr(Options.get("IgnoreConstantIntExpr", false)),
       IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
                                                utils::IncludeSorter::IS_LLVM),
                       areDiagsSelfContained()) {}
@@ -56,6 +57,7 @@ void ImplicitWideningOfMultiplicationResultCheck::storeOptions(
   Options.store(Opts, "UseCXXStaticCastsInCppSources",
                 UseCXXStaticCastsInCppSources);
   Options.store(Opts, "UseCXXHeadersInCppSources", UseCXXHeadersInCppSources);
+  Options.store(Opts, "IgnoreConstantIntExpr", IgnoreConstantIntExpr);
   Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle());
 }
 
@@ -84,6 +86,19 @@ void ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
   if (TgtWidth <= SrcWidth)
     return;
 
+  // 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)) {
+      const auto TypeSize = Context->getTypeSize(ETy);
+      llvm::APSInt WidenedResult = ConstExprResult->extOrTrunc(TypeSize);
+      if (WidenedResult <= llvm::APSInt::getMaxValue(TypeSize, false) &&
+          WidenedResult >= llvm::APSInt::getMinValue(TypeSize, false))
+        return;
+    }
+  }
+
   // Does the index expression look like it might be unintentionally computed
   // in a narrower-than-wanted type?
   const Expr *LHS = getLHSOfMulBinOp(E);
diff --git a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
index 8b99930ae7a89..077a4b847cd9c 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.h
@@ -41,6 +41,7 @@ class ImplicitWideningOfMultiplicationResultCheck : public ClangTidyCheck {
 private:
   const bool UseCXXStaticCastsInCppSources;
   const bool UseCXXHeadersInCppSources;
+  const bool IgnoreConstantIntExpr;
   utils::IncludeInserter IncludeInserter;
 };
 
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 c4ddd02602b73..6b857b9b82a21 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
@@ -45,6 +45,12 @@ Options
    should ``<cstddef>`` header be suggested, or ``<stddef.h>``.
    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.  Only considers expressions where the source
+   expression is a signed integer type.
 
 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
new file mode 100644
index 0000000000000..5a3dac0255df0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy -check-suffixes=ALL,NI %s bugprone-implicit-widening-of-multiplication-result %t -- -- -target x86_64-unknown-unknown -x c++
+// RUN: %check_clang_tidy -check-suffixes=ALL %s bugprone-implicit-widening-of-multiplication-result %t -- \
+// RUN:     -config='{CheckOptions: { \
+// RUN:         bugprone-implicit-widening-of-multiplication-result.IgnoreConstantIntExpr: true \
+// RUN:     }}' -- -target x86_64-unknown-unknown -x c++
+
+long t0() {
+  return 1 * 4;
+  // CHECK-NOTES-NI: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-NI: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-NI:                  static_cast<long>( )
+  // CHECK-NOTES-NI: :[[@LINE-4]]:10: note: perform multiplication in a wider type
+}
+
+unsigned long t1() {
+  const int a = 2;
+  const int b = 3;
+  return a * b;
+  // CHECK-NOTES-NI: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'unsigned long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-NI: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-NI:                  static_cast<unsigned long>( )
+  // CHECK-NOTES-NI: :[[@LINE-4]]:10: note: perform multiplication in a wider type
+}
+
+long t2() {
+  constexpr int a = 16383; // ~1/2 of int16_t max
+  constexpr int b = 2;
+  return a * b;
+  // CHECK-NOTES-NI: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-NI: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-NI:                  static_cast<long>( )
+  // CHECK-NOTES-NI: :[[@LINE-4]]:10: note: perform multiplication in a wider type
+}
+
+constexpr int global_value() {
+  return 16;
+}
+
+unsigned long t3() {
+  constexpr int a = 3;
+  return a * global_value();
+  // CHECK-NOTES-NI: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'unsigned long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-NI: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-NI:                  static_cast<unsigned long>( )
+  // CHECK-NOTES-NI: :[[@LINE-4]]:10: note: perform multiplication in a wider type
+}
+
+long t4() {
+  const char a = 3;
+  const short b = 2;
+  const int c = 5;
+  return c * b * a;
+  // CHECK-NOTES-NI: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-NI: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-NI:                  static_cast<long>( )
+  // CHECK-NOTES-NI: :[[@LINE-4]]:10: note: perform multiplication in a wider type
+}
+
+long t5() {
+  constexpr int min_int = (-2147483647 - 1); // A literal of -2147483648 evaluates to long
+  return 1 * min_int;
+  // CHECK-NOTES-NI: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-NI: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-NI:                  static_cast<long>( )
+  // CHECK-NOTES-NI: :[[@LINE-4]]:10: note: perform multiplication in a wider type
+}
+
+unsigned long n0() {
+  const int a = 1073741824; // 1/2 of int32_t max
+  const int b = 3;
+  return a * b;
+  // CHECK-NOTES-ALL: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'unsigned long' of a multiplication performed in type 'int'
+  // CHECK-NOTES-ALL: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-NOTES-ALL:                  static_cast<unsigned long>( )
+  // CHECK-NOTES-ALL: :[[@LINE-4]]:10: note: perform multiplication in a wider type
+}
+
+double n1() {
+  const long a = 100000000;
+  return a * 400;
+}

cwarner-8702 · 2024-07-10T17:15:11Z

@AaronBallman @njames93 Could you review?

PiotrZSL

Missing release notes entry about added new config options for check.

...g-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst

...a/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp

clang-tools-extra/clang-tidy/bugprone/ImplicitWideningOfMultiplicationResultCheck.cpp

...a/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp

PiotrZSL · 2024-07-10T18:14:32Z

You could also mention in documentation of option that those issues are detected by compiler warning: -Winteger-overflow

cwarner-8702 · 2024-07-10T19:26:25Z

@PiotrZSL Not sure what to mention about -Winteger-overflow, except maybe:

When the `IgnoreConstantIntExpr` option is enabled, this checker behaves like the `-Winteger-overflow` flag

Do you have other suggestions on what should be mentioned?

PiotrZSL · 2024-07-10T19:43:14Z

@PiotrZSL Not sure what to mention about -Winteger-overflow, except maybe:
When the `IgnoreConstantIntExpr` option is enabled, this checker behaves like the `-Winteger-overflow` flag
Do you have other suggestions on what should be mentioned?

Actually it's oposite. Simply when -Winteger-overflow is enabled, then IgnoreConstantIntExpr can be set to true, to avoid duplicated warnings. For me IgnoreConstantIntExpr should even by ON by default.

...g-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst

cwarner-8702 · 2024-07-10T20:12:14Z

Actually it's oposite. Simply when -Winteger-overflow is enabled, then IgnoreConstantIntExpr can be set to true, to avoid duplicated warnings. For me IgnoreConstantIntExpr should even by ON by default.

Sorry, I'm still struggling to see the connection. Maybe I'm getting too caught up on the fact one's for the compiler, the other is for clang-tidy, and they don't affect one another. If both are enabled, you will still get duplicated warnings if there is in fact an overflow problem. The change here is that if there isn't a real overflow problem, IgnoreConstantIntExpr will be silent just like -Winteger-overflow will.

So it just brings them into alignment.

For me IgnoreConstantIntExpr should even by ON by default.

I was trying to minimize impact, but I'm certainly open to it. :-)

PiotrZSL

I will merge this, as change looks fine at first glance. Any new fixes/changes please push in separate review.

PiotrZSL · 2024-07-15T19:34:42Z

...a/test/clang-tidy/checkers/bugprone/implicit-widening-of-multiplication-result-constants.cpp

+  return a * b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: performing an implicit widening conversion to type 'unsigned long' of a multiplication performed in type 'int'
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: note: make conversion explicit to silence this warning
+  // CHECK-MESSAGES:                  static_cast<unsigned long>( )


actually you should use CHECK-FIXES for this.

…98352) Summary: Add an option to the `bugprone-implicit-widening-of-multiplication-result` clang-tidy checker to suppress warnings when the expression is made up of all compile-time constants (literals, `constexpr` values or results, etc.) and the result of the multiplication is guaranteed to fit in both the source and destination types. This currently only works for signed integer types: * For unsigned integers, the well-defined rollover behavior on overflow prevents the checker from detecting that the expression does not actually fit in the source expr type, and would produce false negatives. I have a somewhat-hacky stab at addressing this I'd like to submit as a follow-on PR * For floating-point types, there's probably a little additional work to be done to `Expr` to calculate the result at compile time Even still, this seems like a helpful addition just for signed types, and additional types can be added. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251699

[clang-tidy] bugprone-implicit-widening ignores const exprs that fit

49c39b0

llvmbot added clang-tools-extra clang-tidy labels Jul 10, 2024

PiotrZSL reviewed Jul 10, 2024

View reviewed changes

CR: Piotr

ffe2d11

PiotrZSL reviewed Jul 10, 2024

View reviewed changes

...g-tools-extra/docs/clang-tidy/checks/bugprone/implicit-widening-of-multiplication-result.rst Show resolved Hide resolved

PiotrZSL approved these changes Jul 15, 2024

View reviewed changes

PiotrZSL merged commit f964e8a into llvm:main Jul 15, 2024
8 checks passed

cwarner-8702 deleted the users/cwarner-8702/implicit-widening-ignore-constexpr branch July 17, 2024 18:12

cwarner-8702 mentioned this pull request Jul 29, 2024

[clang-tidy] bugprone-implicit-widening ignores unsigned consts #101073

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-tidy] bugprone-implicit-widening ignores const exprs that fit #98352

[clang-tidy] bugprone-implicit-widening ignores const exprs that fit #98352

Uh oh!

cwarner-8702 commented Jul 10, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 10, 2024 •

edited

Loading

Uh oh!

cwarner-8702 commented Jul 10, 2024

Uh oh!

PiotrZSL left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL commented Jul 10, 2024

Uh oh!

cwarner-8702 commented Jul 10, 2024

Uh oh!

PiotrZSL commented Jul 10, 2024

Uh oh!

Uh oh!

cwarner-8702 commented Jul 10, 2024

Uh oh!

PiotrZSL left a comment

Uh oh!

PiotrZSL Jul 15, 2024

Uh oh!

Uh oh!

Uh oh!

[clang-tidy] bugprone-implicit-widening ignores const exprs that fit #98352

[clang-tidy] bugprone-implicit-widening ignores const exprs that fit #98352

Uh oh!

Conversation

cwarner-8702 commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwarner-8702 commented Jul 10, 2024

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL commented Jul 10, 2024

Uh oh!

cwarner-8702 commented Jul 10, 2024

Uh oh!

PiotrZSL commented Jul 10, 2024

Uh oh!

Uh oh!

cwarner-8702 commented Jul 10, 2024

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrZSL Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cwarner-8702 commented Jul 10, 2024 •

edited

Loading

llvmbot commented Jul 10, 2024 •

edited

Loading