Merge pull request #27192 from vespa-engine/balder/remove-assert-for-btreeiterator-setNode

Balder/remove assert for btreeiterator set node

12 files changed, 85 insertions, 68 deletions

diff --git a/vespalib/src/apps/vespa-stress-and-validate-memory/stress_and_validate_memory.cpp b/vespalib/src/apps/vespa-stress-and-validate-memory/stress_and_validate_memory.cpp
index a7a227e45b3..e31f5e6413e 100644
--- a/vespalib/src/apps/vespa-stress-and-validate-memory/stress_and_validate_memory.cpp
+++ b/vespalib/src/apps/vespa-stress-and-validate-memory/stress_and_validate_memory.cpp
@@ -14,6 +14,7 @@
 std::atomic<bool> stopped = false;
 std::mutex log_mutex;
 using namespace vespalib;
+using vespalib::alloc::PtrAndSize;
 
 const char * description =
         "Runs stress test of memory by slowly growing a heap filled with 0.\n"
@@ -121,7 +122,6 @@ public:
     size_t make_and_load_alloc_per_thread();
     void random_write(unsigned int *seed);
 private:
-    using PtrAndSize = std::pair<void *, size_t>;
     const Config           & _cfg;
     mutable std::mutex       _mutex;
     alloc::MmapFileAllocator _allocator;
@@ -153,7 +153,7 @@ FileBackedMemory::make_and_load_alloc_per_thread() {
         std::lock_guard guard(_mutex);
         alloc = _allocator.alloc(cfg().alloc_size());
     }
-    memset(alloc.first, 0, cfg().alloc_size());
+    memset(alloc.get(), 0, cfg().alloc_size());
     std::lock_guard guard(_mutex);
     _allocations.push_back(std::move(alloc));
     return 1;
@@ -166,7 +166,7 @@ FileBackedMemory::random_write(unsigned int *seed) {
         std::lock_guard guard(_mutex);
         ptrAndSize = _allocations[rand_r(seed) % _allocations.size()];
     }
-    memset(ptrAndSize.first, rand_r(seed)%256, ptrAndSize.second);
+    memset(ptrAndSize.get(), rand_r(seed)%256, ptrAndSize.size());
 }
 
 void
diff --git a/vespalib/src/tests/util/mmap_file_allocator/mmap_file_allocator_test.cpp b/vespalib/src/tests/util/mmap_file_allocator/mmap_file_allocator_test.cpp
index 545733a1ebd..ef16998902e 100644
--- a/vespalib/src/tests/util/mmap_file_allocator/mmap_file_allocator_test.cpp
+++ b/vespalib/src/tests/util/mmap_file_allocator/mmap_file_allocator_test.cpp
@@ -6,6 +6,7 @@
 
 using vespalib::alloc::MemoryAllocator;
 using vespalib::alloc::MmapFileAllocator;
+using vespalib::alloc::PtrAndSize;
 
 namespace {
 
@@ -18,10 +19,10 @@ struct MyAlloc
     void*                  data;
     size_t                 size;
 
-    MyAlloc(MemoryAllocator& allocator_in, MemoryAllocator::PtrAndSize buf)
+    MyAlloc(MemoryAllocator& allocator_in, PtrAndSize buf)
         : allocator(allocator_in),
-          data(buf.first),
-          size(buf.second)
+          data(buf.get()),
+          size(buf.size())
     {
     }
 
@@ -30,7 +31,7 @@ struct MyAlloc
         allocator.free(data, size);
     }
 
-    MemoryAllocator::PtrAndSize asPair() const noexcept { return std::make_pair(data, size); }
+    PtrAndSize asPair() const noexcept { return PtrAndSize(data, size); }
 };
 
 }
diff --git a/vespalib/src/vespa/vespalib/btree/btreeiterator.h b/vespalib/src/vespa/vespalib/btree/btreeiterator.h
index 7ab28c9b5c9..01f0ad3c2d3 100644
--- a/vespalib/src/vespa/vespalib/btree/btreeiterator.h
+++ b/vespalib/src/vespa/vespalib/btree/btreeiterator.h
@@ -48,13 +48,12 @@ class NodeElement
     static_assert((NodeType::maxSlots() + 1) < (1ul << IDX_BITS), "IDX can be out of bounds above 127");
 public:
     NodeElement() noexcept : _nodeAndIdx(0ul) { }
