From 5fd6f1a84e84f99a79831ce6efbda9f51a9e1ac8 Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Wed, 30 Nov 2022 12:32:30 +0100 Subject: Add range checks for BitVector setBits and clearBits member functions, enabled when VESPA_BITVECTOR_RANGE_CHECK environment variable is set to "yes". --- .../vespa/searchlib/common/allocatedbitvector.cpp | 4 +-- searchlib/src/vespa/searchlib/common/bitvector.cpp | 26 ++++++++++++++- searchlib/src/vespa/searchlib/common/bitvector.h | 38 ++++++++++++++++++---- .../vespa/searchlib/common/growablebitvector.cpp | 2 +- .../vespa/searchlib/common/partialbitvector.cpp | 2 +- 5 files changed, 61 insertions(+), 11 deletions(-) (limited to 'searchlib') diff --git a/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp b/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp index 33f044845aa..2dd402a66f7 100644 --- a/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp +++ b/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp @@ -69,7 +69,7 @@ AllocatedBitVector::AllocatedBitVector(Index numberOfElements, Index capacityBit if (minCount/8 == numberOfElements/8) { static_cast(getStart())[numWords()-1] &= ~endBits(minCount); } - setBit(size()); // Guard bit + set_bit_no_range_check(size()); // Guard bit } updateCount(); } @@ -90,7 +90,7 @@ AllocatedBitVector::AllocatedBitVector(const BitVector & rhs, std::pair #include #include +#include #include #include @@ -42,6 +43,8 @@ namespace search { using vespalib::nbostream; +bool BitVector::_enable_range_check = false; + Alloc BitVector::allocatePaddedAndAligned(Index start, Index end, Index capacity, const Alloc* init_alloc) { @@ -79,7 +82,7 @@ void BitVector::clear() { memset(getActiveStart(), '\0', getActiveBytes()); - setBit(size()); // Guard bit + set_bit_no_range_check(size()); // Guard bit setTrueBits(0); } @@ -369,6 +372,15 @@ BitVector::create(const BitVector & rhs) return std::make_unique(rhs); } +void +BitVector::consider_enable_range_check() +{ + const char *env = getenv("VESPA_BITVECTOR_RANGE_CHECK"); + if (env != nullptr && strcmp(env, "yes") == 0) { + _enable_range_check = true; + } +} + MMappedBitVector::MMappedBitVector(Index numberOfElements, FastOS_FileInterface &file, int64_t offset, Index doccount) : BitVector() @@ -422,5 +434,17 @@ operator>>(nbostream &in, AllocatedBitVector &bv) return in; } +class ConsiderEnableRangeCheckCaller +{ +public: + ConsiderEnableRangeCheckCaller(); +}; + +ConsiderEnableRangeCheckCaller::ConsiderEnableRangeCheckCaller() +{ + BitVector::consider_enable_range_check(); +} + +ConsiderEnableRangeCheckCaller consider_enable_range_check_caller; } // namespace search diff --git a/searchlib/src/vespa/searchlib/common/bitvector.h b/searchlib/src/vespa/searchlib/common/bitvector.h index d3671d61ccf..1d289435f6d 100644 --- a/searchlib/src/vespa/searchlib/common/bitvector.h +++ b/searchlib/src/vespa/searchlib/common/bitvector.h @@ -8,6 +8,7 @@ #include #include #include +#include namespace vespalib { class nbostream; @@ -134,23 +135,38 @@ public: } void setSize(Index sz) { - setBit(sz); // Need to place the new stop sign first + set_bit_no_range_check(sz); // Need to place the new stop sign first std::atomic_thread_fence(std::memory_order_release); if (sz > _sz) { // Can only remove the old stopsign if it is ahead of the new. - clearBit(_sz); + clear_bit_no_range_check(_sz); } vespalib::atomic::store_ref_release(_sz, sz); } - void setBit(Index idx) { + void set_bit_no_range_check(Index idx) { store(_words[wordNum(idx)], _words[wordNum(idx)] | mask(idx)); } - void clearBit(Index idx) { + void clear_bit_no_range_check(Index idx) { store(_words[wordNum(idx)], _words[wordNum(idx)] & ~ mask(idx)); } - void flipBit(Index idx) { + void flip_bit_no_range_check(Index idx) { store(_words[wordNum(idx)], _words[wordNum(idx)] ^ mask(idx)); } + void range_check(Index idx) { + assert(!_enable_range_check || (idx >= _startOffset && idx < _sz)); + } + void setBit(Index idx) { + range_check(idx); + set_bit_no_range_check(idx); + } + void clearBit(Index idx) { + range_check(idx); + clear_bit_no_range_check(idx); + } + void flipBit(Index idx) { + range_check(idx); + flip_bit_no_range_check(idx); + } void andWith(const BitVector &right); void orWith(const BitVector &right); @@ -187,6 +203,14 @@ public: incNumBits(); } } + + void clear_bit_and_maintain_count_no_range_check(Index idx) { + if (testBit(idx)) { + clear_bit_no_range_check(idx); + decNumBits(); + } + } + /** * Clears a bit and maintains count of number of bits set. * @param idx @@ -251,6 +275,7 @@ public: static UP create(const BitVector & org, Index start, Index end); static UP create(Index numberOfElements); static UP create(const BitVector & rhs); + static void consider_enable_range_check(); protected: using Alloc = vespalib::alloc::Alloc; VESPA_DLL_LOCAL BitVector(void * buf, Index start, Index end); @@ -291,7 +316,7 @@ private: return (end >= start) ? (numWords(end) - wordNum(start)) : 0; } static Index invalidCount() { return std::numeric_limits::max(); } - void setGuardBit() { setBit(size()); } + void setGuardBit() { set_bit_no_range_check(size()); } void incNumBits() { if ( isValidCount() ) { _numTrueBits.store(_numTrueBits.load(std::memory_order_relaxed) + 1, std::memory_order_relaxed); @@ -360,6 +385,7 @@ private: Index _startOffset; // This is the official start Index _sz; // This is the official end. mutable std::atomic _numTrueBits; + static bool _enable_range_check; protected: friend vespalib::nbostream & diff --git a/searchlib/src/vespa/searchlib/common/growablebitvector.cpp b/searchlib/src/vespa/searchlib/common/growablebitvector.cpp index 5f971e21cd3..b0f8ff365ca 100644 --- a/searchlib/src/vespa/searchlib/common/growablebitvector.cpp +++ b/searchlib/src/vespa/searchlib/common/growablebitvector.cpp @@ -28,7 +28,7 @@ GrowableBitVector::grow(BitWord::Index newSize, BitWord::Index newCapacity) if (newCapacity != self.capacity()) { auto tbv = std::make_unique(newSize, newCapacity, self._alloc.get(), self.size(), &self._alloc); if (newSize > self.size()) { - tbv->clearBitAndMaintainCount(self.size()); // Clear old guard bit. + tbv->clear_bit_and_maintain_count_no_range_check(self.size()); // Clear old guard bit. } auto to_hold = std::make_unique(std::move(_stored)); _self.store(tbv.get(), std::memory_order_release); diff --git a/searchlib/src/vespa/searchlib/common/partialbitvector.cpp b/searchlib/src/vespa/searchlib/common/partialbitvector.cpp index 128a9ab8452..af4e2efd315 100644 --- a/searchlib/src/vespa/searchlib/common/partialbitvector.cpp +++ b/searchlib/src/vespa/searchlib/common/partialbitvector.cpp @@ -26,7 +26,7 @@ PartialBitVector::PartialBitVector(const BitVector & org, Index start, Index end if (org.size() < end) { clearInterval(org.size(), end); } - setBit(size()); + set_bit_no_range_check(size()); } PartialBitVector::~PartialBitVector() = default; -- cgit v1.2.3 From de067a4ecc33fbaca48fe8c7c8d42043f58f1d1b Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Wed, 30 Nov 2022 13:48:08 +0100 Subject: Include cstdlib instead of stdlib.h. --- searchlib/src/vespa/searchlib/common/bitvector.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'searchlib') diff --git a/searchlib/src/vespa/searchlib/common/bitvector.cpp b/searchlib/src/vespa/searchlib/common/bitvector.cpp index 9adcd27c85c..c068c78be23 100644 --- a/searchlib/src/vespa/searchlib/common/bitvector.cpp +++ b/searchlib/src/vespa/searchlib/common/bitvector.cpp @@ -9,8 +9,8 @@ #include #include #include -#include #include +#include #include LOG_SETUP(".searchlib.common.bitvector"); -- cgit v1.2.3 From 5db1c3a16cf17ad26b059c5a9ba5a14d9ee8a5e4 Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Wed, 30 Nov 2022 16:17:19 +0100 Subject: Check for "true" instead of "yes" when considering turning on range checks. --- searchlib/src/vespa/searchlib/common/bitvector.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'searchlib') diff --git a/searchlib/src/vespa/searchlib/common/bitvector.cpp b/searchlib/src/vespa/searchlib/common/bitvector.cpp index c068c78be23..47ca590c9bc 100644 --- a/searchlib/src/vespa/searchlib/common/bitvector.cpp +++ b/searchlib/src/vespa/searchlib/common/bitvector.cpp @@ -376,7 +376,7 @@ void BitVector::consider_enable_range_check() { const char *env = getenv("VESPA_BITVECTOR_RANGE_CHECK"); - if (env != nullptr && strcmp(env, "yes") == 0) { + if (env != nullptr && strcmp(env, "true") == 0) { _enable_range_check = true; } } -- cgit v1.2.3