[analyzer] Fix crash analyzing _BitInt() in evalIntegralCast

[vabridgers]

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.

/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

[llvmbot]

@llvm/pr-subscribers-clang

Changes

evalIntegralCast is using APInt method to get the value of _BitInt() values after _BitInt() changes were introduced. Some of those methods assume values are less than or equal to 64-bits, which is not true for _BitInt() types. This change simply side steps that issue if the _BitInt() type is greater than 64 bits.

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

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/SValBuilder.cpp (+6)
• (added) clang/test/Analysis/bitint-no-crash.c (+11)

diff --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index 4fe828bdf7681fc..c9765e3a653e30a 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -598,6 +598,12 @@ SVal SValBuilder::evalIntegralCast(ProgramStateRef state, SVal val,
   APSIntType ToType(getContext().getTypeSize(castTy),
                     castTy->isUnsignedIntegerType());
   llvm::APSInt ToTypeMax = ToType.getMaxValue();
+  // With the introduction of _BitInt(), integral types can be
+  // > 64 bits. So check for this and skip the size checks
+  // falling back to making a non loc return type.
+  if (ToTypeMax.getSignificantBits() > 64) {
+    return makeNonLoc(se, originalTy, castTy);
+  }
   NonLoc ToTypeMaxVal =
       makeIntVal(ToTypeMax.isUnsigned() ? ToTypeMax.getZExtValue()
                                         : ToTypeMax.getSExtValue(),
diff --git a/clang/test/Analysis/bitint-no-crash.c b/clang/test/Analysis/bitint-no-crash.c
new file mode 100644
index 000000000000000..6fa041974a3c981
--- /dev/null
+++ b/clang/test/Analysis/bitint-no-crash.c
@@ -0,0 +1,11 @@
+ // RUN: %clang_analyze_cc1 -analyzer-checker=core \
+ // RUN:   -analyzer-checker=debug.ExprInspection \
+ // RUN:   -verify %s
+
+// Don't crash when using _BitInt()
+// expected-no-diagnostics
+_BitInt(256) a;
+_BitInt(129) b;
+void c() {
+  b = a;
+}

[steakhal]

I guess this is one sideeffect of getZExtValue and getSExtValue.
To me, it feels like all such calls could hit the same assert, thus it fixes this instance, but we still lack a generic solution to this problem at other places.
I'm not opposing to this fix, but it might make sense to step back and grep for these calls to see the wider picture.

[NagyDonat]

I agree that it would be useful to systematically check the use of the APSInt -> uint64_t conversions, because it's likely that there are other ones that can lead to crashes.

[vabridgers]

The status above shows 1 change requested, but I believe I've resolved the requested changes. Please review at your convenience. Thank you.

[NagyDonat]

Don't forget to update the commit message!

[steakhal]

What are the semantics of assigning a wider biting to a smaller one?
BTW LGTM.

[bjope]

I reverted this patch since buildbots have been complaining for almost an hour.
Here is the first failure mail I got:

The Buildbot has detected a failed build on builder clang-armv8-quick while building clang.

Full details are available at:
    https://lab.llvm.org/buildbot/#/builders/245/builds/14207

Worker for this Build: linaro-clang-armv8-quick

BUILD FAILED: failed 1 unexpected failures 38187 expected passes 71 expected failures 36091 unsupported tests (failure)

Step 5 (ninja check 1) failure: 1 unexpected failures 38187 expected passes 71 expected failures 36091 unsupported tests (failure)
******************** TEST 'Clang :: Analysis/bitint-no-crash.c' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/bin/clang -cc1 -internal-isystem /home/tcwg-buildbot/worker/clang-armv8-quick/stage1/lib/clang/18/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core    -analyzer-checker=debug.ExprInspection    -verify /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/bitint-no-crash.c
--
Exit Code: 1

Command Output (stderr):
--
error: 'expected-error' diagnostics seen but not expected: 
  File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/bitint-no-crash.c Line 7: signed _BitInt of bit sizes greater than 128 not supported
  File /home/tcwg-buildbot/worker/clang-armv8-quick/llvm/clang/test/Analysis/bitint-no-crash.c Line 8: signed _BitInt of bit sizes greater than 128 not supported
2 errors generated.

--