Possible bug in using a stateful custom allocator in phmap::node_hash_map

[dipanshu-grc]

Hi, was trying to use a stateful custom allocator in phmap::node_hash_map.
I am segmentation faults, possible because of memleaks and also compiler warning -
phmap_base.h:4160:10: error: function may return address of local variable [-Werror=return-local-addr] 4160 | return p;

On looking at
template <size_t Alignment, class Alloc> void* Allocate(Alloc* alloc, size_t n) { static_assert(Alignment > 0, ""); assert(n && "n must be positive"); struct alignas(Alignment) M {}; using A = typename phmap::allocator_traits<Alloc>::template rebind_alloc<M>; using AT = typename phmap::allocator_traits<Alloc>::template rebind_traits<M>; **A mem_alloc(*alloc);** void* p = &*AT::allocate(mem_alloc, (n + sizeof(M) - 1) / sizeof(M)); assert(reinterpret_cast<uintptr_t>(p) % Alignment == 0 && "allocator does not respect alignment"); return p; }

Is there a reason to create a local copy mem_alloc? This seems to be an issue in my case. Attaching my custom allocator as reference

`
template <typename T, std::size_t Capacity = 1024>
class FixedAllocator {
public:
using value_type = T;

FixedAllocator() noexcept {
    init();
}


template <typename U>
FixedAllocator(const FixedAllocator<U, Capacity>&) 
{
    init();
}

void init()
{
    static_assert(sizeof(T) >= sizeof(void*), "Type size must be >= pointer size.");
    static_assert(std::is_trivially_destructible<T>::value, "This allocator does not call destructors. Use only with trivially destructible types.");

    char* start = reinterpret_cast<char*>(buffer_);
    for (std::size_t i = 0; i < Capacity - 1; ++i) {
        void* curr = start + i * sizeof(T);
        void* next = start + (i + 1) * sizeof(T);
        *reinterpret_cast<void**>(curr) = next;
    }
    std::cout << "Fixed Buffer init" << std::endl;
    *reinterpret_cast<void**>(start + (Capacity - 1) * sizeof(T)) = nullptr;
    freeList_ = buffer_;

}
T* allocate(std::size_t n) {
    if (n == 1) {
        if (!freeList_)
            throw std::bad_alloc();
        void* block = freeList_;
        std::cout << "Block used " << n << " " << sizeof(T) << std::endl;
        // mutable_universe.error_log.writeln(forward_as_tuple("Block used", n, sizeof(T), block));
        freeList_ = *reinterpret_cast<void**>(freeList_);
        return static_cast<T*>(block);
    } else {
        // Fallback to global new
        std::cout << "Fallback alloc " << n <<" " << sizeof(T) << std::endl;
        // mutable_universe.error_log.writeln(forward_as_tuple("Fallback allocation", n, sizeof(T)));
        return static_cast<T*>(::operator new(n * sizeof(T)));
    }
}

// Deallocate memory for n elements
void deallocate(T* ptr, std::size_t n) noexcept {
    if (n == 1) {
        *reinterpret_cast<void**>(ptr) = freeList_;
        freeList_ = ptr;
        std::cout << "Block freed " << n << " " << sizeof(T) << ptr << std::endl; 
        // mutable_universe.error_log.writeln(forward_as_tuple("Block freed", n, sizeof(T)), ptr);
    } else {
        // Fallback to global delete
        std::cout << "Fallback free " << std::endl;
        // mutable_universe.error_log.writeln(forward_as_tuple("Fallback Deallocation", n, sizeof(T)));
        ::operator delete(ptr);
    }
}

template <typename U>
struct rebind {
    using other = FixedAllocator<U, Capacity>;
};

private:
alignas(alignof(T)) char buffer_[Capacity * sizeof(T)];
void* freeList_ = nullptr;
};
`

[greg7mdp]

Yes, the issue is that the hash_map is provided an allocator for the map's value_type, but we try to allocate other types with it.
In the case of your allocator, it wouldn't help with phmap as we would never fall in the case if (n == 1), since we alwats store the values in large atrrays of value_type.

[dipanshu-grc]

Thanks for the quick reply.

but we try to allocate other types with it

rebind() would take care of that, right. Any reason for creating an allocator instance for other type inside every Allocate() call, and not once in init? I could be totally off here, don't have much experience with allocators and neithere phmap implementation.

phmap as we would never fall in the case if (n == 1), since we alwats store the values in large atrrays of value_type

Got it. Just mentioning the problem I am trying to solve for my usecase, would really appeciate your input -
In most of the cases, the number of keys in my usecase is bounded by a fixed value, with frequent insert, delete, lookups. I want to avoid any kind of dynamic memory allocation overhead, and would want the container to handle the cases when #keys exceed that fixed value as well.