[Sema] Handle large shift counts in GetExprRange

[bjope]

GetExprRange did not expect that very large shift counts when
narrowing the range based on logical right shifts. So with inputs
such as
*a >> 123456789012345678901uwb
it would hit assertions about trying to convert a too large APInt
into uint64_t.
This patch fixes that by using the APInt value when determining if
we should reduce the range by the shift count or not.

[llvmbot]

@llvm/pr-subscribers-clang

Changes

GetExprRange did not expect that very large shift counts when
narrowing the range based on logical right shifts. So with inputs
such as
*a >> 123456789012345678901uwb
it would hit assertions about trying to convert a too large APInt
into uint64_t.
This patch fixes that by using the APInt value when determining if
we should reduce the range by the shift count or not.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+2-3)
• (added) clang/test/Sema/c2x-expr-range.c (+20)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 3932d9cd07d9864..eb4d8d2e2806bcb 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -13554,11 +13554,10 @@ static IntRange GetExprRange(ASTContext &C, const Expr *E, unsigned MaxWidth,
       if (std::optional<llvm::APSInt> shift =
               BO->getRHS()->getIntegerConstantExpr(C)) {
         if (shift->isNonNegative()) {
-          unsigned zext = shift->getZExtValue();
-          if (zext >= L.Width)
+          if (shift->uge(L.Width))
             L.Width = (L.NonNegative ? 0 : 1);
           else
-            L.Width -= zext;
+            L.Width -= shift->getZExtValue();
         }
       }
 
diff --git a/clang/test/Sema/c2x-expr-range.c b/clang/test/Sema/c2x-expr-range.c
new file mode 100644
index 000000000000000..1690a6280386bd5
--- /dev/null
+++ b/clang/test/Sema/c2x-expr-range.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -std=c2x -triple=x86_64-unknown-linux %s
+
+// test1 used to hit an assertion failure like this during parsing:
+// clang: ../include/llvm/ADT/APInt.h:1488: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed.
+// With a stack trace like this leading up to the assert:
+//
+//  #9 0x00005645bca16518 GetExprRange(clang::ASTContext&, clang::Expr const*, unsigned int, bool, bool) SemaChecking.cpp:0:0
+// #10 0x00005645bca048a8 AnalyzeImplicitConversions(clang::Sema&, clang::Expr*, clang::SourceLocation, bool) SemaChecking.cpp:0:0
+// #11 0x00005645bca06af1 clang::Sema::CheckCompletedExpr(clang::Expr*, clang::SourceLocation, bool)
+//
+void test1(int *a)
+{
+  (void)(*a >> 123456789012345678901uwb <= 0); // expected-warning {{shift count >= width of type}}
+}
+
+// Same as above but using __uint128_t instead of __BitInt.
+void test2(__uint128_t *a)
+{
+  (void)(*a >> ((__uint128_t)__UINT64_MAX__ + (__uint128_t)1) <= 0); // expected-warning {{shift count >= width of type}}
+}

[erichkeane]

So the change itself looks right. The comment at the top of the test belongs in the commit message/github discussion, not in the test itself.

Also, needs a release note.

[bjope]

So the change itself looks right. The comment at the top of the test belongs in the commit message/github discussion, not in the test itself.

Also, needs a release note.

Thanks. I pushed an update. It fixes the test case to just explain the purpose of the tests. And I added info to the ReleaseNotes.

Note: I've never edited release notes for clang in the past, so please let me know if that should go somewhere else or if I missed something about what it should look like, etc.

[erichkeane]

Is there an issue filed in the issue tracker for this bug?

[bjope]

Is there an issue filed in the issue tracker for this bug?

Not that I'm aware of. Problem was found in downstream fuzzy testing involving BitInt types.

[erichkeane]

Suggested change

	// Regression test for bug where we used to hit assertion due to shift amount
	// Regression test for bug where we used to hit an assertion due to shift amount

[erichkeane]

curleys have to be on the same line as the declaration here by coding standard.

[erichkeane]

Same here.

[erichkeane]

2nd uint128_t cast shouldn't be necessary

[AaronBallman]

LGTM!