[clang][Interp] Add more shift error checking

[shafik]

clang/lib/AST/Interp/Interp.h

129

Maybe this quote for C++20 should be moved below the else block with a comment noting that C++20 forward the above is no longer UB? My first reading I thought the second quote applied to the else if below and was super confused.

132

Do we test this diagnostic?

[tbaeder]

clang/lib/AST/Interp/Interp.h

132

OH! No, and the 12 was just a debug value I forgot to replace with the real thing.

[shafik]

LGTM

clang/lib/AST/Interp/Interp.h

137–138

Just adding a reference to https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r4.html since that is the paper that changed this behavior

[aaron.ballman]

clang/test/AST/Interp/shifts.cpp

165–171

I'd like a test along the lines of:

constexpr int foo(int a) {
  return -a << 2;
}
static_assert(foo(10)); // Should fail
constexpr int a = -2;
static_assert(foo(a)); // Should be fine
static_assert(foo(-a)); // Should fail

[aaron.ballman]

LGTM!

clang/lib/AST/Interp/Integral.h

public:
 
   bool isNegative() const { return V < ReprT(0); }
   bool isPositive() const { return !isNegative(); }
 
   ComparisonCategoryResult compare(const Integral &RHS) const {
     return Compare(V, RHS.V);
   }
 
-  unsigned countLeadingZeros() const { return llvm::countl_zero<ReprT>(V); }
+  unsigned countLeadingZeros() const {
+    if constexpr (!Signed)
+      return llvm::countl_zero<ReprT>(V);
+    llvm_unreachable("Don't call countLeadingZeros() on signed types.");
+  }
 
   Integral truncate(unsigned TruncBits) const {
     if (TruncBits >= Bits)
       return *this;
     const ReprT BitMask = (ReprT(1) << ReprT(TruncBits)) - 1;
     const ReprT SignBit = ReprT(1) << (TruncBits - 1);
     const ReprT ExtMask = ~BitMask;
     return Integral((V & BitMask) | (Signed && (V & SignBit) ? ExtMask : 0));

clang/lib/AST/Interp/Interp.h

 
 /// Checks if a method is pure virtual.
 bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD);
 
 /// Checks that all fields are initialized after a constructor call.
 bool CheckCtorCall(InterpState &S, CodePtr OpPC, const Pointer &This);
 
 /// Checks if the shift operation is legal.
