Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy] Avoid checking magic numbers if _BitInt #65888

Merged
merged 1 commit into from
Sep 17, 2023

Conversation

vabridgers
Copy link
Contributor

Recent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below.

/llvm/include/llvm/ADT/APInt.h:1488:
uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits()
<= 64 && "Too many bits for uint64_t"' failed.

...
#9

llvm::APInt::getZExtValue() const
/llvm/include/llvm/ADT/APInt.h:1488:5
clang::IntegerLiteral const*) const
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47
clang::IntegerLiteral(clang::ast_matchers::MatchFinder::MatchResult
const&, char const*)
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5
clang::ast_matchers::MatchFinder::MatchResult const&)
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35
...

Reviewed By: donat.nagy

Recent changes to add _BitInt support have caused our internal random
testing to fail. This change just avoids a readability magic numbers
check for now if a _BitInt. The crash seen (edited for clarity) is shown
below.

<src-root>/llvm/include/llvm/ADT/APInt.h:1488:
  uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits()
  <= 64 && "Too many bits for uint64_t"' failed.

...
 llvm#9 <address> llvm::APInt::getZExtValue() const
      <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5
      clang::IntegerLiteral const*) const
      <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47
      <clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult
      const&, char const*)
      <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5
      clang::ast_matchers::MatchFinder::MatchResult const&)
      <src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35
...

Reviewed By: donat.nagy
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 10, 2023

@llvm/pr-subscribers-clang-tidy

Changes

Recent changes to add _BitInt support have caused our internal random testing to fail. This change just avoids a readability magic numbers check for now if a _BitInt. The crash seen (edited for clarity) is shown below.

/llvm/include/llvm/ADT/APInt.h:1488:
uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits()
<= 64 && "Too many bits for uint64_t"' failed.

...
#9

llvm::APInt::getZExtValue() const
/llvm/include/llvm/ADT/APInt.h:1488:5
clang::IntegerLiteral const*) const
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47
clang::IntegerLiteral(clang::ast_matchers::MatchFinder::MatchResult
const&, char const*)
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5
clang::ast_matchers::MatchFinder::MatchResult const&)
/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35
...

Reviewed By: donat.nagy

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

2 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp (+3)
  • (added) clang-tools-extra/test/clang-tidy/checkers/readability/bitint-no-crash.c (+6)
diff --git a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
index 7f3c2cb9a0434cc..97c20cf200fa21c 100644
--- a/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -191,6 +191,9 @@ bool MagicNumbersCheck::isConstant(const MatchFinder::MatchResult &Result,
 }
 
 bool MagicNumbersCheck::isIgnoredValue(const IntegerLiteral *Literal) const {
+  if (Literal->getType()->isBitIntType()) {
+    return true;
+  }
   const llvm::APInt IntValue = Literal->getValue();
   const int64_t Value = IntValue.getZExtValue();
   if (Value == 0)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/bitint-no-crash.c b/clang-tools-extra/test/clang-tidy/checkers/readability/bitint-no-crash.c
new file mode 100644
index 000000000000000..f8660924cbef5a0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/bitint-no-crash.c
@@ -0,0 +1,6 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t --
+
+// Don't crash
+
+_BitInt(128) A = 4533629751480627964421wb;
+// CHECK-MESSAGES: warning

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
not perfect, as we going to ignore big ints, but still better than crashing for now.

@NagyDonat
Copy link
Contributor

LGTM as a temporary measure. Perhaps add a TODO note which asks for a more through solution; but you can also merge this as it is now.

@vabridgers vabridgers merged commit 4b5366c into llvm:main Sep 17, 2023
3 checks passed
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
Recent changes to add _BitInt support have caused our internal random
testing to fail. This change just avoids a readability magic numbers
check for now if a _BitInt. The crash seen (edited for clarity) is shown
below.

<src-root>/llvm/include/llvm/ADT/APInt.h:1488:
  uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits()
  <= 64 && "Too many bits for uint64_t"' failed.

...
 llvm#9 <address> llvm::APInt::getZExtValue() const
      <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5
      clang::IntegerLiteral const*) const

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47
<clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult
      const&, char const*)

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5
      clang::ast_matchers::MatchFinder::MatchResult const&)

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35
...

Reviewed By: donat.nagy
zahiraam pushed a commit to tahonermann/llvm-project that referenced this pull request Oct 24, 2023
Recent changes to add _BitInt support have caused our internal random
testing to fail. This change just avoids a readability magic numbers
check for now if a _BitInt. The crash seen (edited for clarity) is shown
below.

<src-root>/llvm/include/llvm/ADT/APInt.h:1488:
  uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits()
  <= 64 && "Too many bits for uint64_t"' failed.

...
 #9 <address> llvm::APInt::getZExtValue() const
      <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5
      clang::IntegerLiteral const*) const

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47
<clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult
      const&, char const*)

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5
      clang::ast_matchers::MatchFinder::MatchResult const&)

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35
...

Reviewed By: donat.nagy
zahiraam pushed a commit to tahonermann/llvm-project that referenced this pull request Oct 24, 2023
Recent changes to add _BitInt support have caused our internal random
testing to fail. This change just avoids a readability magic numbers
check for now if a _BitInt. The crash seen (edited for clarity) is shown
below.

<src-root>/llvm/include/llvm/ADT/APInt.h:1488:
  uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits()
  <= 64 && "Too many bits for uint64_t"' failed.

...
 #9 <address> llvm::APInt::getZExtValue() const
      <src-root>/llvm/include/llvm/ADT/APInt.h:1488:5
      clang::IntegerLiteral const*) const

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:198:47
<clang::IntegerLiteral>(clang::ast_matchers::MatchFinder::MatchResult
      const&, char const*)

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h:67:5
      clang::ast_matchers::MatchFinder::MatchResult const&)

<src-root>/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:152:35
...

Reviewed By: donat.nagy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants