From f72e8e1a6a63b4a6e0f4095f20f551703a037c77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= Date: Fri, 13 Dec 2024 17:02:22 +0100 Subject: [PATCH] [clang][bytecode] Fix some shift edge cases Around shifting negative values. --- clang/lib/AST/ByteCode/Integral.h | 7 ++- clang/lib/AST/ByteCode/Interp.h | 68 +++++++++++++++--------------- clang/test/AST/ByteCode/shifts.cpp | 41 +++++++++++------- 3 files changed, 65 insertions(+), 51 deletions(-) diff --git a/clang/lib/AST/ByteCode/Integral.h b/clang/lib/AST/ByteCode/Integral.h index 26585799e5eadc..13fdb5369f2b7a 100644 --- a/clang/lib/AST/ByteCode/Integral.h +++ b/clang/lib/AST/ByteCode/Integral.h @@ -179,7 +179,10 @@ template class Integral final { unsigned countLeadingZeros() const { if constexpr (!Signed) return llvm::countl_zero(V); - llvm_unreachable("Don't call countLeadingZeros() on signed types."); + if (isPositive()) + return llvm::countl_zero( + static_cast(V)); + llvm_unreachable("Don't call countLeadingZeros() on negative values."); } Integral truncate(unsigned TruncBits) const { @@ -210,7 +213,7 @@ template class Integral final { return Integral(Value.V); } - static Integral zero() { return from(0); } + static Integral zero(unsigned BitWidth = 0) { return from(0); } template static Integral from(T Value, unsigned NumBits) { return Integral(Value); diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h index cdf05e36304acb..f085f96fdf5391 100644 --- a/clang/lib/AST/ByteCode/Interp.h +++ b/clang/lib/AST/ByteCode/Interp.h @@ -2511,50 +2511,52 @@ inline bool DoShift(InterpState &S, CodePtr OpPC, LT &LHS, RT &RHS) { S, OpPC, LHS, RHS); } - if constexpr (Dir == ShiftDir::Left) { - if (LHS.isNegative() && !S.getLangOpts().CPlusPlus20) { - // 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. - const SourceInfo &Loc = S.Current->getSource(OpPC); - S.CCEDiag(Loc, diag::note_constexpr_lshift_of_negative) << LHS.toAPSInt(); - if (!S.noteUndefinedBehavior()) - return false; - } - } - if (!CheckShift(S, OpPC, LHS, RHS, Bits)) return false; // Limit the shift amount to Bits - 1. If this happened, // it has already been diagnosed by CheckShift() above, // but we still need to handle it. + // Note that we have to be extra careful here since we're doing the shift in + // any case, but we need to adjust the shift amount or the way we do the shift + // for the potential error cases. typename LT::AsUnsigned R; + unsigned MaxShiftAmount = LHS.bitWidth() - 1; if constexpr (Dir == ShiftDir::Left) { - if (RHS > RT::from(Bits - 1, RHS.bitWidth())) - LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(LHS), - LT::AsUnsigned::from(Bits - 1), Bits, &R); - else + if (Compare(RHS, RT::from(MaxShiftAmount, RHS.bitWidth())) == + ComparisonCategoryResult::Greater) { + if (LHS.isNegative()) + R = LT::AsUnsigned::zero(LHS.bitWidth()); + else { + RHS = RT::from(LHS.countLeadingZeros(), RHS.bitWidth()); + LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(LHS), + LT::AsUnsigned::from(RHS, Bits), Bits, &R); + } + } else if (LHS.isNegative()) { + if (LHS.isMin()) { + R = LT::AsUnsigned::zero(LHS.bitWidth()); + } else { + // If the LHS is negative, perform the cast and invert the result. + typename LT::AsUnsigned LHSU = LT::AsUnsigned::from(-LHS); + LT::AsUnsigned::shiftLeft(LHSU, LT::AsUnsigned::from(RHS, Bits), Bits, + &R); + R = -R; + } + } else { + // The good case, a simple left shift. LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(LHS), LT::AsUnsigned::from(RHS, Bits), Bits, &R); + } } else { - if (RHS > RT::from(Bits - 1, RHS.bitWidth())) - LT::AsUnsigned::shiftRight(LT::AsUnsigned::from(LHS), - LT::AsUnsigned::from(Bits - 1), Bits, &R); - else - LT::AsUnsigned::shiftRight(LT::AsUnsigned::from(LHS), - LT::AsUnsigned::from(RHS, Bits), Bits, &R); - } - - // We did the shift above as unsigned. Restore the sign bit if we need to. - if constexpr (Dir == ShiftDir::Right) { - if (LHS.isSigned() && LHS.isNegative()) { - typename LT::AsUnsigned SignBit; - LT::AsUnsigned::shiftLeft(LT::AsUnsigned::from(1, Bits), - LT::AsUnsigned::from(Bits - 1, Bits), Bits, - &SignBit); - LT::AsUnsigned::bitOr(R, SignBit, Bits, &R); + // Right shift. + if (Compare(RHS, RT::from(MaxShiftAmount, RHS.bitWidth())) == + ComparisonCategoryResult::Greater) { + R = LT::AsUnsigned::from(-1); + } else { + // Do the shift on potentially signed LT, then convert to unsigned type. + LT A; + LT::shiftRight(LHS, LT::from(RHS, Bits), Bits, &A); + R = LT::AsUnsigned::from(A); } } diff --git a/clang/test/AST/ByteCode/shifts.cpp b/clang/test/AST/ByteCode/shifts.cpp index 0b3383731c6774..493f8b78204259 100644 --- a/clang/test/AST/ByteCode/shifts.cpp +++ b/clang/test/AST/ByteCode/shifts.cpp @@ -21,27 +21,15 @@ namespace shifts { c = 1 << 0; c = 1 << -0; c = 1 >> -0; - c = 1 << -1; // expected-warning {{shift count is negative}} \ - // expected-note {{negative shift count -1}} \ - // cxx17-note {{negative shift count -1}} \ - // cxx17-warning {{shift count is negative}} \ - // ref-warning {{shift count is negative}} \ - // ref-note {{negative shift count -1}} \ - // ref-cxx17-warning {{shift count is negative}} \ - // ref-cxx17-note {{negative shift count -1}} + c = 1 << -1; // all-warning {{shift count is negative}} \ + // all-note {{negative shift count -1}} c = 1 >> -1; // expected-warning {{shift count is negative}} \ // cxx17-warning {{shift count is negative}} \ // ref-warning {{shift count is negative}} \ // ref-cxx17-warning {{shift count is negative}} - c = 1 << (unsigned)-1; // expected-warning {{shift count >= width of type}} \ - // expected-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} \ - // cxx17-warning {{shift count >= width of type}} \ - // cxx17-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} \ - // ref-warning {{shift count >= width of type}} \ - // ref-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} \ - // ref-cxx17-warning {{shift count >= width of type}} \ - // ref-cxx17-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} + c = 1 << (unsigned)-1; // all-warning {{shift count >= width of type}} \ + // all-warning {{implicit conversion from 'int' to 'char' changes value from -2147483648 to 0}} c = 1 >> (unsigned)-1; // expected-warning {{shift count >= width of type}} \ // cxx17-warning {{shift count >= width of type}} \ // ref-warning {{shift count >= width of type}} \ @@ -212,3 +200,24 @@ enum shiftof { X3 = (1<<32) // all-error {{expression is not an integral constant expression}} \ // all-note {{shift count 32 >= width of type 'int'}} }; + +#if __WCHAR_WIDTH__ == 32 +static_assert(((wchar_t)-1U >> 31) == -1); +#endif + +#if __INT_WIDTH__ == 32 +static_assert(((int)-1U >> 32) == -1); // all-error {{not an integral constant expression}} \ + // all-note {{shift count 32 >= width of type 'int' (32 bits)}} +#endif + +static_assert((-4 << 32) == 0); // all-error {{not an integral constant expression}} \ + // all-note {{shift count}} + +static_assert((-4 << 1) == -8); // ref-cxx17-error {{not an integral constant expression}} \ + // ref-cxx17-note {{left shift of negative value -4}} \ + // cxx17-error {{not an integral constant expression}} \ + // cxx17-note {{left shift of negative value -4}} +static_assert((-4 << 31) == 0); // ref-cxx17-error {{not an integral constant expression}} \ + // ref-cxx17-note {{left shift of negative value -4}} \ + // cxx17-error {{not an integral constant expression}} \ + // cxx17-note {{left shift of negative value -4}}