From 1c5ab8c5bb3b48d54aac2eb67eb337bb448f8840 Mon Sep 17 00:00:00 2001 From: Marika Lehmann Date: Tue, 6 Dec 2022 17:47:39 +0100 Subject: [PATCH] iox-#1732 Change allocate return type to expected Signed-off-by: Marika Lehmann --- .../memory/include/iox/bump_allocator.hpp | 17 +++-- .../memory/source/bump_allocator.cpp | 25 ++++--- .../posix_wrapper/shared_memory_object.cpp | 11 ++- .../test_memory_bump_allocator.cpp | 69 ++++++++++--------- 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 +- 8 files changed, 90 insertions(+), 58 deletions(-) diff --git a/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp b/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp index cbb8266821b..dbd1cf2c52d 100644 --- a/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp +++ b/iceoryx_hoofs/memory/include/iox/bump_allocator.hpp @@ -17,6 +17,7 @@ #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 @@ -26,7 +27,13 @@ namespace iox namespace posix { class SharedMemoryObject; -} +} // namespace posix + +enum class BumpAllocatorError +{ + OUT_OF_MEMORY, + ALLOCATION_FINALIZED +}; class BumpAllocator { @@ -43,10 +50,11 @@ 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; @@ -54,6 +62,7 @@ class BumpAllocator protected: friend class posix::SharedMemoryObject; // make free function; destructive move? - then we would not be able to deallocate and allocate again afterwards + // move to SharedMemoryObject void finalizeAllocation() noexcept; private: diff --git a/iceoryx_hoofs/memory/source/bump_allocator.cpp b/iceoryx_hoofs/memory/source/bump_allocator.cpp index 95910ffeb92..4e7cfeff798 100644 --- a/iceoryx_hoofs/memory/source/bump_allocator.cpp +++ b/iceoryx_hoofs/memory/source/bump_allocator.cpp @@ -31,15 +31,15 @@ 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::ExpectsWithMsg(size > 0, "Cannot allocate memory of 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... + if (m_allocationFinalized) + { + IOX_LOG(WARN) << "allocate() call after finalizeAllocation()! You are not allowed to acquire more memory"; + return cxx::error(BumpAllocatorError::ALLOCATION_FINALIZED); + } // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) required for low level pointer alignment uint64_t currentAddress = reinterpret_cast(m_startAddress) + m_currentPosition; @@ -58,14 +58,13 @@ 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); + return cxx::success(l_returnValue); } void BumpAllocator::finalizeAllocation() noexcept diff --git a/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp b/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp index 332359512cd..683292f6506 100644 --- a/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp +++ b/iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp @@ -168,7 +168,16 @@ SharedMemoryObject::SharedMemoryObject(SharedMemory&& sharedMemory, void* SharedMemoryObject::allocate(const uint64_t size, const uint64_t alignment) noexcept { - return m_allocator.allocate(size, alignment); + auto allocationResult = m_allocator.allocate(size, alignment); + if (allocationResult.has_error()) + { + if (allocationResult.get_error() == BumpAllocatorError::OUT_OF_MEMORY) + { + IOX_LOG(ERROR) << "Not enough space left in shared memory"; + } + cxx::Expects(false); + } + return allocationResult.value(); } void SharedMemoryObject::finalizeAllocation() 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..d6a0f56abc0 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)); } @@ -141,28 +143,25 @@ TEST_F(BumpAllocator_Test, allocateAfterFinalizeAllocation) using iox::BumpAllocator::finalizeAllocation; }; AllocatorAccess sut(memory, memorySize); - sut.allocate(5, MEMORY_ALIGNMENT); + ASSERT_FALSE(sut.allocate(5, MEMORY_ALIGNMENT).has_error()); 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) + auto allocationResult = sut.allocate(5, MEMORY_ALIGNMENT); + ASSERT_TRUE(allocationResult.has_error()); + EXPECT_THAT(allocationResult.get_error(), Eq(iox::BumpAllocatorError::ALLOCATION_FINALIZED)); } TEST_F(BumpAllocator_Test, allocateAfterDeallocateWorks) { ::testing::Test::RecordProperty("TEST_ID", ""); 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)); } @@ -182,12 +181,14 @@ TEST_F(BumpAllocator_Test, allocateAfterFinalizeAllocationAndDeallocateWorks) using iox::BumpAllocator::finalizeAllocation; }; AllocatorAccess sut(memory, memorySize); - sut.allocate(memorySize, 1); + ASSERT_FALSE(sut.allocate(memorySize, 1).has_error()); sut.finalizeAllocation(); 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 = 32; EXPECT_THAT(*bla, Eq(32)); } 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); }