[PATCH] D150209: [clang][Interp] Add more shift error checking
This revision was automatically updated to reflect the committed changes. Closed by commit rGd6b0af0574ca: [clang][Interp] Add more shift error checking (authored by tbaeder). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 Files: clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/test/AST/Interp/shifts.cpp Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,39 @@ 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)'}} }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -105,8 +105,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo = S.Current->getSource(OpPC); S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt(); @@ -122,6 +123,20 @@ 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(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; } @@ -1523,7 +1538,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; @@ -1540,7 +1555,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; Index: clang/lib/AST/Interp/Integral.h === --- clang/lib/AST/Interp/Integral.h +++ clang/lib/AST/Interp/Integral.h @@ -127,7 +127,11 @@ return Compare(V, RHS.V); } - unsigned countLeadingZeros() const { return llvm::countl_zero(V); } +
[PATCH] D150209: [clang][Interp] Add more shift error checking
aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150209: [clang][Interp] Add more shift error checking
tbaeder updated this revision to Diff 526965. tbaeder marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 Files: clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/test/AST/Interp/shifts.cpp Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,39 @@ 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)'}} }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo = S.Current->getSource(OpPC); S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt(); @@ -126,6 +127,20 @@ 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(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; } @@ -1612,7 +1627,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; @@ -1629,7 +1644,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; Index: clang/lib/AST/Interp/Integral.h === --- clang/lib/AST/Interp/Integral.h +++ clang/lib/AST/Interp/Integral.h @@ -127,7 +127,11 @@ return Compare(V, RHS.V); } - unsigned countLeadingZeros() const { return llvm::countl_zero(V); } + unsigned countLeadingZeros() const { +if constexpr (!Signed) + return llvm::countl_zero(V); +llvm_unreachable("Don't
[PATCH] D150209: [clang][Interp] Add more shift error checking
aaron.ballman added inline comments. Comment at: clang/test/AST/Interp/shifts.cpp:165-171 + 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}} + } 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 ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150209: [clang][Interp] Add more shift error checking
tbaeder updated this revision to Diff 521237. tbaeder marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 Files: clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/test/AST/Interp/shifts.cpp Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,21 @@ 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}} + } }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo = S.Current->getSource(OpPC); S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt(); @@ -126,6 +127,20 @@ 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(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; } @@ -1612,7 +1627,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; @@ -1629,7 +1644,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; Index: clang/lib/AST/Interp/Integral.h === --- clang/lib/AST/Interp/Integral.h +++ clang/lib/AST/Interp/Integral.h @@ -127,7 +127,11 @@ return Compare(V, RHS.V); } - unsigned countLeadingZeros() const { return llvm::countl_zero(V); } + unsigned countLeadingZeros() const { +if constexpr (!Signed) + return llvm::countl_zero(V); +llvm_unreachable("Don't call countLeadingZeros() on signed types."); + } Integral truncate(unsigned TruncBits) const { if (TruncBits >= Bits) Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,21 @@ 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}} +
[PATCH] D150209: [clang][Interp] Add more shift error checking
shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/Interp/Interp.h:141-142 + + // C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to + // E1 x 2^E2 module 2^N. + 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 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150209: [clang][Interp] Add more shift error checking
tbaeder updated this revision to Diff 521082. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 Files: clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/test/AST/Interp/shifts.cpp Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,21 @@ 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}} + } }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo = S.Current->getSource(OpPC); S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt(); @@ -126,6 +127,20 @@ 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(RHS)) + S.CCEDiag(E, diag::note_constexpr_lshift_discards); + } + + // C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to + // E1 x 2^E2 module 2^N. + return true; } @@ -1612,7 +1627,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; @@ -1629,7 +1644,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; Index: clang/lib/AST/Interp/Integral.h === --- clang/lib/AST/Interp/Integral.h +++ clang/lib/AST/Interp/Integral.h @@ -127,7 +127,11 @@ return Compare(V, RHS.V); } - unsigned countLeadingZeros() const { return llvm::countl_zero(V); } + unsigned countLeadingZeros() const { +if constexpr (!Signed) + return llvm::countl_zero(V); +llvm_unreachable("Don't call countLeadingZeros() on signed types."); + } Integral truncate(unsigned TruncBits) const { if (TruncBits >= Bits) Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,21 @@ 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
[PATCH] D150209: [clang][Interp] Add more shift error checking
tbaeder updated this revision to Diff 521078. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 Files: clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/test/AST/Interp/shifts.cpp Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,21 @@ 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}} + } }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo = S.Current->getSource(OpPC); S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt(); @@ -125,6 +126,16 @@ QualType Ty = E->getType(); S.CCEDiag(E, diag::note_constexpr_large_shift) << Val << Ty << Bits; return false; + } else 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. +// C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to +// E1 x 2^E2 module 2^N. +if (LHS.isNegative()) + S.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << LHS.toAPSInt(); +else if (LHS.toUnsigned().countLeadingZeros() < static_cast(RHS)) + S.CCEDiag(E, diag::note_constexpr_lshift_discards); } return true; } @@ -1612,7 +1623,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; @@ -1629,7 +1640,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; Index: clang/lib/AST/Interp/Integral.h === --- clang/lib/AST/Interp/Integral.h +++ clang/lib/AST/Interp/Integral.h @@ -127,7 +127,11 @@ return Compare(V, RHS.V); } - unsigned countLeadingZeros() const { return llvm::countl_zero(V); } + unsigned countLeadingZeros() const { +if constexpr (!Signed) + return llvm::countl_zero(V); +llvm_unreachable("Don't call countLeadingZeros() on signed types."); + } Integral truncate(unsigned TruncBits) const { if (TruncBits >= Bits) Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,21 @@ 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
[PATCH] D150209: [clang][Interp] Add more shift error checking
tbaeder updated this revision to Diff 520954. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 Files: clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/test/AST/Interp/shifts.cpp Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,14 @@ 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}} + } + }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo = S.Current->getSource(OpPC); S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt(); @@ -125,6 +126,16 @@ QualType Ty = E->getType(); S.CCEDiag(E, diag::note_constexpr_large_shift) << Val << Ty << Bits; return false; + } else 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. +// C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to +// E1 x 2^E2 module 2^N. +if (LHS.isNegative()) + S.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << LHS.toAPSInt(); +else if (LHS.toUnsigned().countLeadingZeros() < static_cast(RHS)) + S.CCEDiag(E, diag::note_constexpr_lshift_discards); } return true; } @@ -1612,7 +1623,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; @@ -1629,7 +1640,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; Index: clang/lib/AST/Interp/Integral.h === --- clang/lib/AST/Interp/Integral.h +++ clang/lib/AST/Interp/Integral.h @@ -127,7 +127,11 @@ return Compare(V, RHS.V); } - unsigned countLeadingZeros() const { return llvm::countl_zero(V); } + unsigned countLeadingZeros() const { +if constexpr (!Signed) + return llvm::countl_zero(V); +llvm_unreachable("Don't call countLeadingZeros() on signed types."); + } Integral truncate(unsigned TruncBits) const { if (TruncBits >= Bits) Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,14 @@ 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}} + } + }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo =
[PATCH] D150209: [clang][Interp] Add more shift error checking
tbaeder added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:136 +if (LHS.isNegative()) + S.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << 12; +else if (LHS.toUnsigned().countLeadingZeros() < static_cast(RHS)) shafik wrote: > Do we test this diagnostic? OH! No, and the `12` was just a debug value I forgot to replace with the real thing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150209: [clang][Interp] Add more shift error checking
shafik added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:133 +// operand, and must not overflow the corresponding unsigned type. +// C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to +// E1 x 2^E2 module 2^N. 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. Comment at: clang/lib/AST/Interp/Interp.h:136 +if (LHS.isNegative()) + S.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << 12; +else if (LHS.toUnsigned().countLeadingZeros() < static_cast(RHS)) Do we test this diagnostic? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150209/new/ https://reviews.llvm.org/D150209 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150209: [clang][Interp] Add more shift error checking
tbaeder created this revision. tbaeder added reviewers: aaron.ballman, erichkeane, tahonermann, shafik. Herald added a project: All. tbaeder requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D150209 Files: clang/lib/AST/Interp/Integral.h clang/lib/AST/Interp/Interp.h clang/test/AST/Interp/shifts.cpp Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,14 @@ 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}} + } + }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) { +template +bool CheckShift(InterpState , CodePtr OpPC, const LT , const RT , +unsigned Bits) { if (RHS.isNegative()) { const SourceInfo = S.Current->getSource(OpPC); S.CCEDiag(Loc, diag::note_constexpr_negative_shift) << RHS.toAPSInt(); @@ -125,6 +126,16 @@ QualType Ty = E->getType(); S.CCEDiag(E, diag::note_constexpr_large_shift) << Val << Ty << Bits; return false; + } else 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. +// C++2a [expr.shift]p2: E1 << E2 is the unique value congruent to +// E1 x 2^E2 module 2^N. +if (LHS.isNegative()) + S.CCEDiag(E, diag::note_constexpr_lshift_of_negative) << 12; +else if (LHS.toUnsigned().countLeadingZeros() < static_cast(RHS)) + S.CCEDiag(E, diag::note_constexpr_lshift_discards); } return true; } @@ -1612,7 +1623,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; @@ -1629,7 +1640,7 @@ const auto = S.Stk.pop(); const unsigned Bits = LHS.bitWidth(); - if (!CheckShift(S, OpPC, RHS, Bits)) + if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; Integral R; Index: clang/lib/AST/Interp/Integral.h === --- clang/lib/AST/Interp/Integral.h +++ clang/lib/AST/Interp/Integral.h @@ -127,7 +127,11 @@ return Compare(V, RHS.V); } - unsigned countLeadingZeros() const { return llvm::countl_zero(V); } + unsigned countLeadingZeros() const { +if constexpr (!Signed) + return llvm::countl_zero(V); +llvm_unreachable("Don't call countLeadingZeros() on signed types."); + } Integral truncate(unsigned TruncBits) const { if (TruncBits >= Bits) Index: clang/test/AST/Interp/shifts.cpp === --- clang/test/AST/Interp/shifts.cpp +++ clang/test/AST/Interp/shifts.cpp @@ -152,4 +152,14 @@ 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}} + } + }; Index: clang/lib/AST/Interp/Interp.h === --- clang/lib/AST/Interp/Interp.h +++ clang/lib/AST/Interp/Interp.h @@ -109,8 +109,9 @@ bool CheckCtorCall(InterpState , CodePtr OpPC, const Pointer ); /// Checks if the shift operation is legal. -template -bool CheckShift(InterpState , CodePtr OpPC, const RT , unsigned Bits) {