Commit 06eaa4f3 authored by Tudor Bosman's avatar Tudor Bosman Committed by Jordan DeLong

Fix bug in Bits<T>::get / set

Summary:
Everything worked except for getting properly aligned full blocks
because 1ULL << 64 is invalid (the shift amount must be strictly less than
the value size in bits)

Test Plan: test added

Reviewed By: philipp@fb.com

FB internal diff: D657800
parent 9284f39b
...@@ -156,7 +156,12 @@ struct Bits { ...@@ -156,7 +156,12 @@ struct Bits {
// (bitStart < sizeof(T) * 8, bitStart + count <= sizeof(T) * 8) // (bitStart < sizeof(T) * 8, bitStart + count <= sizeof(T) * 8)
static UnderlyingType innerGet(const T* p, size_t bitStart, size_t count); static UnderlyingType innerGet(const T* p, size_t bitStart, size_t count);
static constexpr UnderlyingType zero = UnderlyingType(0);
static constexpr UnderlyingType one = UnderlyingType(1); static constexpr UnderlyingType one = UnderlyingType(1);
static constexpr UnderlyingType ones(size_t count) {
return count < bitsPerBlock ? (one << count) - 1 : ~zero;
}
}; };
template <class T, class Traits> template <class T, class Traits>
...@@ -180,8 +185,6 @@ template <class T, class Traits> ...@@ -180,8 +185,6 @@ template <class T, class Traits>
inline void Bits<T, Traits>::set(T* p, size_t bitStart, size_t count, inline void Bits<T, Traits>::set(T* p, size_t bitStart, size_t count,
UnderlyingType value) { UnderlyingType value) {
assert(count <= sizeof(UnderlyingType) * 8); assert(count <= sizeof(UnderlyingType) * 8);
assert(count == sizeof(UnderlyingType) ||
(value & ~((one << count) - 1)) == 0);
size_t idx = blockIndex(bitStart); size_t idx = blockIndex(bitStart);
size_t offset = bitOffset(bitStart); size_t offset = bitOffset(bitStart);
if (offset + count <= bitsPerBlock) { if (offset + count <= bitsPerBlock) {
...@@ -217,7 +220,7 @@ inline void Bits<T, Traits>::innerSet(T* p, size_t offset, size_t count, ...@@ -217,7 +220,7 @@ inline void Bits<T, Traits>::innerSet(T* p, size_t offset, size_t count,
UnderlyingType value) { UnderlyingType value) {
// Mask out bits and set new value // Mask out bits and set new value
UnderlyingType v = Traits::loadRMW(*p); UnderlyingType v = Traits::loadRMW(*p);
v &= ~(((one << count) - 1) << offset); v &= ~(ones(count) << offset);
v |= (value << offset); v |= (value << offset);
Traits::store(*p, v); Traits::store(*p, v);
} }
...@@ -225,7 +228,7 @@ inline void Bits<T, Traits>::innerSet(T* p, size_t offset, size_t count, ...@@ -225,7 +228,7 @@ inline void Bits<T, Traits>::innerSet(T* p, size_t offset, size_t count,
template <class T, class Traits> template <class T, class Traits>
inline auto Bits<T, Traits>::innerGet(const T* p, size_t offset, size_t count) inline auto Bits<T, Traits>::innerGet(const T* p, size_t offset, size_t count)
-> UnderlyingType { -> UnderlyingType {
return (Traits::load(*p) >> offset) & ((one << count) - 1); return (Traits::load(*p) >> offset) & ones(count);
} }
template <class T, class Traits> template <class T, class Traits>
......
...@@ -148,6 +148,7 @@ void runMultiBitTest64() { ...@@ -148,6 +148,7 @@ void runMultiBitTest64() {
auto load = detail::BitsTraits<T>::load; auto load = detail::BitsTraits<T>::load;
T buf[] = {0x123456789abcdef0, 0x13579bdf2468ace0}; T buf[] = {0x123456789abcdef0, 0x13579bdf2468ace0};
EXPECT_EQ(0x123456789abcdef0, load(Bits<T>::get(buf, 0, 64)));
EXPECT_EQ(0xf0, load(Bits<T>::get(buf, 0, 8))); EXPECT_EQ(0xf0, load(Bits<T>::get(buf, 0, 8)));
EXPECT_EQ(0x89abcdef, load(Bits<T>::get(buf, 4, 32))); EXPECT_EQ(0x89abcdef, load(Bits<T>::get(buf, 4, 32)));
EXPECT_EQ(0x189abcdef, load(Bits<T>::get(buf, 4, 33))); EXPECT_EQ(0x189abcdef, load(Bits<T>::get(buf, 4, 33)));
...@@ -156,6 +157,9 @@ void runMultiBitTest64() { ...@@ -156,6 +157,9 @@ void runMultiBitTest64() {
EXPECT_EQ(0xd5555555, load(Bits<T>::get(buf, 4, 32))); EXPECT_EQ(0xd5555555, load(Bits<T>::get(buf, 4, 32)));
EXPECT_EQ(0x1d5555555, load(Bits<T>::get(buf, 4, 33))); EXPECT_EQ(0x1d5555555, load(Bits<T>::get(buf, 4, 33)));
EXPECT_EQ(0xd55555550, load(Bits<T>::get(buf, 0, 36))); EXPECT_EQ(0xd55555550, load(Bits<T>::get(buf, 0, 36)));
Bits<T>::set(buf, 0, 64, 0x23456789abcdef01);
EXPECT_EQ(0x23456789abcdef01, load(Bits<T>::get(buf, 0, 64)));
} }
TEST(Bits, MultiBit64) { TEST(Bits, MultiBit64) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment