1
\$\begingroup\$

I am trying to implement smart pointers for our project. We're using FreeRTOS which is written with a C API so it was quite a challenge to implement sending smart pointers around but somehow I managed it by combining casts, new calls (dont really like that..) move semantics and some other tricks.

On the surface it behaves as expected, but I want to be sure there is not any memory leak. Furthermore, is there a way to use make_unique together with new? I have not really found a way yet.

Here is the code, it should work with an online C++ compiler (or use this link). It would be great if an expert could look over this. Maybe some things can be optimized, or maybe this code is not save at all and I just dont see it.

// Example program
#include <iostream>
#include <string>
#include <memory>
#include <cstring>

class DummyHelper {
public:
    DummyHelper(uint8_t someResource): someResource(someResource) {}

    virtual ~DummyHelper() {
       someResource = 0;
       std::cout << "Dtor called" << std::endl;
    }

    int getResource() {
       return someResource;
    }
private:
   int someResource = 0; 
};

class CommandMessage {
public:
    CommandMessage() = default;
    virtual~ CommandMessage() {};

    void setParameter(uint64_t parameter) {
        memcpy(rawContent.data() + 4, &parameter, sizeof(parameter));
    }

    uint64_t getParameter() const {
       uint64_t param;
       memcpy(&param, rawContent.data() + 4, sizeof(param));
       return param;
    }

private:
    std::array<uint8_t, 12> rawContent;
};

void setTestMessage(CommandMessage *testMsg,
       uint8_t someResource) {
  auto unique_pptr = new std::unique_ptr<DummyHelper>(new DummyHelper(someResource));
  uint64_t test_ptr = reinterpret_cast<uint64_t>(unique_pptr);
  testMsg->setParameter(test_ptr);
}

std::unique_ptr<DummyHelper> getUniquePtr(
       CommandMessage *testMsg) {
   uint64_t raw_ptr = testMsg->getParameter();
   auto unique_pptr = reinterpret_cast<std::unique_ptr<DummyHelper>*>(raw_ptr);
   auto unique_ptr = std::move(*unique_pptr);
   delete unique_pptr;
   return unique_ptr;
}


int main()
{
  auto storagePtr = std::make_unique<DummyHelper>(42);
  CommandMessage testMessage;
  std::cout << "Setting message" << std::endl;
  setTestMessage(&testMessage, 42);
  // a message has been set and can be sent, e.g. via a message queue

  // in another class, the pointer is retrieved.
  auto unique_ptr = getUniquePtr(&testMessage);
  DummyHelper & test = *unique_ptr;
  std::cout << test.getResource() << std::endl;
  unique_ptr.reset();
} 
\$\endgroup\$
3
  • \$\begingroup\$ Yes it leaks. SImply count the calls to new/delete and make sure they are the same. They are not. But I don't understand what you are trying to achieve. Why are you trying to create a smart pointer when you are using a smart pointer! You have a std::unique_ptr this is a smart pointer. \$\endgroup\$ May 7, 2020 at 16:54
  • \$\begingroup\$ I think I am not able to send it via the message queue. If i create a simple unique_ptr and copy it into the message queue, the resource will be destroyed after going out of scope. I don't think it is possible to transfer ownership to the FreeRTOS message queue, given it was not programmed with C++ features in mind. I actually updated the code: I deleted the pointer in the getUniquePtr() call. \$\endgroup\$
    – Spacefish
    May 7, 2020 at 18:22
  • \$\begingroup\$ You're making a pointer. To a unique_ptr. With new. I don't think you grasp the concept of smart pointers yet. \$\endgroup\$
    – S.S. Anne
    May 7, 2020 at 23:17

2 Answers 2

3
\$\begingroup\$

3 years late to the party, but here's my suggestion to solve the problem. I'm assuming you specifically want to send a smart pointer through the FreeRTOS Queue. Your implementation technically works, but has a couple of issues:

  1. You are sending 'unique_pptr' to the receiver which is a non-smart pointer. That defeats your original purpose. Your 'unique_pptr' can potentially leak.
  2. The sending thread and receiving thread both have access to the same instance of std::unique_ptr. However, std:unique_ptr is not thread safe.

Reference counting using a C API can't be done effectively, so it's not worth trying. Instead make the C++ std::queue thread-safe and synchronized using FreeRTOS.

I'm using the CMSIS wrapper for FreeRTOS, but it's easily translated.

