[clang][bytecode] Fix shifts with an allocated RHS

[tbaederr]

This was broken before because we ended up using a constructor that was disabled via assert(false). Use ShiftAP() if either LHS or RHS is allocated.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

This was broken before because we ended up using a constructor that was disabled via assert(false). Use ShiftAP() if either LHS or RHS is allocated.

---

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

3 Files Affected:

• (modified) clang/lib/AST/ByteCode/IntegralAP.h (+1-1)
• (modified) clang/lib/AST/ByteCode/Interp.h (+37-26)
• (modified) clang/test/AST/ByteCode/intap.cpp (+14)

diff --git a/clang/lib/AST/ByteCode/IntegralAP.h b/clang/lib/AST/ByteCode/IntegralAP.h
index 316c003e0e50c..e7499fc9bf5a9 100644
--- a/clang/lib/AST/ByteCode/IntegralAP.h
+++ b/clang/lib/AST/ByteCode/IntegralAP.h
@@ -131,8 +131,8 @@ template <bool Signed> class IntegralAP final {
     if (NumBits == 0)
       NumBits = sizeof(T) * 8;
     assert(NumBits > 0);
+    assert(APInt::getNumWords(NumBits) == 1);
     APInt Copy = APInt(NumBits, static_cast<uint64_t>(Value), Signed);
-    assert(false);
     return IntegralAP<Signed>(Copy);
   }
 
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 66d3e6d79e8b2..cb6eb1e76c434 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -2713,7 +2713,7 @@ inline bool RVOPtr(InterpState &S, CodePtr OpPC) {
 template <class LT, class RT, ShiftDir Dir>
 inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS,
                     LT *Result) {
-
+  static_assert(!needsAlloc<LT>());
   const unsigned Bits = LHS.bitWidth();
 
   // OpenCL 6.3j: shift values are effectively % word size of LHS.
@@ -2770,7 +2770,10 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS,
       LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(LHS),
                                 LT::AsUnsigned::from(RHS, Bits), Bits, &R);
     }
-  } else {
+    S.Stk.push<LT>(LT::from(R));
+    return true;
+  }
+
     // Right shift.
     if (Compare(RHS, RT::from(MaxShiftAmount, RHS.bitWidth())) ==
         ComparisonCategoryResult::Greater) {
@@ -2779,10 +2782,8 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS,
       // Do the shift on potentially signed LT, then convert to unsigned type.
       LT A;
       LT::shiftRight(LHS, LT::from(RHS, Bits), Bits, &A);
-      // LT::shiftRight(LHS, LT(RHSTemp), Bits, &A);
       R = LT::AsUnsigned::from(A);
     }
-  }
 
   S.Stk.push<LT>(LT::from(R));
   return true;
@@ -2790,40 +2791,43 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS,
 
 /// A version of DoShift that works on IntegralAP.
 template <class LT, class RT, ShiftDir Dir>
