diff options
author | Tor Egge <Tor.Egge@online.no> | 2021-11-19 20:01:12 +0100 |
---|---|---|
committer | Tor Egge <Tor.Egge@online.no> | 2021-11-19 20:01:12 +0100 |
commit | c6410fa8f01f49b2b96fdf18ea057a8e7537e8a0 (patch) | |
tree | 351d9d6e922bd4a1ee9d0d385de5cc6f1f704ee9 /vespalib | |
parent | 06884214e04c7ea6550b8a8c275cd9bd543c10e5 (diff) |
Detect direct free of element to held buffer.
Diffstat (limited to 'vespalib')
-rw-r--r-- | vespalib/src/tests/datastore/datastore/datastore_test.cpp | 52 | ||||
-rw-r--r-- | vespalib/src/vespa/vespalib/datastore/datastore.h | 4 | ||||
-rw-r--r-- | vespalib/src/vespa/vespalib/datastore/datastore.hpp | 17 |
3 files changed, 62 insertions, 11 deletions
diff --git a/vespalib/src/tests/datastore/datastore/datastore_test.cpp b/vespalib/src/tests/datastore/datastore/datastore_test.cpp index 2e2728781f4..964978e5510 100644 --- a/vespalib/src/tests/datastore/datastore/datastore_test.cpp +++ b/vespalib/src/tests/datastore/datastore/datastore_test.cpp @@ -705,6 +705,58 @@ TEST(DataStoreTest, control_static_sizes) { EXPECT_EQ(0, bs.size()); } +namespace { + +void test_free_element_to_held_buffer(bool direct, bool before_hold_buffer) +{ + MyStore s; + auto ref = s.addEntry(1); + EXPECT_EQ(0u, MyRef(ref).bufferId()); + s.switch_primary_buffer(); + EXPECT_EQ(1u, s.primary_buffer_id()); + + if (before_hold_buffer) { + if (direct) { + s.freeElem(ref, 1); + } else { + s.holdElem(ref, 1); + } + } + s.holdBuffer(0); // hold last buffer + if (!before_hold_buffer) { + if (direct) { + ASSERT_DEATH({ s.freeElem(ref, 1); }, "state.isOnHold\\(\\) && was_held"); + } else { + ASSERT_DEATH({ s.holdElem(ref, 1); }, "state.isActive\\(\\)"); + } + } + s.transferHoldLists(100); + s.trimHoldLists(101); +} + +} + +TEST(DataStoreTest, free_to_active_then_held_buffer_is_ok) +{ + test_free_element_to_held_buffer(true, true); +} + +TEST(DataStoreTest, hold_to_active_then_held_buffer_is_ok) +{ + test_free_element_to_held_buffer(false, true); +} + +#ifndef NDEBUG +TEST(DataStoreDeathTest, free_to_held_buffer_is_not_ok) +{ + test_free_element_to_held_buffer(true, false); +} + +TEST(DataStoreDeathTest, hold_to_held_buffer_is_not_ok) +{ + test_free_element_to_held_buffer(false, false); +} +#endif } diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.h b/vespalib/src/vespa/vespalib/datastore/datastore.h index d8bb0fb3d0f..be74c2a60d5 100644 --- a/vespalib/src/vespa/vespalib/datastore/datastore.h +++ b/vespalib/src/vespa/vespalib/datastore/datastore.h @@ -27,6 +27,8 @@ template <typename RefT = EntryRefT<22> > class DataStoreT : public DataStoreBase { private: + void free_elem_internal(EntryRef ref, size_t numElems, bool was_held); + public: typedef RefT RefType; @@ -49,7 +51,7 @@ public: /** * Free element(s). */ - void freeElem(EntryRef ref, size_t numElems); + void freeElem(EntryRef ref, size_t numElems) { free_elem_internal(ref, numElems, false); } /** * Hold element(s). diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.hpp b/vespalib/src/vespa/vespalib/datastore/datastore.hpp index 81027196a75..23dcb9222a1 100644 --- a/vespalib/src/vespa/vespalib/datastore/datastore.hpp +++ b/vespalib/src/vespa/vespalib/datastore/datastore.hpp @@ -23,7 +23,7 @@ DataStoreT<RefT>::~DataStoreT() = default; template <typename RefT> void -DataStoreT<RefT>::freeElem(EntryRef ref, size_t numElems) +DataStoreT<RefT>::free_elem_internal(EntryRef ref, size_t numElems, bool was_held) { RefType intRef(ref); BufferState &state = getBufferState(intRef.bufferId()); @@ -35,9 +35,12 @@ DataStoreT<RefT>::freeElem(EntryRef ref, size_t numElems) state.freeList().push_back(ref); } } else { - assert(state.isOnHold()); + assert(state.isOnHold() && was_held); } state.incDeadElems(numElems); + if (was_held) { + state.decHoldElems(numElems); + } state.cleanHold(getBuffer(intRef.bufferId()), intRef.unscaled_offset() * state.getArraySize(), numElems); } @@ -71,10 +74,7 @@ DataStoreT<RefT>::trimElemHoldList(generation_t usedGen) for (; it != ite; ++it) { if (static_cast<sgeneration_t>(it->_generation - usedGen) >= 0) break; - RefType intRef(it->_ref); - BufferState &state = getBufferState(intRef.bufferId()); - freeElem(it->_ref, it->_len); - state.decHoldElems(it->_len); + free_elem_internal(it->_ref, it->_len, true); ++freed; } if (freed != 0) { @@ -91,10 +91,7 @@ DataStoreT<RefT>::clearElemHoldList() ElemHold2List::iterator it(elemHold2List.begin()); ElemHold2List::iterator ite(elemHold2List.end()); for (; it != ite; ++it) { - RefType intRef(it->_ref); - BufferState &state = getBufferState(intRef.bufferId()); - freeElem(it->_ref, it->_len); - state.decHoldElems(it->_len); + free_elem_internal(it->_ref, it->_len, true); } elemHold2List.clear(); } |