#pragma once
#include <queue>
#include <limits>
#include <memory>
#include "cmsis_os.h"

template <typename T>
class MessageQueue
{
public:
    MessageQueue()
    {
        mSemaId = osSemaphoreNew(std::numeric_limits<uint32_t>::max(), 0, nullptr);
        mMutexId = osMutexNew(nullptr);
        
    };

    ~MessageQueue()
    {
        if (mSemaId)
        {
            osSemaphoreDelete(mSemaId);
        }
        
        if (mMutexId)
        {
            osMutexDelete(mMutexId);
        }
    };
    
    void PushBack(const T& value)
    {
        // add an item to the queue, but thread safely
        osMutexAcquire(mMutexId, osWaitForever);
        mQueue.push(value);
        osMutexRelease(mMutexId);

        osSemaphoreRelease(mSemaId);  // signal another message is ready

    }
    
    
    T PopFront()
    {
        // wait here for a message to come in
        osSemaphoreAcquire(mSemaId, osWaitForever);  // use a timeout parameter instead
        
        // take the first item out of the queue, but thread safely
        osMutexAcquire(mMutexId, osWaitForever);
        T value = mQueue.front();
        mQueue.pop();
        osMutexRelease(mMutexId);

        return value;
    }
    
    
private:
    std::queue<T> mQueue;   // <--- replaces the FreeRTOS queue
    osSemaphoreId_t mSemaId = nullptr;
    osMutexId_t mMutexId = nullptr;
};

Basic usage shown, but no thread creation for clarity

struct Msg
{
     using Ptr = std::shared_ptr<Msg>;
     std:string data;
};

class ReceiverThread
{
    
public:
    
    void CalledBySenderThread(const Msg::Ptr& msg)
    {
        mMessageQueue.PushBack(msg);
    }

private:   
    void ReceiverThreadLoop()
    {
        while (true)
        {
            Msg::Ptr msg = mMessageQueue.PopFront();
            // process(msg);
            // msg is automatically deleted
        }
    }

    MessageQueue<Msg::Ptr> mMessageQueue;
};

Notice I'm using std::shared_ptr. I only want the message destroyed once the sender AND receiver have finished using it. I could use std::unique_ptr but would require the additional move semantics.

One gotcha: you can't access the queue from an interrupt.

\$\endgroup\$
0
2
\$\begingroup\$

Overview:

In relation to the message queue:

  • Since you are doing a manual new and delete that seems a bit of a waste.
  • Also the only thing you are creating is a unique_ptr.
  • The unique_ptr is never in stored correctly typed so it never does any work.

So I would not store a pointer to a std::unique_ptr I would just store the original pointer. I like the idea of passing in and retrieving a unique_ptr to the access function.


Why are you passing a pointer to CommandMessage? It can't be null (you never check so must be valid. To make this more formal pass by reference to indicate to the user that it should be a normal object.

void setTestMessage(CommandMessage *testMsg, uint8_t someResource)
std::unique_ptr<DummyHelper> getUniquePtr(CommandMessage *testMsg)

// I would do this:

void setTestMessage(CommandMessage& testMsg, uint8_t someResource)
std::unique_ptr<DummyHelper> getUniquePtr(CommandMessage& testMsg)

Note: Putting the '*' on the right by the variable is very "C" like. C++ considers this part of the type information so it is usually on the left with the type.


Why is the input message different from the output message?

void setTestMessage(CommandMessage *testMsg, uint8_t someResource)
std::unique_ptr<DummyHelper> getUniquePtr(CommandMessage *testMsg)

So input is a uint8_t but the output is std::unique_ptr<DummyHelper>? I would expect the interface to be symmetric.

template<typename T>
void sendMessage(CommandMessage& messageStream, std::unique_ptr<T>&& msg);

template<typename T>
std::unique_ptr<T> recvMessage(CommandMessage& messageStream);

template<typename T>
std::unique_ptr<T> recvMessage(CommandMessage& messageStream)
{
   // To store pointers in an integer type you should be using `std::intptr_t`
   // This is an int type that is guaranteed large enough to store a pointer.
   // Don't make the assumption that `uint64_t` is large enough.
   // It might be but that is not guaranteed for the future.
   std::intptr_t raw_ptr = messageStream.getParameter();

   return reinterpret_cast<T*>(raw_ptr);
}


template<typename T>
void sendMessage(CommandMessage& messageStream, std::unique_ptr<T>&& msg)
{
    messageStream.setParameter(msg.release());
}

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.