rtos::MemoryPool doesn't handle non-trivial (i.e. non-"C compatible") types

[Crzyrndm]

Description

Background - I'm writing a desktop / portable implementation of the mbed rtos and utilities to enable more testing of low level infrastructure code (without the limitations imposed by working directly on the hardware). Due to this I've been digging quite deeply into some of the rtos constructs

Problem - rtos::MemoryPool is a simple wrapper over rtx_MemoryPool. This in itself is not an issue, but there is no protection in place preventing (or code to allow) a user using rtos::MemoryPool with a type (T) that relies on the ctor/dtor being called appropriately (e.g. a smart pointer member which relies on it's destructor being called to free linked memory).

I see this issue (https://github.com/ARMmbed/mbed-os/issues/5891) which pointed out the lack of documentation around this, but I have to wonder why not just fix the issue at source? Having ctor/dtor is one of the biggest advantages of working in C++ instead of C and the fact they are missing here is very non-obvious.

Affects

• rtos::MemoryPool
• rtos::Mail (based on MemoryPool)

This is relatively easy to resolve in my own projects by wrapping it in a class which calls ctor/dtor on the way in and out, but it seems like a footgun in the library which should be resolved (either through asserting that the type is compatible with the current implementation, or adding ctor/dtor calls).

The assert is dead easy with C++11 compilers but could be a little more difficult if that is not available

#if __cplusplus >= 201103L
#include <type_traits>
#endif

#if __cplusplus >= 201103L
static_assert(std::is_trivial<T>::value,
                     "rtos::MemoryPool can only handle types with no ctor/dtor requirements");
#endif

Placement new is I believe standard right through from C++98 (requires the < new > header)

#include <new>
// MemoryPool<T>::alloc
// default ctor on block of memory returned by the memory pool.
// The "new" here doesn't allocate at all
//
// other notes on placement new:
// - "::new" ensures the use of the global (rather than class specific) new operator.
//       this enables us to guarantee no heap allocation is made
// - the placement pointer should always be a "void*" 
//       (avoiding hijacks by overloaded new with other pointer args)
return ::new (osMemoryPoolAlloc(_id, 0)) T;

// MemoryPool<T>::free
block->~T(); // manually call dtor of T. No deallocation of memory
return osMemoryPoolFree(_id, (void *)block);

Godbolt link demonstrating in a simplified form (note how the "pool" memory is allocated) how placement new could be used and that compile output is not modified when the ctor is a no-op. NOTE: I also included a C++11 version (cpp11_alloc) which allows non-default ctors to be called. This is how I've written my wrapper (because why require a default ctor ;) )

Issue request type

[ ] Question
[X] Enhancement
[ ] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-773

[0xc0170]

Background - I'm writing a desktop / portable implementation of the mbed rtos and utilities to enable more testing of low level infrastructure code (without the limitations imposed by working directly on the hardware). Due to this I've been digging quite deeply into some of the rtos constructs

I am eager to hear more about this one, will you be able to share more?

I see this issue (#5891) which pointed out the lack of documentation around this, but I have to wonder why not just fix the issue at source? Having ctor/dtor is one of the biggest advantages of working in C++ instead of C and the fact they are missing here is very non-obvious.

@ARMmbed/mbed-os-core Please review.

[Crzyrndm]

I am eager to hear more about this one, will you be able to share more?

Will definitely be considered once I've solved some of the less trivial issues. MemoryPool is not exactly complicated, while a portable emulation of Thread (priority, stack usage, etc.) will be "interesting" to say the least (probably not worth the trouble for my application). The initial push for this was an rtos::Mail replacement that only exposes push/pop (at the cost of a slightly higher naive stack usage due to the required copy) so I've only really looked into Queue and MemoryPool so far.

EDIT

For reference: MemoryPool wrapper

[kjbracey]

Makes sense that MemoryPool should be able to do this, but changing alloc and free would not be a backwards-compatible change - anyone already using it with C++ may well be doing their own placement new in the pointer returned, and that requirement is documented.

You would have to make it new methods, rather than changing alloc and free.

Feels like it's a teeny bit backward though - why would any user of your class be explicitly calling the MemoryPool?

If you have a system pool for a given object, should that object not have its own new and delete methods to force allocation via that pool, transparently to the user of the class? And they would want to call the "plain" alloc and free from the pool, and then normal constructors and destructors would be called.

[Crzyrndm]

My wrapper is required only b/c MemoryPool doesn't itself provide methods that safetly construct/destruct non-trivial objects. I wrote it as a drop in replacement for my projects (hence still "alloc/free") and shared it as an example of how this functionality can be added (for those who are unfamiliar with manual init/deinit). As a library modification I would suggest something like "borrow/return" (deprecating alloc/free)

I raised the issue because (as I see it) there is (almost**) no reason for MemoryPool not to "do the right thing" and invoke ctor/dtor. Current memory pool has little advatage over using the RTX functions directly (I would push people that way because it makes it obvious that it only works for C like types...)

* "almost" because pre-C++11 (variadic arguments) you need a default ctor for T. My suggestion there would be to make an ugly function name ("uninitialised_borrow") with lots of warnings about using placement new to initialise the returned pointer (make it return void. Safer for placement new AND difficult to overlook)

[kjbracey]

I guess my point is that in C++ code should not usually be explicitly calling "alloc" or "free" functions to allocate or delete objects, just as they shouldn't be calling ::malloc or ::free. They should really be using new and delete, or an Allocator.

If working that way, the implementation of the custom new/delete operators, or the Allocator, will be using the plain alloc/free from MemoryPool, but users of the object won't be seeing that detail.

Certainly we could add new MemoryPool methods, but that is the conceptual equivalent of giving you

template<class T>
T *malloc();

and expecting applications to do Foo *object = malloc<Foo>() instead of Foo *object = new Foo(). That's just not idiomatic C++.

I tend to agree that the existing MemoryPool::alloc method is misleading - being a raw allocator, suitable for making a custom new or Allocator, it should arguably be returning void *, not T *. Although I note that a C++ Allocator's allocate does actually return a pointer, not void *, despite not constructing, so this is consistent with that.

It seems that MemoryPool could be upgraded to a standard C++ Allocator, which would give it some value.

You could write your code like this:

template<class T>
void *operator new(std::size_t count, MemoryPool<T> &pool)
{
    assert(count == sizeof(T));
    return pool.alloc();
}

MemoryPool<Obj> pool;
Obj *object = new (pool) Obj()

Maybe MemoryPool should provide that new overload itself.

Or if you're defining the type Obj yourself, you could include your own custom new which means it automatically gets allocated from the pool whenever anyone uses new Obj.

[Crzyrndm]

Fair call on not using *alloc in a C++ program without some care (and I like the idea of the new overload for the pool, hadn't thought of that). Many of the firmware devs I know are more C-based though so it doesn't have quite the same red flags for everyone (have had reactions that would result in errors ranging from assuming the ctor was run, to mixing C style memset init with a C++ class).

RE: std::allocator/allocator_traits returning a T*, I don't neccesarily disagree, but will point out that they additionally provide construct/destroy (a strong hint that the 4 operations are seperate).

I think this issue has served it's purpose from my end in prompting some conversation. I will be experimenting with a more invasive (i.e. not drop-in) pool allocator interface to see if I can find a middle-ground that is less error-prone and still explicit enough about the memory pool usage (I'm not sure if the "new with allocator" will be as cleanly understood as a function call since it's not seen very often)

[kjbracey]

My suggestion above isn't as elegant as I hoped - I forgot you can't just do delete (pool) object. The syntax isn't symmetrical.

Simplest thing I would have thought for users is to add operator new to whichever class you want. That's the best fit for a memory pool usually, isn't it? Any dynamic allocation of an object should come from its corresponding pool automatically for anyone using plain new Obj.

(Okay, that doesn't cover the cases where there might be multiple pools for one object type, but covers simple one).

Although maybe that's too magic - people tend to assume plain new means "allocate from main heap", not realising it can have class-dependent behaviour.

Anyway, if you want to make a PR to add new constructing and deconstructing methods to MemoryPool, I'm fine with that. The trick will be to give them an obvious name that's more attractive than alloc and free to the casual browser.

[Crzyrndm]

Tthat was my final position as well, the lack of a allocator delete ends up quite ugly (does give you full ctor access without variadics though) so I'm sticking with the function pair (settled on take/release for now, but I think you'd need something better for a PR. I have the option of just removing alloc/free from the API so I don't need great names 👍).

Adding a new operator to the specific type isn't an option for me as the current purpose is a generic thread-thread messaging system along similar lines to rtos::Mail but with an interface that:

• doesn't allow anyone to screw it up (mail doesn't exactly add anything over pool/queue)
• transports only serialised messages (after being shown yet another case of "a Mail instance per message type" because that's the easy way for a prototype. The easy way getting much harder when they want a certain order, or just having added their 10th message type)

tl;dr there are multiple pools all using the same message type

Although maybe that's too magic - people tend to assume plain new means "allocate from main heap", not realising it can have class-dependent behaviour.

Way too hard to communicate across a team. Given I'm using C++14 at work and for this project (an absolute luxury compared to a few years ago...), my rule recently has been "no bare operator new (and definitely no malloc)" (std::make_unique if heap allocation is required). The pool new was obviously not a normal allocation so wasn't a problem, but overriding the types new would be extremely difficult for review/consistency/etc. Just too much of a headache

I won't be opening any PR over this. You've got more constraints than I originally acknowledged and I don't like the asymmetry of operator new / 'osStatus pool_delete(MemoryPool&, T*)' that you need without variadics.