From 4be18a70fa0d08a6c45050d170619a1b90190b08 Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Tue, 6 Dec 2022 17:47:39 +0100 Subject: [PATCH] iox-#1732 Adapt allocate method Change return type to expected, remove option to finalize allocation Signed-off-by: Marika Lehmann --- .../posix_wrapper/shared_memory_object.hpp | 1 + .../memory/include/iox/bump_allocator.hpp | 21 ++-- .../memory/source/bump_allocator.cpp | 28 ++--- .../posix_wrapper/shared_memory_object.cpp | 10 +- .../test_memory_bump_allocator.cpp | 104 +++++------------- .../test_posix_shared_memory_object.cpp | 6 +- iceoryx_posh/source/mepoo/mem_pool.cpp | 11 +- .../source/roudi/memory/memory_provider.cpp | 7 +- .../mempool_collection_memory_block.cpp | 4 +- .../mempool_segment_manager_memory_block.cpp | 4 +- 10 files changed, 76 insertions(+), 120 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp index 8922e0dfc1d..60eda25b9a8 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp @@ -103,6 +103,7 @@ class SharedMemoryObject SharedMemory m_sharedMemory; MemoryMap m_memoryMap; BumpAllocator m_allocator; + bool m_allocationFinalized{false}; }; class SharedMemoryObjectBuilder diff --git a/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp b/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp index cbb8266821b..c580ea7a5e2 100644 --- a/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp +++ b/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp @@ -17,16 +17,18 @@ #ifndef IOX_HOOFS_MEMORY_BUMP_ALLOCATOR_HPP #define IOX_HOOFS_MEMORY_BUMP_ALLOCATOR_HPP +#include "iceoryx_hoofs/cxx/expected.hpp" #include "iceoryx_hoofs/iceoryx_hoofs_types.hpp" #include namespace iox { -namespace posix + +enum class BumpAllocatorError { -class SharedMemoryObject; -} + OUT_OF_MEMORY +}; class BumpAllocator { @@ -43,24 +45,19 @@ class BumpAllocator ~BumpAllocator() noexcept = default; /// @brief allocates on the memory supplied with the ctor - /// @param[in] size of the memory to allocate + /// @param[in] size of the memory to allocate, must be greater than 0 /// @param[in] alignment of the memory to allocate - /// @note May terminate if out of memory or finalizeAllocation() was called before - void* allocate(const uint64_t size, const uint64_t alignment) noexcept; + /// @return returns an expected containing a pointer to the allocated memory if memory is available, otherwise + /// BumpAllocatorError::OUT_OF_MEMORY + cxx::expected allocate(const uint64_t size, const uint64_t alignment) noexcept; /// @brief mark the memory as unused void deallocate() noexcept; - protected: - friend class posix::SharedMemoryObject; - // make free function; destructive move? - then we would not be able to deallocate and allocate again afterwards - void finalizeAllocation() noexcept; - private: cxx::byte_t* m_startAddress{nullptr}; uint64_t m_length{0U}; uint64_t m_currentPosition = 0U; - bool m_allocationFinalized = false; }; } // namespace iox diff --git a/iceoryx_hoofs/memory/source/bump_allocator.cpp b/iceoryx_hoofs/memory/source/bump_allocator.cpp index 95910ffeb92..4d39c9d8f8c 100644 --- a/iceoryx_hoofs/memory/source/bump_allocator.cpp +++ b/iceoryx_hoofs/memory/source/bump_allocator.cpp @@ -31,15 +31,9 @@ BumpAllocator::BumpAllocator(void* const startAddress, const uint64_t length) no // NOLINTJUSTIFICATION allocation interface requires size and alignment as integral types // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -void* BumpAllocator::allocate(const uint64_t size, const uint64_t alignment) noexcept +cxx::expected BumpAllocator::allocate(const uint64_t size, const uint64_t alignment) noexcept { - cxx::Expects(size > 0); - - cxx::Expects( - !m_allocationFinalized - && "allocate() call after finalizeAllocation()! You are not allowed to acquire shared memory chunks anymore"); - // return a nullptr instead of terminate? Then this could be checked in SharedMemoryObject and the log messages - // could be printed there... + cxx::ExpectsWithMsg(size > 0, "Cannot allocate memory of size 0"); // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) required for low level pointer alignment uint64_t currentAddress = reinterpret_cast(m_startAddress) + m_currentPosition; @@ -58,25 +52,17 @@ void* BumpAllocator::allocate(const uint64_t size, const uint64_t alignment) noe } else { - std::cerr << "Trying to allocate additional " << size << " bytes in the shared memory of capacity " << m_length - << " when there are already " << alignedPosition << " aligned bytes in use." << std::endl; - std::cerr << "Only " << m_length - alignedPosition << " bytes left." << std::endl; - - cxx::Expects(false && "Not enough space left in shared memory"); + IOX_LOG(WARN) << "Trying to allocate additional " << size << " bytes in the memory of capacity " << m_length + << " when there are already " << alignedPosition << " aligned bytes in use.\n Only " + << m_length - alignedPosition << " bytes left."; + return cxx::error(BumpAllocatorError::OUT_OF_MEMORY); } - return static_cast(l_returnValue); -} - -void BumpAllocator::finalizeAllocation() noexcept -{ - m_allocationFinalized = true; + return cxx::success(l_returnValue); } void BumpAllocator::deallocate() noexcept { m_currentPosition = 0; - m_allocationFinalized = false; } - } // namespace iox diff --git a/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp b/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp index 332359512cd..08433e13163 100644 --- a/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp +++ b/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp @@ -168,12 +168,18 @@ SharedMemoryObject::SharedMemoryObject(SharedMemory&& sharedMemory, void* SharedMemoryObject::allocate(const uint64_t size, const uint64_t alignment) noexcept { - return m_allocator.allocate(size, alignment); + cxx::ExpectsWithMsg( + !m_allocationFinalized, + "allocate() call after finalizeAllocation()! You are not allowed to acquire shared memory chunks anymore"); + + auto allocationResult = m_allocator.allocate(size, alignment); + cxx::ExpectsWithMsg(!allocationResult.has_error(), "Not enough space left in shared memory"); + return allocationResult.value(); } void SharedMemoryObject::finalizeAllocation() noexcept { - m_allocator.finalizeAllocation(); + m_allocationFinalized = true; } BumpAllocator& SharedMemoryObject::getBumpAllocator() noexcept diff --git a/iceoryx_hoofs/test/moduletests/test_memory_bump_allocator.cpp b/iceoryx_hoofs/test/moduletests/test_memory_bump_allocator.cpp index c3a974dac01..d9edadfbca5 100644 --- a/iceoryx_hoofs/test/moduletests/test_memory_bump_allocator.cpp +++ b/iceoryx_hoofs/test/moduletests/test_memory_bump_allocator.cpp @@ -47,7 +47,9 @@ TEST_F(BumpAllocator_Test, allocateOneSmallElement) { ::testing::Test::RecordProperty("TEST_ID", "f689e95c-5743-4370-93f0-8a23b909c75a"); iox::BumpAllocator sut(memory, memorySize); - int* bla = static_cast(sut.allocate(sizeof(int), MEMORY_ALIGNMENT)); + auto allocationResult = sut.allocate(sizeof(int), MEMORY_ALIGNMENT); + ASSERT_FALSE(allocationResult.has_error()); + int* bla = static_cast(allocationResult.value()); *bla = 123; EXPECT_THAT(*bla, Eq(123)); } @@ -56,7 +58,9 @@ TEST_F(BumpAllocator_Test, allocateEverythingWithSingleElement) { ::testing::Test::RecordProperty("TEST_ID", "f2e1085b-08fe-4b08-b022-0385b5a53fca"); iox::BumpAllocator sut(memory, memorySize); - int* bla = static_cast(sut.allocate(memorySize, 1)); + auto allocationResult = sut.allocate(memorySize, 1); + ASSERT_FALSE(allocationResult.has_error()); + int* bla = static_cast(allocationResult.value()); *bla = 123; EXPECT_THAT(*bla, Eq(123)); } @@ -67,7 +71,9 @@ TEST_F(BumpAllocator_Test, allocateEverythingWithMultipleElements) iox::BumpAllocator sut(memory, memorySize); for (size_t i = 0; i < memorySize; i += 32) { - auto* bla = static_cast(sut.allocate(32, 1)); + auto allocationResult = sut.allocate(32, 1); + ASSERT_FALSE(allocationResult.has_error()); + auto* bla = static_cast(allocationResult.value()); *bla = i; EXPECT_THAT(*bla, Eq(i)); } @@ -77,13 +83,9 @@ TEST_F(BumpAllocator_Test, allocateTooMuchSingleElement) { ::testing::Test::RecordProperty("TEST_ID", "9deed5c0-19d8-4469-a5c3-f185d4d881f1"); iox::BumpAllocator sut(memory, memorySize); - std::set_terminate([]() { std::cout << "", std::abort(); }); - // @todo iox-#1613 remove EXPECT_DEATH - // NOLINTBEGIN(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, - // hiccpp-vararg) - EXPECT_DEATH({ sut.allocate(memorySize + 1, MEMORY_ALIGNMENT); }, ".*"); - // NOLINTEND(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, - // hiccpp-vararg) + auto allocationResult = sut.allocate(memorySize + 1, MEMORY_ALIGNMENT); + ASSERT_TRUE(allocationResult.has_error()); + EXPECT_THAT(allocationResult.get_error(), Eq(iox::BumpAllocatorError::OUT_OF_MEMORY)); } TEST_F(BumpAllocator_Test, allocateTooMuchMultipleElement) @@ -92,25 +94,25 @@ TEST_F(BumpAllocator_Test, allocateTooMuchMultipleElement) iox::BumpAllocator sut(memory, memorySize); for (size_t i = 0; i < memorySize; i += 32) { - sut.allocate(32, 1); + ASSERT_FALSE(sut.allocate(32, 1).has_error()); } - std::set_terminate([]() { std::cout << "", std::abort(); }); - - // @todo iox-#1613 remove EXPECT_DEATH - // NOLINTBEGIN(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, - // hiccpp-vararg) - EXPECT_DEATH({ sut.allocate(1, MEMORY_ALIGNMENT); }, ".*"); - // NOLINTEND(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, - // hiccpp-vararg) + auto allocationResult = sut.allocate(1, MEMORY_ALIGNMENT); + ASSERT_TRUE(allocationResult.has_error()); + EXPECT_THAT(allocationResult.get_error(), Eq(iox::BumpAllocatorError::OUT_OF_MEMORY)); } TEST_F(BumpAllocator_Test, allocateAndAlignment) { ::testing::Test::RecordProperty("TEST_ID", "4252ddcc-05d4-499f-ad7c-30bffb420e08"); iox::BumpAllocator sut(memory, memorySize); - auto* bla = static_cast(sut.allocate(5, MEMORY_ALIGNMENT)); - auto* bla2 = static_cast(sut.allocate(5, MEMORY_ALIGNMENT)); + auto allocationResult = sut.allocate(5, MEMORY_ALIGNMENT); + ASSERT_FALSE(allocationResult.has_error()); + auto* bla = static_cast(allocationResult.value()); + + allocationResult = sut.allocate(5, MEMORY_ALIGNMENT); + ASSERT_FALSE(allocationResult.has_error()); + auto* bla2 = static_cast(allocationResult.value()); EXPECT_THAT(bla2 - bla, Eq(8U)); } @@ -127,68 +129,18 @@ TEST_F(BumpAllocator_Test, allocateElementOfSizeZero) // hiccpp-vararg) } -TEST_F(BumpAllocator_Test, allocateAfterFinalizeAllocation) -{ - ::testing::Test::RecordProperty("TEST_ID", "323fc1af-481f-4732-b7d3-fa32da389cef"); - class AllocatorAccess : iox::BumpAllocator - { - public: - AllocatorAccess(void* const f_startAddress, const uint64_t f_length) - : iox::BumpAllocator(f_startAddress, f_length) - { - } - using iox::BumpAllocator::allocate; - using iox::BumpAllocator::finalizeAllocation; - }; - AllocatorAccess sut(memory, memorySize); - sut.allocate(5, MEMORY_ALIGNMENT); - sut.finalizeAllocation(); - - std::set_terminate([]() { std::cout << "", std::abort(); }); - - // @todo iox-#1613 remove EXPECT_DEATH - // NOLINTBEGIN(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, - // hiccpp-vararg) - EXPECT_DEATH({ sut.allocate(5, MEMORY_ALIGNMENT); }, ".*"); - // NOLINTEND(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, - // hiccpp-vararg) -} - TEST_F(BumpAllocator_Test, allocateAfterDeallocateWorks) { - ::testing::Test::RecordProperty("TEST_ID", ""); + ::testing::Test::RecordProperty("TEST_ID", "323fc1af-481f-4732-b7d3-fa32da389cef"); iox::BumpAllocator sut(memory, memorySize); - sut.allocate(memorySize, 1); + ASSERT_FALSE(sut.allocate(memorySize, 1).has_error()); sut.deallocate(); - int* bla = static_cast(sut.allocate(memorySize, 1)); + auto allocationResult = sut.allocate(memorySize, 1); + ASSERT_FALSE(allocationResult.has_error()); + int* bla = static_cast(allocationResult.value()); *bla = 1990; EXPECT_THAT(*bla, Eq(1990)); } - -TEST_F(BumpAllocator_Test, allocateAfterFinalizeAllocationAndDeallocateWorks) -{ - ::testing::Test::RecordProperty("TEST_ID", ""); - class AllocatorAccess : iox::BumpAllocator - { - public: - AllocatorAccess(void* const startAddress, const uint64_t length) - : iox::BumpAllocator(startAddress, length) - { - } - using iox::BumpAllocator::allocate; - using iox::BumpAllocator::deallocate; - using iox::BumpAllocator::finalizeAllocation; - }; - AllocatorAccess sut(memory, memorySize); - sut.allocate(memorySize, 1); - sut.finalizeAllocation(); - - sut.deallocate(); - - int* bla = static_cast(sut.allocate(memorySize, 1)); - *bla = 32; - EXPECT_THAT(*bla, Eq(32)); -} } // namespace diff --git a/iceoryx_hoofs/test/moduletests/test_posix_shared_memory_object.cpp b/iceoryx_hoofs/test/moduletests/test_posix_shared_memory_object.cpp index f8db16fb606..fb5f545f44b 100644 --- a/iceoryx_hoofs/test/moduletests/test_posix_shared_memory_object.cpp +++ b/iceoryx_hoofs/test/moduletests/test_posix_shared_memory_object.cpp @@ -46,9 +46,9 @@ class SharedMemoryObject_Test : public Test std::set_terminate([]() { std::cout << "", std::abort(); }); internal::GetCapturedStderr(); - // @todo iox-#1613 remove EXPECT_DEATH - // NOLINTBEGIN(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, - // hiccpp-vararg) + // @todo iox-#1613 remove EXPECT_DEATH + // NOLINTBEGIN(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, + // hiccpp-vararg) EXPECT_DEATH({ deathTest(); }, ".*"); // NOLINTEND(hicpp-avoid-goto, cppcoreguidelines-avoid-goto, cert-err33-c, cppcoreguidelines-pro-type-vararg, // hiccpp-vararg) diff --git a/iceoryx_posh/source/mepoo/mem_pool.cpp b/iceoryx_posh/source/mepoo/mem_pool.cpp index 17382f5c74e..b4456ec9ea5 100644 --- a/iceoryx_posh/source/mepoo/mem_pool.cpp +++ b/iceoryx_posh/source/mepoo/mem_pool.cpp @@ -49,10 +49,15 @@ MemPool::MemPool(const cxx::greater_or_equal c { if (isMultipleOfAlignment(chunkSize)) { - m_rawMemory = static_cast(chunkMemoryAllocator.allocate( - static_cast(m_numberOfChunks) * m_chunkSize, CHUNK_MEMORY_ALIGNMENT)); - auto memoryLoFFLi = + auto allocationResult = chunkMemoryAllocator.allocate(static_cast(m_numberOfChunks) * m_chunkSize, + CHUNK_MEMORY_ALIGNMENT); + cxx::Expects(!allocationResult.has_error()); + m_rawMemory = static_cast(allocationResult.value()); + + allocationResult = managementAllocator.allocate(freeList_t::requiredIndexMemorySize(m_numberOfChunks), CHUNK_MEMORY_ALIGNMENT); + cxx::Expects(!allocationResult.has_error()); + auto* memoryLoFFLi = allocationResult.value(); m_freeIndices.init(static_cast(memoryLoFFLi), m_numberOfChunks); } else diff --git a/iceoryx_posh/source/roudi/memory/memory_provider.cpp b/iceoryx_posh/source/roudi/memory/memory_provider.cpp index 445c9de1184..78212badb7a 100644 --- a/iceoryx_posh/source/roudi/memory/memory_provider.cpp +++ b/iceoryx_posh/source/roudi/memory/memory_provider.cpp @@ -98,7 +98,12 @@ cxx::expected MemoryProvider::create() noexcept for (auto* memoryBlock : m_memoryBlocks) { - memoryBlock->m_memory = allocator.allocate(memoryBlock->size(), memoryBlock->alignment()); + auto allocationResult = allocator.allocate(memoryBlock->size(), memoryBlock->alignment()); + if (allocationResult.has_error()) + { + return cxx::error(MemoryProviderError::MEMORY_ALLOCATION_FAILED); + } + memoryBlock->m_memory = allocationResult.value(); } return cxx::success(); diff --git a/iceoryx_posh/source/roudi/memory/mempool_collection_memory_block.cpp b/iceoryx_posh/source/roudi/memory/mempool_collection_memory_block.cpp index c5dee89af73..d2226449b9d 100644 --- a/iceoryx_posh/source/roudi/memory/mempool_collection_memory_block.cpp +++ b/iceoryx_posh/source/roudi/memory/mempool_collection_memory_block.cpp @@ -52,7 +52,9 @@ uint64_t MemPoolCollectionMemoryBlock::alignment() const noexcept void MemPoolCollectionMemoryBlock::onMemoryAvailable(cxx::not_null memory) noexcept { BumpAllocator allocator(memory, size()); - auto memoryManager = allocator.allocate(sizeof(mepoo::MemoryManager), alignof(mepoo::MemoryManager)); + auto allocationResult = allocator.allocate(sizeof(mepoo::MemoryManager), alignof(mepoo::MemoryManager)); + cxx::Expects(!allocationResult.has_error()); + auto* memoryManager = allocationResult.value(); m_memoryManager = new (memoryManager) mepoo::MemoryManager; m_memoryManager->configureMemoryManager(m_memPoolConfig, allocator, allocator); diff --git a/iceoryx_posh/source/roudi/memory/mempool_segment_manager_memory_block.cpp b/iceoryx_posh/source/roudi/memory/mempool_segment_manager_memory_block.cpp index 3572fcef567..f436a0c6bb3 100644 --- a/iceoryx_posh/source/roudi/memory/mempool_segment_manager_memory_block.cpp +++ b/iceoryx_posh/source/roudi/memory/mempool_segment_manager_memory_block.cpp @@ -49,7 +49,9 @@ uint64_t MemPoolSegmentManagerMemoryBlock::alignment() const noexcept void MemPoolSegmentManagerMemoryBlock::onMemoryAvailable(cxx::not_null memory) noexcept { BumpAllocator allocator(memory, size()); - auto segmentManager = allocator.allocate(sizeof(mepoo::SegmentManager<>), alignof(mepoo::SegmentManager<>)); + auto allocationResult = allocator.allocate(sizeof(mepoo::SegmentManager<>), alignof(mepoo::SegmentManager<>)); + cxx::Expects(!allocationResult.has_error()); + auto* segmentManager = allocationResult.value(); m_segmentManager = new (segmentManager) mepoo::SegmentManager<>(m_segmentConfig, &allocator); }