Remove UB from indeterminate value handling
Move SIMD operations on potentially-indeterminate Node16::index bytes into file-level assembly, where loading and operating on indeterminate values is well-defined (unlike C++). Restructure scalar fallback loops to iterate [0, numChildren) instead of [0, kMaxNodes). Fix TrivialSpan construction from indeterminate pointers in check::Job::init and insertPointWritesOrSorted to only construct when end.len > 0. Add MSan toolchain to the debug CI build to catch these issues going forward.
This commit is contained in:
+29
-81
@@ -28,6 +28,7 @@ limitations under the License.
|
||||
#include "Internal.h"
|
||||
#include "LongestCommonPrefix.h"
|
||||
#include "Metrics.h"
|
||||
#include "simd.h"
|
||||
|
||||
#include <algorithm>
|
||||
#include <bit>
|
||||
@@ -910,34 +911,11 @@ int getNodeIndexExists(Node3 *self, uint8_t index) {
|
||||
|
||||
int getNodeIndex(Node16 *self, uint8_t index) {
|
||||
|
||||
#ifdef HAS_AVX
|
||||
// Based on https://www.the-paper-trail.org/post/art-paper-notes/
|
||||
|
||||
// key_vec is 16 repeated copies of the searched-for byte, one for every
|
||||
// possible position in child_keys that needs to be searched.
|
||||
__m128i key_vec = _mm_set1_epi8(index);
|
||||
|
||||
// Compare all child_keys to 'index' in parallel. Don't worry if some of the
|
||||
// keys aren't valid, we'll mask the results to only consider the valid ones
|
||||
// below.
|
||||
__m128i indices;
|
||||
memcpy(&indices, self->index, Node16::kMaxNodes);
|
||||
__m128i results = _mm_cmpeq_epi8(key_vec, indices);
|
||||
|
||||
// Build a mask to select only the first node->num_children values from the
|
||||
// comparison (because the other values are meaningless)
|
||||
uint32_t mask = (1 << self->numChildren) - 1;
|
||||
|
||||
// Change the results of the comparison into a bitfield, masking off any
|
||||
// invalid comparisons.
|
||||
uint32_t bitfield = _mm_movemask_epi8(results) & mask;
|
||||
|
||||
// No match if there are no '1's in the bitfield.
|
||||
#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
|
||||
uint32_t bitfield =
|
||||
find_eq_16(self->index, index) & ((1 << self->numChildren) - 1);
|
||||
if (bitfield == 0)
|
||||
return -1;
|
||||
|
||||
// Find the index of the first '1' in the bitfield by counting the leading
|
||||
// zeros.
|
||||
return std::countr_zero(bitfield);
|
||||
#elif defined(HAS_ARM_NEON)
|
||||
// Based on
|
||||
@@ -970,13 +948,9 @@ int getNodeIndex(Node16 *self, uint8_t index) {
|
||||
|
||||
int getNodeIndexExists(Node16 *self, uint8_t index) {
|
||||
|
||||
#ifdef HAS_AVX
|
||||
__m128i key_vec = _mm_set1_epi8(index);
|
||||
__m128i indices;
|
||||
memcpy(&indices, self->index, Node16::kMaxNodes);
|
||||
__m128i results = _mm_cmpeq_epi8(key_vec, indices);
|
||||
uint32_t mask = (1 << self->numChildren) - 1;
|
||||
uint32_t bitfield = _mm_movemask_epi8(results) & mask;
|
||||
#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
|
||||
uint32_t bitfield =
|
||||
find_eq_16(self->index, index) & ((1 << self->numChildren) - 1);
|
||||
assume(bitfield != 0);
|
||||
return std::countr_zero(bitfield);
|
||||
#elif defined(HAS_ARM_NEON)
|
||||
@@ -1266,13 +1240,9 @@ TaggedNodePointer getChildGeq(Node16 *self, int child) {
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
#ifdef HAS_AVX
|
||||
__m128i key_vec = _mm_set1_epi8(child);
|
||||
__m128i indices;
|
||||
memcpy(&indices, self->index, Node16::kMaxNodes);
|
||||
__m128i results = _mm_cmpeq_epi8(key_vec, _mm_min_epu8(key_vec, indices));
|
||||
int mask = (1 << self->numChildren) - 1;
|
||||
uint32_t bitfield = _mm_movemask_epi8(results) & mask;
|
||||
#if defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
|
||||
uint32_t bitfield =
|
||||
find_ge_16(self->index, child) & ((1 << self->numChildren) - 1);
|
||||
return bitfield == 0 ? nullptr : self->children[std::countr_zero(bitfield)];
|
||||
#elif defined(HAS_ARM_NEON)
|
||||
uint8x16_t indices;
|
||||
@@ -2130,13 +2100,9 @@ bool scan16(const InternalVersionT *vs, const uint8_t *is, int begin, int end,
|
||||
|
||||
return !(compared & mask);
|
||||
|
||||
#elif defined(HAS_AVX)
|
||||
#elif defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
|
||||
|
||||
__m128i indices;
|
||||
memcpy(&indices, is, 16);
|
||||
indices = _mm_sub_epi8(indices, _mm_set1_epi8(begin));
|
||||
uint32_t mask = ~_mm_movemask_epi8(_mm_cmpeq_epi8(
|
||||
indices, _mm_max_epu8(indices, _mm_set1_epi8(end - begin))));
|
||||
uint32_t mask = mask_in_range_16(is, begin, end);
|
||||
|
||||
uint32_t compared = 0;
|
||||
if constexpr (kAVX512) {
|
||||
@@ -2153,12 +2119,14 @@ bool scan16(const InternalVersionT *vs, const uint8_t *is, int begin, int end,
|
||||
auto inBounds = [&](unsigned c) { return c - shiftAmount < shiftUpperBound; };
|
||||
|
||||
uint32_t compared = 0;
|
||||
for (int i = 0; i < 16; ++i) {
|
||||
compared |= (vs[i] > readVersion) << i;
|
||||
}
|
||||
uint32_t mask = 0;
|
||||
for (int i = 0; i < 16; ++i) {
|
||||
mask |= inBounds(is[i]) << i;
|
||||
if (vs[i] > readVersion) {
|
||||
compared |= 1u << i;
|
||||
if (inBounds(is[i])) {
|
||||
mask |= 1u << i;
|
||||
}
|
||||
}
|
||||
}
|
||||
return !(compared & mask);
|
||||
|
||||
@@ -2250,17 +2218,9 @@ bool checkMaxBetweenExclusiveImpl(Node3 *n, int begin, int end,
|
||||
auto inBounds = [&](unsigned c) { return c - shiftAmount < shiftUpperBound; };
|
||||
|
||||
uint32_t mask = 0;
|
||||
for (int i = 0; i < Node3::kMaxNodes; ++i) {
|
||||
for (int i = 0; i < self->numChildren; ++i) {
|
||||
mask |= inBounds(self->index[i]) << i;
|
||||
}
|
||||
mask &= (1 << self->numChildren) - 1;
|
||||
#ifdef __aarch64__
|
||||
// The bits surviving the mask above don't derive from uninitialized slots,
|
||||
// but clang 21+ on aarch64 lowers inBounds through flags+csel, which
|
||||
// memcheck models imprecisely, tainting bits the mask provably clears.
|
||||
// https://git.weaselab.dev/weaselab/conflict-set/issues/39
|
||||
VALGRIND_MAKE_MEM_DEFINED(&mask, sizeof(mask));
|
||||
#endif
|
||||
if (!mask) {
|
||||
return true;
|
||||
}
|
||||
@@ -2268,17 +2228,11 @@ bool checkMaxBetweenExclusiveImpl(Node3 *n, int begin, int end,
|
||||
const bool firstRangeOk =
|
||||
!child->entryPresent || child->entry.rangeVersion <= readVersion;
|
||||
uint32_t compared = 0;
|
||||
for (int i = 0; i < Node3::kMaxNodes; ++i) {
|
||||
for (int i = 0; i < self->numChildren; ++i) {
|
||||
compared |= (self->childMaxVersion[i] > readVersion) << i;
|
||||
}
|
||||
|
||||
uint32_t compared_masked = compared & mask;
|
||||
#ifdef __aarch64__
|
||||
// Same imprecise csel modeling as above.
|
||||
// https://git.weaselab.dev/weaselab/conflict-set/issues/39
|
||||
VALGRIND_MAKE_MEM_DEFINED(&compared_masked, sizeof(compared_masked));
|
||||
#endif
|
||||
return !compared_masked && firstRangeOk;
|
||||
return !(compared & mask) && firstRangeOk;
|
||||
}
|
||||
|
||||
template <bool kAVX512>
|
||||
@@ -2344,15 +2298,10 @@ bool checkMaxBetweenExclusiveImpl(Node16 *n, int begin, int end,
|
||||
|
||||
return !(compared & mask) && firstRangeOk;
|
||||
|
||||
#elif defined(HAS_AVX)
|
||||
#elif defined(__x86_64__) && !defined(USE_SIMD_FALLBACK)
|
||||
|
||||
__m128i indices;
|
||||
memcpy(&indices, self->index, 16);
|
||||
indices = _mm_sub_epi8(indices, _mm_set1_epi8(begin));
|
||||
uint32_t mask =
|
||||
0xffff & ~_mm_movemask_epi8(_mm_cmpeq_epi8(
|
||||
indices, _mm_max_epu8(indices, _mm_set1_epi8(end - begin))));
|
||||
mask &= (1 << self->numChildren) - 1;
|
||||
uint32_t mask = mask_in_range_16(self->index, begin, end) &
|
||||
((1 << self->numChildren) - 1);
|
||||
if (!mask) {
|
||||
return true;
|
||||
}
|
||||
@@ -2375,10 +2324,9 @@ bool checkMaxBetweenExclusiveImpl(Node16 *n, int begin, int end,
|
||||
auto inBounds = [&](unsigned c) { return c - shiftAmount < shiftUpperBound; };
|
||||
|
||||
uint32_t mask = 0;
|
||||
for (int i = 0; i < 16; ++i) {
|
||||
for (int i = 0; i < self->numChildren; ++i) {
|
||||
mask |= inBounds(self->index[i]) << i;
|
||||
}
|
||||
mask &= (1 << self->numChildren) - 1;
|
||||
if (!mask) {
|
||||
return true;
|
||||
}
|
||||
@@ -2386,7 +2334,7 @@ bool checkMaxBetweenExclusiveImpl(Node16 *n, int begin, int end,
|
||||
const bool firstRangeOk =
|
||||
!child->entryPresent || child->entry.rangeVersion <= readVersion;
|
||||
uint32_t compared = 0;
|
||||
for (int i = 0; i < 16; ++i) {
|
||||
for (int i = 0; i < self->numChildren; ++i) {
|
||||
compared |= (self->childMaxVersion[i] > readVersion) << i;
|
||||
}
|
||||
return !(compared & mask) && firstRangeOk;
|
||||
@@ -3864,17 +3812,17 @@ PRESERVE_NONE void right_side_iter(Job *job, Context *context) {
|
||||
void Job::init(const ConflictSet::ReadRange *read, ConflictSet::Result *result,
|
||||
Node *root, int64_t oldestVersionFullPrecision) {
|
||||
auto begin = TrivialSpan(read->begin.p, read->begin.len);
|
||||
auto end = TrivialSpan(read->end.p, read->end.len);
|
||||
if (read->readVersion < oldestVersionFullPrecision) [[unlikely]] {
|
||||
*result = ConflictSet::TooOld;
|
||||
continuation = complete;
|
||||
} else if (end.size() == 0) {
|
||||
} else if (read->end.len == 0) {
|
||||
this->begin = begin;
|
||||
this->n = root;
|
||||
this->readVersion = InternalVersionT(read->readVersion);
|
||||
this->result = result;
|
||||
continuation = check::point_read_state_machine::begin;
|
||||
} else {
|
||||
auto end = TrivialSpan(read->end.p, read->end.len);
|
||||
this->begin = begin;
|
||||
this->end = end;
|
||||
this->n = root;
|
||||
@@ -5046,8 +4994,8 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
|
||||
for (int i = 0; i < count; ++i) {
|
||||
const auto &w = writes[i];
|
||||
auto begin = TrivialSpan(w.begin.p, w.begin.len);
|
||||
auto end = TrivialSpan(w.end.p, w.end.len);
|
||||
if (w.end.len > 0) {
|
||||
auto end = TrivialSpan(w.end.p, w.end.len);
|
||||
addWriteRange(rootParent->children[0], begin, end,
|
||||
InternalVersionT(writeVersion), &writeContext);
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user