-inline bool DoShiftAP(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS,
-                      LT *Result) {
-  const unsigned Bits = LHS.bitWidth();
-  const APSInt &LHSAP = LHS.toAPSInt();
-  APSInt RHSAP = RHS.toAPSInt();
+inline bool DoShiftAP(InterpState &S, CodePtr OpPC, const APSInt &LHS,
+                      APSInt RHS, LT *Result) {
+  const unsigned Bits = LHS.getBitWidth();
 
   // OpenCL 6.3j: shift values are effectively % word size of LHS.
   if (S.getLangOpts().OpenCL)
-    RHSAP &= APSInt(llvm::APInt(RHSAP.getBitWidth(),
-                                static_cast<uint64_t>(LHSAP.getBitWidth() - 1)),
-                    RHSAP.isUnsigned());
+    RHS &=
+        APSInt(llvm::APInt(RHS.getBitWidth(), static_cast<uint64_t>(Bits - 1)),
+               RHS.isUnsigned());
 
   if (RHS.isNegative()) {
     // During constant-folding, a negative shift is an opposite shift. Such a
     // shift is not a constant expression.
     const SourceInfo &Loc = S.Current->getSource(OpPC);
-    S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt();
+    S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS; //.toAPSInt();
     if (!S.noteUndefinedBehavior())
       return false;
-    RHS = -RHS;
     return DoShiftAP<LT, RT,
                      Dir == ShiftDir::Left ? ShiftDir::Right : ShiftDir::Left>(
-        S, OpPC, LHS, RHS, Result);
+        S, OpPC, LHS, -RHS, Result);
   }
 
-  if (!CheckShift<Dir>(S, OpPC, LHS, RHS, Bits))
+  if (!CheckShift<Dir>(S, OpPC, static_cast<LT>(LHS), static_cast<RT>(RHS),
+                       Bits))
     return false;
 
+  unsigned SA = (unsigned)RHS.getLimitedValue(Bits - 1);
   if constexpr (Dir == ShiftDir::Left) {
-    unsigned SA = (unsigned)RHSAP.getLimitedValue(LHS.bitWidth() - 1);
-    Result->copy(LHSAP << SA);
+    if constexpr (needsAlloc<LT>())
+      Result->copy(LHS << SA);
+    else
+      *Result = LT(LHS << SA);
   } else {
-    unsigned SA = (unsigned)RHSAP.getLimitedValue(LHS.bitWidth() - 1);
-    Result->copy(LHSAP >> SA);
+    if constexpr (needsAlloc<LT>())
+      Result->copy(LHS >> SA);
+    else
+      *Result = LT(LHS >> SA);
   }
 
   S.Stk.push<LT>(*Result);
@@ -2837,9 +2841,12 @@ inline bool Shr(InterpState &S, CodePtr OpPC) {
   auto RHS = S.Stk.pop<RT>();
   auto LHS = S.Stk.pop<LT>();
 
-  if constexpr (needsAlloc<LT>()) {
-    LT Result = S.allocAP<LT>(LHS.bitWidth());
-    return DoShiftAP<LT, RT, ShiftDir::Right>(S, OpPC, LHS, RHS, &Result);
+  if constexpr (needsAlloc<LT>() || needsAlloc<RT>()) {
+    LT Result;
+    if constexpr (needsAlloc<LT>())
+      Result = S.allocAP<LT>(LHS.bitWidth());
+    return DoShiftAP<LT, RT, ShiftDir::Right>(S, OpPC, LHS.toAPSInt(),
+                                              RHS.toAPSInt(), &Result);
   } else {
     LT Result;
     return DoShift<LT, RT, ShiftDir::Right>(S, OpPC, LHS, RHS, &Result);
@@ -2852,9 +2859,13 @@ inline bool Shl(InterpState &S, CodePtr OpPC) {
   using RT = typename PrimConv<NameR>::T;
   auto RHS = S.Stk.pop<RT>();
   auto LHS = S.Stk.pop<LT>();
-  if constexpr (needsAlloc<LT>()) {
-    LT Result = S.allocAP<LT>(LHS.bitWidth());
-    return DoShiftAP<LT, RT, ShiftDir::Left>(S, OpPC, LHS, RHS, &Result);
+
+  if constexpr (needsAlloc<LT>() || needsAlloc<RT>()) {
+    LT Result;
+    if constexpr (needsAlloc<LT>())
+      Result = S.allocAP<LT>(LHS.bitWidth());
+    return DoShiftAP<LT, RT, ShiftDir::Left>(S, OpPC, LHS.toAPSInt(),
+                                             RHS.toAPSInt(), &Result);
   } else {
     LT Result;
     return DoShift<LT, RT, ShiftDir::Left>(S, OpPC, LHS, RHS, &Result);
diff --git a/clang/test/AST/ByteCode/intap.cpp b/clang/test/AST/ByteCode/intap.cpp
index 3f952ddf626b5..11dd9edb61a94 100644
--- a/clang/test/AST/ByteCode/intap.cpp
+++ b/clang/test/AST/ByteCode/intap.cpp
@@ -273,4 +273,18 @@ namespace IncDec {
 #endif
 }
 
+#if __cplusplus >= 201402L
+const __int128_t a = ( (__int128_t)1 << 64 );
+const _BitInt(72) b = ( 1 << 72 ); // both-warning {{shift count >= width of type}}
+constexpr int shifts() { // both-error {{never produces a constant expression}}
+  (void)(2 >> a); // both-warning {{shift count >= width of type}} \
+                  // both-note {{shift count 18446744073709551616 >= width of type 'int' (32 bits)}}
+  (void)(2 >> b); // ref-warning {{shift count is negative}}
+  (void)(2 << a); // both-warning {{shift count >= width of type}}
+  (void)(2 << b); // ref-warning {{shift count is negative}}
+  return 1;
+}
+#endif
+
+
 #endif

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- clang/lib/AST/ByteCode/IntegralAP.h clang/lib/AST/ByteCode/Interp.h clang/test/AST/ByteCode/intap.cpp

View the diff from clang-format here.

diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index cb6eb1e76..1b81ea4de 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -2808,9 +2808,10 @@ inline bool DoShiftAP(InterpState &S, CodePtr OpPC, const APSInt &LHS,
     S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS; //.toAPSInt();
     if (!S.noteUndefinedBehavior())
       return false;
-    return DoShiftAP<LT, RT,
-                     Dir == ShiftDir::Left ? ShiftDir::Right : ShiftDir::Left>(
-        S, OpPC, LHS, -RHS, Result);
+    return DoShiftAP < LT, RT,
+           Dir == ShiftDir::Left
+               ? ShiftDir::Right
+               : ShiftDir::Left > (S, OpPC, LHS, -RHS, Result);
   }
 
   if (!CheckShift<Dir>(S, OpPC, static_cast<LT>(LHS), static_cast<RT>(RHS),