iox-eclipse-iceoryx#1732 Adapt allocate method

Change return type to expected, remove option to finalize allocation

Signed-off-by: Marika Lehmann <[email protected]>

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

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 <cstdint>
 
 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<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
-    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
 

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<void*, BumpAllocatorError> 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<uint64_t>(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>(BumpAllocatorError::OUT_OF_MEMORY);
     }
 
-    return static_cast<void*>(l_returnValue);
-}
-
-void BumpAllocator::finalizeAllocation() noexcept
-{
-    m_allocationFinalized = true;
+    return cxx::success<void*>(l_returnValue);
 }
 
 void BumpAllocator::deallocate() noexcept
 {
     m_currentPosition = 0;
-    m_allocationFinalized = false;
 }
-
 } // namespace iox

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

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));
 }
 
@@ -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<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));
 }
-
-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<int*>(sut.allocate(memorySize, 1));
-    *bla = 32;
-    EXPECT_THAT(*bla, Eq(32));
-}
 } // namespace

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)

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

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>();

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);

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);
 }