-    NodeElement(const NodeType *node, uint32_t idx) noexcept : _nodeAndIdx(uint64_t(node) | uint64_t(idx) << IDX_SHIFT) {
-        assert((uint64_t(node) & ~NODE_MASK) == 0ul);
-    }
+    NodeElement(const NodeType *node, uint32_t idx) noexcept
+        : _nodeAndIdx(uint64_t(node) | uint64_t(idx) << IDX_SHIFT)
+    { }
 
     void invalidate() noexcept { _nodeAndIdx = 0; }
     void setNode(const NodeType *node) noexcept {
-        assert((uint64_t(node) & ~NODE_MASK) == 0ul);
         _nodeAndIdx = (_nodeAndIdx & ~NODE_MASK) | uint64_t(node);
     }
     const NodeType * getNode() const noexcept { return reinterpret_cast<const NodeType *>(_nodeAndIdx & NODE_MASK); }
@@ -66,7 +65,6 @@ public:
     void decIdx() noexcept { _nodeAndIdx -= IDX_ONE; }
 
     void setNodeAndIdx(const NodeType *node, uint32_t idx) noexcept {
-        assert((uint64_t(node) & ~NODE_MASK) == 0ul);
         _nodeAndIdx = uint64_t(node) | uint64_t(idx) << IDX_SHIFT;
     }
 
diff --git a/vespalib/src/vespa/vespalib/stllike/allocator.h b/vespalib/src/vespa/vespalib/stllike/allocator.h
index b34533740a2..f35cfce8359 100644
--- a/vespalib/src/vespa/vespalib/stllike/allocator.h
+++ b/vespalib/src/vespa/vespalib/stllike/allocator.h
@@ -17,7 +17,7 @@ public:
     allocator_large() noexcept : _allocator(alloc::MemoryAllocator::select_allocator()) {}
     using value_type = T;
     constexpr T * allocate(std::size_t n) {
-        return static_cast<T *>(_allocator->alloc(n*sizeof(T)).first);
+        return static_cast<T *>(_allocator->alloc(n*sizeof(T)).get());
     }
     void deallocate(T * p, std::size_t n) {
         _allocator->free(p, n*sizeof(T));
diff --git a/vespalib/src/vespa/vespalib/test/memory_allocator_observer.cpp b/vespalib/src/vespa/vespalib/test/memory_allocator_observer.cpp
index ba23970f0ea..43817e63948 100644
--- a/vespalib/src/vespa/vespalib/test/memory_allocator_observer.cpp
+++ b/vespalib/src/vespa/vespalib/test/memory_allocator_observer.cpp
@@ -20,7 +20,7 @@ MemoryAllocatorObserver::MemoryAllocatorObserver(Stats &stats)
 }
 MemoryAllocatorObserver::~MemoryAllocatorObserver() = default;
 
