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.

Sign up for GitHub

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

Jump to bottom

[clang] [C23] Fix crash with _BitInt running clang-tidy #65889

Merged
merged 1 commit into from
Sep 20, 2023
Merged

[clang] [C23] Fix crash with _BitInt running clang-tidy #65889

merged 1 commit into from
Sep 20, 2023

Conversation

vabridgers
Copy link
Contributor

@vabridgers vabridgers commented Sep 10, 2023 •
edited by tbaederr
Loading

This crash was exposed recently in our randomized testing. _BitInts were not being handled properly during IntegerLiteral visitation. This patch addresses the problem for now.

The BitIntType has no getKind() method, so the FoldingSetID is taken from the APInt value representing the _BitInt(), similar to other methods in StmtProfile.cpp.

Crash seen (summary form):

clang-tidy: /llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From&) [with To = clang::BuiltinType; From = clang::QualType]: Assertion `isa(Val) && "cast() argument of incompatible type!"' failed

  #9 <address> decltype(auto) llvm::cast<clang::BuiltinType,
       clang::QualType>(clang::QualType const&)
       <src-root>/llvm/include/llvm/Support/Casting.h:566:3
 #10 <address> clang::BuiltinType const* clang::Type::castAs<clang::BuiltinType>() const
       <bin-root>/tools/clang/include/clang/AST/TypeNodes.inc:86:1
 #11 <address> (anonymous namespace)::StmtProfiler::VisitIntegerLiteral(
       clang::IntegerLiteral const*)
       <src-root>/clang/lib/AST/StmtProfile.cpp:1362:64
 #12 <address> clang::StmtVisitorBase<llvm::make_const_ptr,
       (anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*)
       <src-root>/clang/include/clang/AST/StmtNodes.inc:1225:1

Reviewed By: donat.nagy

@vabridgers vabridgers requested review from a team as code owners September 10, 2023 11:10
@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tidy clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Sep 10, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 10, 2023

@llvm/pr-subscribers-clang-tidy

Changes

This crash was exposed recently in our randomized testing. _BitInts were not being handled properly during IntegerLiteral visitation. This patch addresses the problem for now.

The BitIntType has no getKind() method, so the FoldingSetID is taken from the APInt value representing the _BitInt(), similar to other methods in StmtProfile.cpp.

Crash seen (summary form):

clang-tidy: /llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From&) [with To = clang::BuiltinType; From = clang::QualType]: Assertion `isa(Val) && "cast() argument of incompatible type!"' failed

...
#9

decltype(auto) llvm::cast<clang::BuiltinType,
clang::QualType>(clang::QualType const&)
/llvm/include/llvm/Support/Casting.h:566:3
#10 clang::BuiltinType const* clang::Type::castAsclang::BuiltinType() const
/tools/clang/include/clang/AST/TypeNodes.inc:86:1
#11 (anonymous namespace)::StmtProfiler::VisitIntegerLiteral(
clang::IntegerLiteral const*)
/clang/lib/AST/StmtProfile.cpp:1362:64
#12 clang::StmtVisitorBase<llvm::make_const_ptr,
(anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*)
/clang/include/clang/AST/StmtNodes.inc:1225:1
...

Reviewed By: donat.nagy

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

