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

[analyzer] Fix crash analyzing _BitInt() in evalIntegralCast #66782

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

vabridgers
Copy link
Contributor

@vabridgers vabridgers commented Sep 19, 2023

evalIntegralCast was using makeIntVal, and when _BitInt() types were introduced this exposed a crash in evalIntegralCast as a result.

This is a reapply of a previous patch that failed post merge on the arm buildbots, because arm cannot handle large
BitInts. Pinning the triple for the testcase solves that problem.

Improve evalIntegralCast to use makeIntVal more efficiently to avoid the crash exposed by use of _BitInt.

This was caught with our internal randomized testing.

/llvm/include/llvm/ADT/APInt.h:1510:
int64_t llvm::APInt::getSExtValue() const: Assertion
`getSignificantBits() <= 64 && "Too many bits for int64_t"' failed.a

...
#9

llvm::APInt::getSExtValue() const
/llvm/include/llvm/ADT/APInt.h:1510:5
llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::ento::SVal, clang::QualType, clang::QualType)
/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:607:24
clang::Expr const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&)
/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:413:61
...

Fixes: #61960

Reviewed By: donat.nagy

evalIntegralCast was using makeIntVal, and when _BitInt() types were
introduced this exposed a crash in evalIntegralCast as a result.

Improve evalIntegralCast to use makeIntVal more efficiently to avoid the
crash exposed by use of _BitInt.

This was caught with our internal randomized testing.

<src-root>/llvm/include/llvm/ADT/APInt.h:1510:
  int64_t llvm::APInt::getSExtValue() const: Assertion
  `getSignificantBits() <= 64 && "Too many bits for int64_t"' failed.a

...
 llvm#9 <address> llvm::APInt::getSExtValue() const
  <src-root>/llvm/include/llvm/ADT/APInt.h:1510:5
  llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
  clang::ento::SVal, clang::QualType, clang::QualType)
  <src-root>/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:607:24
  clang::Expr const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&)
  <src-root>/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:413:61
...

 Fixes: llvm#61960

 Reviewed By: donat.nagy
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html labels Sep 19, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Changes

evalIntegralCast was using makeIntVal, and when _BitInt() types were introduced this exposed a crash in evalIntegralCast as a result.

Improve evalIntegralCast to use makeIntVal more efficiently to avoid the crash exposed by use of _BitInt.

This was caught with our internal randomized testing.

<src-root>/llvm/include/llvm/ADT/APInt.h:1510:
int64_t llvm::APInt::getSExtValue() const: Assertion
`getSignificantBits() <= 64 && "Too many bits for int64_t"' failed.a

...
#9 <address> llvm::APInt::getSExtValue() const
<src-root>/llvm/include/llvm/ADT/APInt.h:1510:5
llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>,
clang::ento::SVal, clang::QualType, clang::QualType)
<src-root>/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:607:24
clang::Expr const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&)
<src-root>/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:413:61
...

Fixes: #61960

Reviewed By: donat.nagy


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

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Core/SValBuilder.cpp (+3-5)
  • (added) clang/test/Analysis/bitint-no-crash.c (+13)
diff --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index 4fe828bdf7681fc..f827f43eaa7da67 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -598,11 +598,9 @@ SVal SValBuilder::evalIntegralCast(ProgramStateRef state, SVal val,
   APSIntType ToType(getContext().getTypeSize(castTy),
                     castTy->isUnsignedIntegerType());
   llvm::APSInt ToTypeMax = ToType.getMaxValue();
-  NonLoc ToTypeMaxVal =
-      makeIntVal(ToTypeMax.isUnsigned() ? ToTypeMax.getZExtValue()
-                                        : ToTypeMax.getSExtValue(),
-                 castTy)
-          .castAs<NonLoc>();
+
+  NonLoc ToTypeMaxVal = makeIntVal(ToTypeMax);
+
   // Check the range of the symbol being casted against the maximum value of the
   // target type.
   NonLoc FromVal = val.castAs<NonLoc>();
diff --git a/clang/test/Analysis/bitint-no-crash.c b/clang/test/Analysis/bitint-no-crash.c
new file mode 100644
index 000000000000000..0a367fa930dc9b1
--- /dev/null
+++ b/clang/test/Analysis/bitint-no-crash.c
@@ -0,0 +1,13 @@
+ // RUN: %clang_analyze_cc1 -analyzer-checker=core \
+ // RUN:   -analyzer-checker=debug.ExprInspection \
+ // RUN:   -triple x86_64-pc-linux-gnu \
+ // RUN:   -verify %s
+
+// Don't crash when using _BitInt(). Pin to the x86_64 triple for now,
+// since not all architectures support _BitInt()
+// expected-no-diagnostics
+_BitInt(256) a;
+_BitInt(129) b;
+void c() {
+  b = a;
+}

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

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

This commit was reverted once (see discussion on earlier pull request #65887), because the buildbot linaro-clang-armv8-quick couldn't handle the large _BitInt()s in the testcase. Pinning to the x86_64 triple solves this problem, so it's safe to resubmit the commit now.

@vabridgers vabridgers merged commit da26500 into llvm:main Sep 20, 2023
4 checks passed
@vabridgers vabridgers deleted the bitint-staticanalysis-fix branch September 20, 2023 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ClangSA] APInt::getSExtValue() crash in SValBuilder::evalIntegralCast() with _BitInt of size > 128
3 participants