Problem with `min_block_size`
jwdevel opened this issue · 3 comments
The calculation for memory_pool::min_block_size
seems wrong in some cases.
Here is a failing test I added to test/memory_pool.cpp
:
#include "container.hpp"
#include "smart_ptr.hpp"
// ...
TEST_CASE("memory_pool min_block_size", "[pool]") {
using pool_type = memory_pool<>;
struct MyObj {
float value;
char data[100];
};
CAPTURE(alignof(MyObj));
auto nodesize = allocate_shared_node_size<MyObj, pool_type>::value;
CAPTURE(nodesize);
// Make a pool with the smallest block size possible.
auto minblock = pool_type::min_block_size(nodesize, 1);
CAPTURE(minblock);
pool_type pool(nodesize, minblock);
auto ptr = allocate_shared<MyObj>(pool);
}
This fails with:
[foonathan::memory] Assertion failure in function insert_impl (.../foonathan-memory/src/detail/free_list.cpp:536): Assertion "no_nodes > 0" failed.
.../foonathan-memory/test/memory_pool.cpp:71: FAILED:
due to a fatal error condition:
alignof(MyObj) := 4
nodesize := 136
minblock := 184
SIGABRT - Abort (abnormal termination) signal
This seems to be due to ordered_free_memory_list::insert_impl
using actual_size = node_size + 2 * fence_size()
, but min_block_size
does not do the same calculation. Specifically, ordered_free_memory_list::fence_size()
gives node_size_
as the fence size. I don't fully understand this, but it definitely seems to be on purpose.
I'm happy to work on a fix for this, but I wonder if you have a recommended approach.
My initial idea is to somehow expose the "actual_size" computation, so it is accessible via memory_pool::pool_type::XXX
, but interested in other ideas.
The calculation for memory_pool::min_block_size seems wrong in some cases.
Good catch, thanks. This was probably exposed by #105.
This seems to be due to ordered_free_memory_list::insert_impl using actual_size = node_size + 2 * fence_size(), but min_block_size does not do the same calculation. Specifically, ordered_free_memory_list::fence_size() gives node_size_ as the fence size. I don't fully understand this, but it definitely seems to be on purpose.
This is on purpose to catch buffer overflow in debug mode yes.
I'm happy to work on a fix for this, but I wonder if you have a recommended approach.
My initial idea is to somehow expose the "actual_size" computation, so it is accessible via memory_pool::pool_type::XXX, but interested in other ideas.
Adding a constexpr std::size_t actual_node_size()
to the pool type seems reasonable, yes. Can you do a PR?
Sure thing, will take a stab a it.
Should be fixed now. For simplicity (and to get rid of lots of potential bugs, if there are any), I've just removed debug fences from the pools. If needed, I can add them back later.