2 Files Affected:

  • (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c (+10)
  • (modified) clang/lib/AST/StmtProfile.cpp (+9-1)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c
new file mode 100644
index 000000000000000..55921205f34875e
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c
@@ -0,0 +1,10 @@
+// RUN: %check_clang_tidy %s bugprone-inc-dec-in-conditions %t
+
+#define foo(x)                                                 \
+  ({                                                           \
+    _BitInt(5) y = x;                                          \
+    16777215wb ?: ++y;                                         \
+  })
+
+_BitInt(8) v_401_0() { 0 && foo(0); }
+// CHECK-MESSAGES: warning 
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 27f71edd6f99b32..0a6bc388bd73e18 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -1333,7 +1333,15 @@ void StmtProfiler::VisitPredefinedExpr(const PredefinedExpr *S) {
 void StmtProfiler::VisitIntegerLiteral(const IntegerLiteral *S) {
   VisitExpr(S);
   S->getValue().Profile(ID);
-  ID.AddInteger(S->getType()->castAs()->getKind());
+
+  int FoldingSetID = 0;
+
+  if (S->getType()->isBitIntType())
+    FoldingSetID = S->getValue().getSExtValue();
+  else
+    FoldingSetID = S->getType()->castAs()->getKind();
+
+  ID.AddInteger(FoldingSetID);
 }
 
 void StmtProfiler::VisitFixedPointLiteral(const FixedPointLiteral *S) {

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 10, 2023

@llvm/pr-subscribers-clang

Changes

This crash was exposed recently in our randomized testing. _BitInts were not being handled properly during IntegerLiteral visitation. This patch addresses the problem for now.

The BitIntType has no getKind() method, so the FoldingSetID is taken from the APInt value representing the _BitInt(), similar to other methods in StmtProfile.cpp.

Crash seen (summary form):

clang-tidy: /llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From&) [with To = clang::BuiltinType; From = clang::QualType]: Assertion `isa(Val) && "cast() argument of incompatible type!"' failed

...
#9

decltype(auto) llvm::cast<clang::BuiltinType,
clang::QualType>(clang::QualType const&)
/llvm/include/llvm/Support/Casting.h:566:3
#10 clang::BuiltinType const* clang::Type::castAsclang::BuiltinType() const
/tools/clang/include/clang/AST/TypeNodes.inc:86:1
#11 (anonymous namespace)::StmtProfiler::VisitIntegerLiteral(
clang::IntegerLiteral const*)
/clang/lib/AST/StmtProfile.cpp:1362:64
#12 clang::StmtVisitorBase<llvm::make_const_ptr,
(anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*)
/clang/include/clang/AST/StmtNodes.inc:1225:1
...

Reviewed By: donat.nagy

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

2 Files Affected:

  • (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c (+10)
  • (modified) clang/lib/AST/StmtProfile.cpp (+9-1)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c
new file mode 100644
index 000000000000000..55921205f34875e
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions-bitint-no-crash.c
@@ -0,0 +1,10 @@
+// RUN: %check_clang_tidy %s bugprone-inc-dec-in-conditions %t
+
+#define foo(x)                                                 \
+  ({                                                           \
+    _BitInt(5) y = x;                                          \
+    16777215wb ?: ++y;                                         \
+  })
+
+_BitInt(8) v_401_0() { 0 && foo(0); }
+// CHECK-MESSAGES: warning 
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 27f71edd6f99b32..0a6bc388bd73e18 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -1333,7 +1333,15 @@ void StmtProfiler::VisitPredefinedExpr(const PredefinedExpr *S) {
 void StmtProfiler::VisitIntegerLiteral(const IntegerLiteral *S) {
   VisitExpr(S);
   S->getValue().Profile(ID);
-  ID.AddInteger(S->getType()->castAs()->getKind());
+
+  int FoldingSetID = 0;
+
+  if (S->getType()->isBitIntType())
+    FoldingSetID = S->getValue().getSExtValue();
+  else
+    FoldingSetID = S->getType()->castAs()->getKind();
+
+  ID.AddInteger(FoldingSetID);
 }
 
 void StmtProfiler::VisitFixedPointLiteral(const FixedPointLiteral *S) {

@vabridgers vabridgers mentioned this pull request Sep 10, 2023
Closed
HerrCai0907
HerrCai0907 reviewed Sep 12, 2023
View reviewed changes
Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I wonder whether we should also add some test in clang/analysis module?
Crash happens in StmtProfiler but we only add test case in clang-tidy level.

AaronBallman
AaronBallman reviewed Sep 12, 2023
View reviewed changes
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Please be sure to add a release note about the fix as well.

clang/lib/AST/StmtProfile.cpp Outdated Show resolved Hide resolved
@vabridgers
Copy link
Contributor Author

The status above is showing 1 change requested, but I think I've addressed all comments. Please review at your convenience. Thank you.

@vabridgers
[clang] [C23] Fix crash with _BitInt running clang-tidy
79e1ab6 
This crash was exposed recently in our randomized testing. _BitInts were
not being handled properly during IntegerLiteral visitation. This patch
addresses the problem for now.

The BitIntType has no getKind() method, so the FoldingSetID is taken
from the APInt value representing the _BitInt(), similar to other
methods in StmtProfile.cpp. Seems also the const qualifier was missing
on the nonstatic profile method of the BitIntType class.

Crash seen (summary form):

clang-tidy: <src-root>/llvm/include/llvm/Support/Casting.h:566:
decltype(auto) llvm::cast(const From&) [with To = clang::BuiltinType;
From = clang::QualType]: Assertion `isa<To>(Val) && "cast<Ty>() argument
of incompatible type!"' failed

...
  llvm#9 <address> decltype(auto) llvm::cast<clang::BuiltinType,
       clang::QualType>(clang::QualType const&)
       <src-root>/llvm/include/llvm/Support/Casting.h:566:3
 llvm#10 <address> clang::BuiltinType const* clang::Type::castAs<clang::BuiltinType>() const
       <bin-root>/tools/clang/include/clang/AST/TypeNodes.inc:86:1
 llvm#11 <address> (anonymous namespace)::StmtProfiler::VisitIntegerLiteral(
       clang::IntegerLiteral const*)
       <src-root>/clang/lib/AST/StmtProfile.cpp:1362:64
 llvm#12 <address> clang::StmtVisitorBase<llvm::make_const_ptr,
       (anonymous namespace)::StmtProfiler, void>::Visit(clang::Stmt const*)
       <src-root>/clang/include/clang/AST/StmtNodes.inc:1225:1
...

Reviewed By: donat.nagy
@vabridgers
Copy link
Contributor Author

I've updated the review per recent comments, please review at your convenience. Thanks!

@vabridgers vabridgers merged commit 1c05651 into llvm:main Sep 20, 2023
2 checks passed
@vabridgers vabridgers deleted the bitint-integerliteral-fix branch September 20, 2023 10:08
This was referenced Sep 20, 2023
Closed
Closed
@kstoimenov kstoimenov mentioned this pull request Sep 22, 2023
Closed
hnrklssn
hnrklssn reviewed Sep 29, 2023
View reviewed changes
clang/lib/AST/StmtProfile.cpp
ID.AddInteger(S->getType()->castAs<BuiltinType>()->getKind());

QualType T = S->getType();
ID.AddInteger(T->getTypeClass());
Copy link
Member

Choose a reason for hiding this comment

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

This addition of the type class doesn't observe the Canonical attribute, leading to IntegerLiterals with different types but same canonical types mismatching even when Canonical is true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oooh, good catch! Thanks for that! Can you contribute a patch to fix that? (That is, canonicalize T).

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing. I haven't been able to find a reproducer for upstream clang, so I don't have a test case, but it doesn't seem to break any tests at least. Waiting for it to push to github now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cor3ntin cor3ntin cor3ntin left review comments

@AaronBallman AaronBallman AaronBallman left review comments

@erichkeane erichkeane erichkeane approved these changes

@hnrklssn hnrklssn hnrklssn left review comments

@HerrCai0907 HerrCai0907 HerrCai0907 left review comments

@NagyDonat NagyDonat NagyDonat approved these changes

@PiotrZSL PiotrZSL Awaiting requested review from PiotrZSL PiotrZSL was automatically assigned from llvm/pr-subscribers-clang-tidy

@carlosgalvezp carlosgalvezp Awaiting requested review from carlosgalvezp carlosgalvezp was automatically assigned from llvm/pr-subscribers-clang-tidy

@njames93 njames93 Awaiting requested review from njames93 njames93 was automatically assigned from llvm/pr-subscribers-clang-tidy

Assignees

@steakhal steakhal

@NagyDonat NagyDonat

@AdamMagierFOSS AdamMagierFOSS

Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category clang-tidy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@vabridgers @llvmbot @cor3ntin @AaronBallman @erichkeane @hnrklssn @NagyDonat @HerrCai0907 @steakhal @AdamMagierFOSS