Skip to content

Commit a3fe685

Browse files
evanjmeta-codesync[bot]
authored andcommitted
math.h BottomNBits: Fix integer underflow (facebook#14231)
Summary: When running make check on aarch64, hash_test reports an integer underflows: util/math.h:44:46: runtime error: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int' util/math.h:44:46: runtime error: signed integer overflow: -9223372036854775808 - 1 cannot be represented in type 'long long' util/math.h:44:46: runtime error: signed integer overflow: -9223372036854775808 - 1 cannot be represented in type 'long' The issue is when BottomNBits(int32 value, 31) does not use BMI2, it executes the following: return static_cast<T>(v & ((T{1} << nbits) - 1)); For int32_t, (1 << 31) is the minimum value, and -1 is an integer underflow. The fix is to cast T to an unsigned type and use that for the bit manipulation. I used Compiler Explorer to verify that this still compiles to the BZHI instruction mentioned in the comment with -march=x86-64-v3: https://godbolt.org/z/8bcTE8xbf To reproduce these errors on x86-64, disable the BMI code path: ``` USE_CLANG=1 PORTABLE=x86-64-v2 LDFLAGS=-fsanitize=undefined CXXFLAGS=-fsanitize=undefined make -j20 hash_test ``` Pull Request resolved: facebook#14231 Reviewed By: mszeszko-meta Differential Revision: D91353147 Pulled By: pdillinger fbshipit-source-id: 64cc191ccb9ecba20c260fab759e8881e30d2352
1 parent cc69112 commit a3fe685

2 files changed

Lines changed: 15 additions & 1 deletion

File tree

util/hash_test.cc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -615,6 +615,13 @@ static void test_BitOps() {
615615

616616
// BottomNBits
617617
{
618+
// build the mask the extremely slow way
619+
T bottom_n_mask = 0x00;
620+
for (int j = 0; j < i; j++) {
621+
bottom_n_mask <<= 1;
622+
bottom_n_mask |= 0x1;
623+
}
624+
618625
// An essentially full length value
619626
T x = everyOtherBit;
620627
if (i > 2) {
@@ -623,6 +630,11 @@ static void test_BitOps() {
623630
}
624631
auto a = BottomNBits(x, i);
625632
auto b = BottomNBits(~x, i);
633+
634+
// check that a and b match the expected values
635+
EXPECT_EQ(a, x & bottom_n_mask);
636+
EXPECT_EQ(b, (~x) & bottom_n_mask);
637+
626638
EXPECT_EQ(x | a, x);
627639
EXPECT_EQ(a | b, vm1);
628640
EXPECT_EQ(a & b, T{0});

util/math.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ inline T BottomNBits(T v, int nbits) {
4141
#endif
4242
// Newer compilers compile this down to bzhi on x86, but some older
4343
// ones don't, thus the need for the intrinsic above.
44-
return static_cast<T>(v & ((T{1} << nbits) - 1));
44+
using UnsignedT = std::make_unsigned_t<T>;
45+
UnsignedT mask = (static_cast<UnsignedT>(1) << nbits) - 1;
46+
return static_cast<T>(static_cast<UnsignedT>(v) & mask);
4547
}
4648

4749
// Fast implementation of floor(log2(v)). Undefined for 0 or negative

0 commit comments

Comments
 (0)