iox-eclipse-iceoryx#1732 Change allocate return type to expected

Signed-off-by: Marika Lehmann <[email protected]>
ApexAI · Dec 6, 2022 · 1c5ab8c · 1c5ab8c
1 parent 0dbb60f
commit 1c5ab8c
Show file tree

Hide file tree

Showing 8 changed files with 90 additions and 58 deletions.
diff --git 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 <cstdint>
@@ -26,7 +27,13 @@ namespace iox
 namespace posix
 {
 class SharedMemoryObject;
-}
+} // namespace posix
+
+enum class BumpAllocatorError
+{
+    OUT_OF_MEMORY,
+    ALLOCATION_FINALIZED
+};
 
 class BumpAllocator
 {
@@ -43,17 +50,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<void*, BumpAllocatorError> 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
+    // 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
@@ -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<void*, BumpAllocatorError> 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>(BumpAllocatorError::ALLOCATION_FINALIZED);
+    }
 
     // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) required for low level pointer alignment
     uint64_t currentAddress = reinterpret_cast<uint64_t>(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>(BumpAllocatorError::OUT_OF_MEMORY);
     }
 
-    return static_cast<void*>(l_returnValue);
+    return cxx::success<void*>(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
@@ -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
@@ -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<int*>(sut.allocate(sizeof(int), MEMORY_ALIGNMENT));
+    auto allocationResult = sut.allocate(sizeof(int), MEMORY_ALIGNMENT);
+    ASSERT_FALSE(allocationResult.has_error());
+    int* bla = static_cast<int*>(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<int*>(sut.allocate(memorySize, 1));
+    auto allocationResult = sut.allocate(memorySize, 1);
+    ASSERT_FALSE(allocationResult.has_error());
+    int* bla = static_cast<int*>(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<size_t*>(sut.allocate(32, 1));
+        auto allocationResult = sut.allocate(32, 1);
+        ASSERT_FALSE(allocationResult.has_error());
+        auto* bla = static_cast<size_t*>(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<uint8_t*>(sut.allocate(5, MEMORY_ALIGNMENT));
-    auto* bla2 = static_cast<uint8_t*>(sut.allocate(5, MEMORY_ALIGNMENT));
+    auto allocationResult = sut.allocate(5, MEMORY_ALIGNMENT);
+    ASSERT_FALSE(allocationResult.has_error());
+    auto* bla = static_cast<uint8_t*>(allocationResult.value());
+
+    allocationResult = sut.allocate(5, MEMORY_ALIGNMENT);
+    ASSERT_FALSE(allocationResult.has_error());
+    auto* bla2 = static_cast<uint8_t*>(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<int*>(sut.allocate(memorySize, 1));
+    auto allocationResult = sut.allocate(memorySize, 1);
+    ASSERT_FALSE(allocationResult.has_error());
+    int* bla = static_cast<int*>(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<int*>(sut.allocate(memorySize, 1));
+    auto allocationResult = sut.allocate(memorySize, 1);
+    ASSERT_FALSE(allocationResult.has_error());
+    int* bla = static_cast<int*>(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
@@ -49,10 +49,15 @@ MemPool::MemPool(const cxx::greater_or_equal<uint32_t, CHUNK_MEMORY_ALIGNMENT> c
 {
     if (isMultipleOfAlignment(chunkSize))
     {
-        m_rawMemory = static_cast<uint8_t*>(chunkMemoryAllocator.allocate(
-            static_cast<uint64_t>(m_numberOfChunks) * m_chunkSize, CHUNK_MEMORY_ALIGNMENT));
-        auto memoryLoFFLi =
+        auto allocationResult = chunkMemoryAllocator.allocate(static_cast<uint64_t>(m_numberOfChunks) * m_chunkSize,
+                                                              CHUNK_MEMORY_ALIGNMENT);
+        cxx::Expects(!allocationResult.has_error());
+        m_rawMemory = static_cast<uint8_t*>(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<concurrent::LoFFLi::Index_t*>(memoryLoFFLi), m_numberOfChunks);
     }
     else

diff --git a/iceoryx_posh/source/roudi/memory/memory_provider.cpp b/iceoryx_posh/source/roudi/memory/memory_provider.cpp
@@ -98,7 +98,12 @@ cxx::expected<MemoryProviderError> 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>(MemoryProviderError::MEMORY_ALLOCATION_FAILED);
+        }
+        memoryBlock->m_memory = allocationResult.value();
     }
 
     return cxx::success<void>();

diff --git 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<void*> 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
@@ -49,7 +49,9 @@ uint64_t MemPoolSegmentManagerMemoryBlock::alignment() const noexcept
 void MemPoolSegmentManagerMemoryBlock::onMemoryAvailable(cxx::not_null<void*> 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);
 }