-MemoryAllocatorObserver::PtrAndSize
+PtrAndSize
 MemoryAllocatorObserver::alloc(size_t sz) const
 {
     ++_stats.alloc_cnt;
diff --git a/vespalib/src/vespa/vespalib/util/alloc.cpp b/vespalib/src/vespa/vespalib/util/alloc.cpp
index 83c8a7de7e2..cb9a4b688b3 100644
--- a/vespalib/src/vespa/vespalib/util/alloc.cpp
+++ b/vespalib/src/vespa/vespalib/util/alloc.cpp
@@ -284,12 +284,12 @@ AutoAllocator::getAllocator(size_t mmapLimit, size_t alignment) {
     return getAutoAllocator(availableAutoAllocators().first, mmapLimit, alignment);
 }
 
-MemoryAllocator::PtrAndSize
+PtrAndSize
 HeapAllocator::alloc(size_t sz) const {
     return salloc(sz);
 }
 
-MemoryAllocator::PtrAndSize
+PtrAndSize
 HeapAllocator::salloc(size_t sz) {
     return PtrAndSize((sz > 0) ? malloc(sz) : nullptr, sz);
 }
@@ -299,10 +299,10 @@ void HeapAllocator::free(PtrAndSize alloc) const {
 }
 
 void HeapAllocator::sfree(PtrAndSize alloc) {
-    if (alloc.first) { ::free(alloc.first); }
+    if (alloc.get()) { ::free(alloc.get()); }
 }
 
-MemoryAllocator::PtrAndSize
+PtrAndSize
 AlignedHeapAllocator::alloc(size_t sz) const {
     if (!sz) { return PtrAndSize(nullptr, 0); }
     void* ptr;
@@ -318,12 +318,12 @@ MMapAllocator::resize_inplace(PtrAndSize current, size_t newSize) const {
     return sresize_inplace(current, newSize);
 }
 
-MemoryAllocator::PtrAndSize
+PtrAndSize
 MMapAllocator::alloc(size_t sz) const {
     return salloc(sz, nullptr);
 }
 
-MemoryAllocator::PtrAndSize
+PtrAndSize
 MMapAllocator::salloc(size_t sz, void * wantedAddress)
 {
     void * buf(nullptr);
@@ -382,23 +382,23 @@ MMapAllocator::salloc(size_t sz, void * wantedAddress)
 size_t
 MMapAllocator::sresize_inplace(PtrAndSize current, size_t newSize) {
     newSize = round_up_to_page_size(newSize);
-    if (newSize > current.second) {
+    if (newSize > current.size()) {
         return extend_inplace(current, newSize);
-    } else if (newSize < current.second) {
+    } else if (newSize < current.size()) {
         return shrink_inplace(current, newSize);
     } else {
-        return current.second;
+        return current.size();
     }
 }
 
 size_t
 MMapAllocator::extend_inplace(PtrAndSize current, size_t newSize) {
-    if (current.second == 0u) {
+    if (current.size() == 0u) {
         return 0u;
     }
-    PtrAndSize got = MMapAllocator::salloc(newSize - current.second, static_cast<char *>(current.first)+current.second);
-    if ((static_cast<const char *>(current.first) + current.second) == static_cast<const char *>(got.first)) {
-        return current.second + got.second;
+    PtrAndSize got = MMapAllocator::salloc(newSize - current.size(), static_cast<char *>(current.get())+current.size());
+    if ((static_cast<const char *>(current.get()) + current.size()) == static_cast<const char *>(got.get())) {
+        return current.size() + got.size();
     } else {
         MMapAllocator::sfree(got);
         return 0;
@@ -407,7 +407,7 @@ MMapAllocator::extend_inplace(PtrAndSize current, size_t newSize) {
 
 size_t
 MMapAllocator::shrink_inplace(PtrAndSize current, size_t newSize) {
-    PtrAndSize toUnmap(static_cast<char *>(current.first)+newSize, current.second - newSize);
+    PtrAndSize toUnmap(static_cast<char *>(current.get())+newSize, current.size() - newSize);
     sfree(toUnmap);
     return newSize;
 }
@@ -418,27 +418,27 @@ void MMapAllocator::free(PtrAndSize alloc) const {
 
 void MMapAllocator::sfree(PtrAndSize alloc)
 {
-    if (alloc.first != nullptr) {
-        int madvise_retval = madvise(alloc.first, alloc.second, MADV_DONTNEED);
+    if (alloc.get() != nullptr) {
+        int madvise_retval = madvise(alloc.get(), alloc.size(), MADV_DONTNEED);
         if (madvise_retval != 0) {
             std::error_code ec(errno, std::system_category());
             if (errno == EINVAL) {
-                LOG(debug, "madvise(%p, %lx)=%d, errno=%s", alloc.first, alloc.second, madvise_retval, ec.message().c_str());
+                LOG(debug, "madvise(%p, %lx)=%d, errno=%s", alloc.get(), alloc.size(), madvise_retval, ec.message().c_str());
             } else {
-                LOG(warning, "madvise(%p, %lx)=%d, errno=%s", alloc.first, alloc.second, madvise_retval, ec.message().c_str());
+                LOG(warning, "madvise(%p, %lx)=%d, errno=%s", alloc.get(), alloc.size(), madvise_retval, ec.message().c_str());
             }
         }
-        int munmap_retval = munmap(alloc.first, alloc.second);
+        int munmap_retval = munmap(alloc.get(), alloc.size());
         if (munmap_retval != 0) {
             std::error_code ec(errno, std::system_category());
-            LOG(warning, "munmap(%p, %lx)=%d, errno=%s", alloc.first, alloc.second, munmap_retval, ec.message().c_str());
+            LOG(warning, "munmap(%p, %lx)=%d, errno=%s", alloc.get(), alloc.size(), munmap_retval, ec.message().c_str());
             abort();
         }
-        if (alloc.second >= _G_MMapLogLimit) {
+        if (alloc.size() >= _G_MMapLogLimit) {
             std::lock_guard guard(_G_lock);
-            MMapInfo info = _G_HugeMappings[alloc.first];
-            assert(alloc.second == info._sz);
-            _G_HugeMappings.erase(alloc.first);
+            MMapInfo info = _G_HugeMappings[alloc.get()];
+            assert(alloc.size() == info._sz);
+            _G_HugeMappings.erase(alloc.get());
             LOG(info, "munmap %ld of size %ld", info._id, info._sz);
             LOG(info, "%ld mappings of accumulated size %ld", _G_HugeMappings.size(), sum(_G_HugeMappings));
         }
@@ -447,7 +447,7 @@ void MMapAllocator::sfree(PtrAndSize alloc)
 
 size_t
 AutoAllocator::resize_inplace(PtrAndSize current, size_t newSize) const {
-    if (useMMap(current.second) && useMMap(newSize)) {
+    if (useMMap(current.size()) && useMMap(newSize)) {
         newSize = roundUpToHugePages(newSize);
         return MMapAllocator::sresize_inplace(current, newSize);
     } else {
@@ -455,7 +455,7 @@ AutoAllocator::resize_inplace(PtrAndSize current, size_t newSize) const {
     }
 }
 
-MMapAllocator::PtrAndSize
+PtrAndSize
 AutoAllocator::alloc(size_t sz) const {
     if ( ! useMMap(sz)) {
         if (_alignment == 0) {
@@ -471,7 +471,7 @@ AutoAllocator::alloc(size_t sz) const {
 
 void
 AutoAllocator::free(PtrAndSize alloc) const {
-    if ( ! isMMapped(alloc.second)) {
+    if ( ! isMMapped(alloc.size())) {
         return HeapAllocator::sfree(alloc);
     } else {
         return MMapAllocator::sfree(alloc);
@@ -513,7 +513,7 @@ Alloc::resize_inplace(size_t newSize)
     }
     size_t extendedSize = _allocator->resize_inplace(_alloc, newSize);
     if (extendedSize >= newSize) {
-        _alloc.second = extendedSize;
+        _alloc = PtrAndSize(_alloc.get(), extendedSize);
         return true;
     }
     return false;
@@ -571,6 +571,14 @@ Alloc::alloc_with_allocator(const MemoryAllocator* allocator) noexcept
     return Alloc(allocator);
 }
 
+PtrAndSize::PtrAndSize(void * ptr, size_t sz) noexcept
+    : _ptr(ptr), _sz(sz)
+{
+    constexpr uint8_t MAX_PTR_BITS = 57;
+    constexpr uint64_t MAX_PTR = 1ul << MAX_PTR_BITS;
+    assert((uint64_t(ptr) + sz) < MAX_PTR);
+}
+
 }
 
 }
diff --git a/vespalib/src/vespa/vespalib/util/alloc.h b/vespalib/src/vespa/vespalib/util/alloc.h
index 4066894b4e3..b78c10dd381 100644
--- a/vespalib/src/vespa/vespalib/util/alloc.h
+++ b/vespalib/src/vespa/vespalib/util/alloc.h
@@ -15,14 +15,12 @@ namespace vespalib::alloc {
 **/
 class Alloc
 {
-private:
-    using PtrAndSize = std::pair<void *, size_t>;
 public:
-    size_t size() const { return _alloc.second; }
-    void * get() { return _alloc.first; }
-    const void * get() const { return _alloc.first; }
-    void * operator -> () { return _alloc.first; }
-    const void * operator -> () const { return _alloc.first; }
+    size_t size() const noexcept { return _alloc.size(); }
+    void * get() noexcept { return _alloc.get(); }
+    const void * get() const noexcept { return _alloc.get(); }
+    void * operator -> () noexcept { return get(); }
+    const void * operator -> () const noexcept { return get(); }
     /*
      * If possible the allocations will be resized. If it was possible it will return true
      * And you have an area that can be accessed up to the new size.
@@ -42,7 +40,7 @@ public:
     }
     Alloc & operator=(Alloc && rhs) noexcept {
         if (this != & rhs) {
-            if (_alloc.first != nullptr) {
+            if (_alloc.get() != nullptr) {
                 _allocator->free(_alloc);
             }
             _alloc = rhs._alloc;
@@ -53,9 +51,9 @@ public:
     }
     Alloc() noexcept : _alloc(nullptr, 0), _allocator(nullptr) { }
     ~Alloc() {
-        if (_alloc.first != nullptr) {
+        if (_alloc.get() != nullptr) {
             _allocator->free(_alloc);
-            _alloc.first = nullptr;
+            _alloc = PtrAndSize();
         }
     }
     void swap(Alloc & rhs) noexcept {
@@ -63,10 +61,9 @@ public:
         std::swap(_allocator, rhs._allocator);
     }
     void reset() {
-        if (_alloc.first != nullptr) {
+        if (_alloc.get() != nullptr) {
             _allocator->free(_alloc);
-            _alloc.first = nullptr;
-            _alloc.second = 0u;
+            _alloc = PtrAndSize();
         }
     }
     Alloc create(size_t sz) const noexcept {
@@ -96,8 +93,7 @@ private:
           _allocator(allocator)
     { }
     void clear() {
-        _alloc.first = nullptr;
-        _alloc.second = 0;
+        _alloc = PtrAndSize();
         _allocator = nullptr;
     }
     PtrAndSize              _alloc;
diff --git a/vespalib/src/vespa/vespalib/util/memory_allocator.h b/vespalib/src/vespa/vespalib/util/memory_allocator.h
index 2bcd7e5889d..e9a494f3e6f 100644
--- a/vespalib/src/vespa/vespalib/util/memory_allocator.h
+++ b/vespalib/src/vespa/vespalib/util/memory_allocator.h
@@ -8,6 +8,17 @@
 
 namespace vespalib::alloc {
 
+class PtrAndSize {
+public:
+    PtrAndSize() noexcept : _ptr(nullptr), _sz(0ul) {}
+    PtrAndSize(void * ptr, size_t sz) noexcept;
+    void * get() const noexcept { return _ptr; }
+    size_t size() const noexcept { return _sz; }
+private:
+    void * _ptr;
+    size_t _sz;
+};
+
 /*
  * Abstract base class for allocating memory at a low level.
  */
@@ -15,7 +26,6 @@ class MemoryAllocator {
 public:
     static constexpr size_t PAGE_SIZE = 4_Ki;
     static constexpr size_t HUGEPAGE_SIZE = 2_Mi;
-    using PtrAndSize = std::pair<void *, size_t>;
     MemoryAllocator(const MemoryAllocator &) = delete;
     MemoryAllocator & operator = (const MemoryAllocator &) = delete;
     MemoryAllocator() = default;
diff --git a/vespalib/src/vespa/vespalib/util/mmap_file_allocator.cpp b/vespalib/src/vespa/vespalib/util/mmap_file_allocator.cpp
index 8c89f6745e4..929b926c03e 100644
--- a/vespalib/src/vespa/vespalib/util/mmap_file_allocator.cpp
+++ b/vespalib/src/vespa/vespalib/util/mmap_file_allocator.cpp
@@ -42,7 +42,7 @@ MmapFileAllocator::alloc_area(size_t sz) const
     return offset;
 }
 
-MmapFileAllocator::PtrAndSize
+PtrAndSize
 MmapFileAllocator::alloc(size_t sz) const
 {
     if (sz == 0) {
@@ -72,23 +72,23 @@ MmapFileAllocator::alloc(size_t sz) const
 void
 MmapFileAllocator::free(PtrAndSize alloc) const
 {
-    if (alloc.second == 0) {
-        assert(alloc.first == nullptr);
+    if (alloc.size() == 0) {
+        assert(alloc.get() == nullptr);
         return; // empty allocation
     }
-    assert(alloc.first != nullptr);
+    assert(alloc.get() != nullptr);
     // Check that matching allocation is registered
-    auto itr = _allocations.find(alloc.first);
+    auto itr = _allocations.find(alloc.get());
     assert(itr != _allocations.end());
-    assert(itr->first == alloc.first);
-    assert(itr->second.size == alloc.second);
+    assert(itr->first == alloc.get());
+    assert(itr->second.size == alloc.size());
     auto offset = itr->second.offset;
     _allocations.erase(itr);
-    int retval = madvise(alloc.first, alloc.second, MADV_DONTNEED);
+    int retval = madvise(alloc.get(), alloc.size(), MADV_DONTNEED);
     assert(retval == 0);
-    retval = munmap(alloc.first, alloc.second);
+    retval = munmap(alloc.get(), alloc.size());
     assert(retval == 0);
-    _freelist.free(offset, alloc.second);
+    _freelist.free(offset, alloc.size());
 }
 
 size_t
diff --git a/vespamalloc/src/vespamalloc/malloc/common.h b/vespamalloc/src/vespamalloc/malloc/common.h
index 58e05878f64..501b45cd067 100644
--- a/vespamalloc/src/vespamalloc/malloc/common.h
+++ b/vespamalloc/src/vespamalloc/malloc/common.h
@@ -59,6 +59,8 @@ using OSMemory = MmapMemory;
 using SizeClassT = int;
 
 constexpr size_t ALWAYS_REUSE_LIMIT = 0x100000ul;
+constexpr uint8_t MAX_PTR_BITS = 57;  // Maximum number of bits a pointer can use (Intel IceLake)
+constexpr uint64_t MAX_PTR = 1ul << MAX_PTR_BITS;
 
 inline constexpr int
 msbIdx(uint64_t v) {
diff --git a/vespamalloc/src/vespamalloc/malloc/threadproxy.cpp b/vespamalloc/src/vespamalloc/malloc/threadproxy.cpp
index 02eb624ee64..4a02d599b63 100644
--- a/vespamalloc/src/vespamalloc/malloc/threadproxy.cpp
+++ b/vespamalloc/src/vespamalloc/malloc/threadproxy.cpp
@@ -58,6 +58,7 @@ void * mallocThreadProxy (void * arg)
     vespamalloc::Mutex::addThread();
     vespamalloc::_G_myMemP->initThisThread();
     void * result = nullptr;
+    ASSERT_STACKTRACE(uint64_t(&result) < vespamalloc::MAX_PTR); // Sanity check that stack is a legal PTR.
     DEBUG(fprintf(stderr, "arg(%p=%p), local(%p=%p)\n", &arg, arg, &ta, ta));
 
     pthread_cleanup_push(cleanupThread, ta);
diff --git a/vespamalloc/src/vespamalloc/util/osmem.cpp b/vespamalloc/src/vespamalloc/util/osmem.cpp
index 0267e091bab..f1d4a527732 100644
--- a/vespamalloc/src/vespamalloc/util/osmem.cpp
+++ b/vespamalloc/src/vespamalloc/util/osmem.cpp
@@ -168,6 +168,7 @@ MmapMemory::get(size_t len)
         errno = prevErrno; // The temporary error should not impact if the end is good.
         memory = getNormalPages(len);
     }
+    ASSERT_STACKTRACE((uint64_t(&memory) + len) < vespamalloc::MAX_PTR);
     return memory;
 }