-template <typename RT>
-bool CheckShift(InterpState &S, CodePtr OpPC, const RT &RHS, unsigned Bits) {
+template <typename LT, typename RT>
+bool CheckShift(InterpState &S, CodePtr OpPC, const LT &LHS, const RT &RHS,
+                unsigned Bits) {
   if (RHS.isNegative()) {
     const SourceInfo &Loc = S.Current->getSource(OpPC);
     S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt();
     return false;
   }
 
   // C++11 [expr.shift]p1: Shift width must be less than the bit width of
   // the shifted type.
   if (Bits > 1 && RHS >= RT::from(Bits, RHS.bitWidth())) {
     const Expr *E = S.Current->getExpr(OpPC);
     const APSInt Val = RHS.toAPSInt();
     QualType Ty = E->getType();
     S.CCEDiag(E, diag::note_constexpr_large_shift) << Val << Ty << Bits;
     return false;
   }
+
+  if (LHS.isSigned() && !S.getLangOpts().CPlusPlus20) {
+    const Expr *E = S.Current->getExpr(OpPC);
+    // C++11 [expr.shift]p2: A signed left shift must have a non-negative
+    // operand, and must not overflow the corresponding unsigned type.
+    if (LHS.isNegative())
+      S.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << LHS.toAPSInt();
+    else if (LHS.toUnsigned().countLeadingZeros() < static_cast<unsigned>(RHS))
+      S.CCEDiag(E, diag::note_constexpr_lshift_discards);
+  }
+
+  // C++2a [expr.shift]p2: [P0907R4]:
+  //    E1 << E2 is the unique value congruent to
+  //    E1 x 2^E2 module 2^N.
   return true;
 }
 
 /// Checks if Div/Rem operation on LHS and RHS is valid.
 template <typename T>
 bool CheckDivRem(InterpState &S, CodePtr OpPC, const T &LHS, const T &RHS) {
   if (RHS.isZero()) {
     const SourceInfo &Loc = S.Current->getSource(OpPC);

 template <PrimType NameL, PrimType NameR>
 inline bool Shr(InterpState &S, CodePtr OpPC) {
   using LT = typename PrimConv<NameL>::T;
   using RT = typename PrimConv<NameR>::T;
   const auto &RHS = S.Stk.pop<RT>();
   const auto &LHS = S.Stk.pop<LT>();
   const unsigned Bits = LHS.bitWidth();
 
-  if (!CheckShift<RT>(S, OpPC, RHS, Bits))
+  if (!CheckShift(S, OpPC, LHS, RHS, Bits))
     return false;
 
   Integral<LT::bitWidth(), false> R;
   Integral<LT::bitWidth(), false>::shiftRight(LHS.toUnsigned(), RHS, Bits, &R);
   S.Stk.push<LT>(R);
   return true;
 }
 
 template <PrimType NameL, PrimType NameR>
 inline bool Shl(InterpState &S, CodePtr OpPC) {
   using LT = typename PrimConv<NameL>::T;
   using RT = typename PrimConv<NameR>::T;
   const auto &RHS = S.Stk.pop<RT>();
   const auto &LHS = S.Stk.pop<LT>();
   const unsigned Bits = LHS.bitWidth();
 
-  if (!CheckShift<RT>(S, OpPC, RHS, Bits))
+  if (!CheckShift(S, OpPC, LHS, RHS, Bits))
     return false;
 
   Integral<LT::bitWidth(), false> R;
   Integral<LT::bitWidth(), false>::shiftLeft(LHS.toUnsigned(), RHS, Bits, &R);
   S.Stk.push<LT>(R);
   return true;
 }
 

clang/test/AST/Interp/shifts.cpp

namespace shifts {
   /// The purpose of these few lines is to test that we can shift more bits
   /// than an unsigned *of the host* has. There was a bug where we casted
   /// to host-unsigned. However, we cannot query what a host-unsigned even is
   /// here, so only test this on platforms where `sizeof(long long) > sizeof(unsigned)`.
   constexpr long long int L = 1;
   constexpr signed int R = (sizeof(unsigned) * 8) + 1;
   constexpr decltype(L) M  = (R > 32 && R < 64) ?  L << R : 0;
   constexpr decltype(L) M2 = (R > 32 && R < 64) ?  L >> R : 0;
+
+
+  constexpr int signedShift() { // cxx17-error {{never produces a constant expression}} \
+                                // ref-cxx17-error {{never produces a constant expression}}
+    return 1024 << 31; // cxx17-warning {{signed shift result}} \
+                       // ref-cxx17-warning {{signed shift result}} \
+                       // cxx17-note {{signed left shift discards bits}} \
+                       // ref-cxx17-note {{signed left shift discards bits}}
+  }
+
+  constexpr int negativeShift() { // cxx17-error {{never produces a constant expression}} \
+                                  // ref-cxx17-error {{never produces a constant expression}}
+    return -1 << 2; // cxx17-warning {{shifting a negative signed value is undefined}} \
+                    // ref-cxx17-warning {{shifting a negative signed value is undefined}} \
+                    // cxx17-note {{left shift of negative value -1}} \
+                    // ref-cxx17-note {{left shift of negative value -1}}
+  }
+
+  constexpr int foo(int a) {
+    return -a << 2; // cxx17-note {{left shift of negative value -10}} \
+                    // ref-cxx17-note {{left shift of negative value -10}} \
+                    // cxx17-note {{left shift of negative value -2}} \
+                    // ref-cxx17-note {{left shift of negative value -2}}
+  }
+  static_assert(foo(10)); // cxx17-error {{not an integral constant expression}} \
+                          // cxx17-note {{in call to 'foo(10)'}} \
+                          // ref-cxx17-error {{not an integral constant expression}} \
+                          // ref-cxx17-note {{in call to 'foo(10)'}}
+
+  constexpr int a = -2;
+  static_assert(foo(a));
+  static_assert(foo(-a)); // cxx17-error {{not an integral constant expression}} \
+                          // cxx17-note {{in call to 'foo(2)'}} \
+                          // ref-cxx17-error {{not an integral constant expression}} \
+                          // ref-cxx17-note {{in call to 'foo(2)'}}
 };