Skip to content

Commit f8288c1

Browse files
derekmaurocopybara-github
authored andcommitted
rotr/rotl: Fix undefined behavior when passing INT_MIN
as the number of positions to rotate by Previously the code was negating INT_MIN, which is undefined PiperOrigin-RevId: 770318129 Change-Id: Iff94b0e3d5777b2f488f2d48b6f8220f47bdada3
1 parent 9ac131c commit f8288c1

File tree

2 files changed

+38
-4
lines changed

2 files changed

+38
-4
lines changed

absl/numeric/bits_test.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,9 @@ TEST(Rotate, Left) {
151151
EXPECT_EQ(rotl(uint32_t{0x12345678UL}, -4), uint32_t{0x81234567UL});
152152
EXPECT_EQ(rotl(uint64_t{0x12345678ABCDEF01ULL}, -4),
153153
uint64_t{0x112345678ABCDEF0ULL});
154+
155+
EXPECT_EQ(rotl(uint32_t{1234}, std::numeric_limits<int>::min()),
156+
uint32_t{1234});
154157
}
155158

156159
TEST(Rotate, Right) {
@@ -190,6 +193,9 @@ TEST(Rotate, Right) {
190193
EXPECT_EQ(rotr(uint32_t{0x12345678UL}, -4), uint32_t{0x23456781UL});
191194
EXPECT_EQ(rotr(uint64_t{0x12345678ABCDEF01ULL}, -4),
192195
uint64_t{0x2345678ABCDEF011ULL});
196+
197+
EXPECT_EQ(rotl(uint32_t{1234}, std::numeric_limits<int>::min()),
198+
uint32_t{1234});
193199
}
194200

195201
TEST(Rotate, Symmetry) {

absl/numeric/internal/bits.h

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,28 @@ template <class T>
7777
static_assert(IsPowerOf2(std::numeric_limits<T>::digits),
7878
"T must have a power-of-2 size");
7979

80-
return static_cast<T>(x >> (s & (std::numeric_limits<T>::digits - 1))) |
81-
static_cast<T>(x << ((-s) & (std::numeric_limits<T>::digits - 1)));
80+
// Rotate by s mod the number of digits to avoid unnecessary rotations.
81+
//
82+
// A negative s represents a left rotation instead of a right rotation.
83+
// We compute it as an equivalent complementary right rotation by leveraging
84+
// its two's complement representation.
85+
//
86+
// For example, suppose we rotate a 3-bit number by -2.
87+
// In that case:
88+
// * s = 0b11111111111111111111111111111110
89+
// * n = 8
90+
// * r = (0b11111111111111111111111111111110 & 0b111) = 0b110
91+
//
92+
// Instead of rotating by 2 to the left, we rotate by 6 to the right, which
93+
// is equivalent.
94+
const int n = std::numeric_limits<T>::digits;
95+
const int r = s & (n - 1);
96+
97+
if (r == 0) {
98+
return x;
99+
} else {
100+
return (x >> r) | (x << (n - r));
101+
}
82102
}
83103

84104
template <class T>
@@ -88,8 +108,16 @@ template <class T>
88108
static_assert(IsPowerOf2(std::numeric_limits<T>::digits),
89109
"T must have a power-of-2 size");
90110

91-
return static_cast<T>(x << (s & (std::numeric_limits<T>::digits - 1))) |
92-
static_cast<T>(x >> ((-s) & (std::numeric_limits<T>::digits - 1)));
111+
// Rotate by s mod the number of digits to avoid unnecessary rotations.
112+
// See comment in RotateRight for a detailed explanation of the logic below.
113+
const int n = std::numeric_limits<T>::digits;
114+
const int r = s & (n - 1);
115+
116+
if (r == 0) {
117+
return x;
118+
} else {
119+
return (x << r) | (x >> (n - r));
120+
}
93121
}
94122

95123
ABSL_ATTRIBUTE_ALWAYS_INLINE ABSL_INTERNAL_CONSTEXPR_POPCOUNT inline int

0 commit comments

Comments
 (0)