[clang] [Sema] Handle large shift counts in GetExprRange (PR #68590)

[Björn Pettersson via cfe-commits]

https://github.com/bjope updated https://github.com/llvm/llvm-project/pull/68590

>From 44eedec9dae39e72d5474426a15a07d9be4144fc Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Mon, 9 Oct 2023 16:13:39 +0200
Subject: [PATCH 1/3] [Sema] Handle large shift counts in GetExprRange

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.
---
 clang/lib/Sema/SemaChecking.cpp  |  5 ++---
 clang/test/Sema/c2x-expr-range.c | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Sema/c2x-expr-range.c

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}}
+}

>From f8881531c6b5f8a7f71554c01c4e7df8e24e7f72 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Mon, 9 Oct 2023 17:34:41 +0200
Subject: [PATCH 2/3] [Sema] Handle large shift counts in GetExprRange

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.
---
 clang/docs/ReleaseNotes.rst      |  4 ++++
 clang/test/Sema/c2x-expr-range.c | 14 +++++---------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2d0302c399fb6f3..45ee6db4112b978 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -219,6 +219,10 @@ Bug Fixes in This Version
 - Clang no longer considers the loss of ``__unaligned`` qualifier from objects as
   an invalid conversion during method function overload resolution.
 
+- Fixed an issue when a shift count larger than ``__INT64_MAX__``, in a right
+  shift operation, could result in missing warnings about
+  ``shift count >= width of type`` or internal compiler error.
+
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang/test/Sema/c2x-expr-range.c b/clang/test/Sema/c2x-expr-range.c
index 1690a6280386bd5..0fe1bc547bbd353 100644
--- a/clang/test/Sema/c2x-expr-range.c
+++ b/clang/test/Sema/c2x-expr-range.c
@@ -1,19 +1,15 @@
 // 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)
-//
+// Regression test for bug where we used to hit assertion due to shift amount
+// being larger than 64 bits. We want to see a warning about too large shift
+// amount.
 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.
+// Similar to test1 above, but using __uint128_t instead of __BitInt.
+// We want to see a warning about too large shift amount.
 void test2(__uint128_t *a)
 {
   (void)(*a >> ((__uint128_t)__UINT64_MAX__ + (__uint128_t)1) <= 0); // expected-warning {{shift count >= width of type}}

>From 0de2c61a6e6de11c24f894bf103cbb56b3cfdb0a Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson at ericsson.com>
Date: Mon, 9 Oct 2023 19:08:33 +0200
Subject: [PATCH 3/3] Update test case given review feedback.

---
 clang/test/Sema/c2x-expr-range.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/clang/test/Sema/c2x-expr-range.c b/clang/test/Sema/c2x-expr-range.c
index 0fe1bc547bbd353..73683e6bfe684aa 100644
--- a/clang/test/Sema/c2x-expr-range.c
+++ b/clang/test/Sema/c2x-expr-range.c
@@ -1,16 +1,14 @@
 // RUN: %clang_cc1 -verify -fsyntax-only -std=c2x -triple=x86_64-unknown-linux %s
 
-// 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
 // being larger than 64 bits. We want to see a warning about too large shift
 // amount.
-void test1(int *a)
-{
+void test1(int *a) {
   (void)(*a >> 123456789012345678901uwb <= 0); // expected-warning {{shift count >= width of type}}
 }
 
 // Similar to test1 above, but using __uint128_t instead of __BitInt.
 // We want to see a warning about too large shift amount.
-void test2(__uint128_t *a)
-{
-  (void)(*a >> ((__uint128_t)__UINT64_MAX__ + (__uint128_t)1) <= 0); // expected-warning {{shift count >= width of type}}
+void test2(__uint128_t *a) {
+  (void)(*a >> ((__uint128_t)__UINT64_MAX__ + 1) <= 0); // expected-warning {{shift count >= width of type}}
 }