Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Allocator issues in segmented_vector #104

Open
sh-mochi opened this issue Dec 21, 2023 · 0 comments
Open

[BUG] Allocator issues in segmented_vector #104

sh-mochi opened this issue Dec 21, 2023 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@sh-mochi
Copy link

auto operator=(segmented_vector&& other) noexcept -> segmented_vector& checks for allocator equality and if allocators are not equal the array is basically rebuilt (with elements moved individually). This is from the current code base:

auto operator=(segmented_vector&& other) noexcept -> segmented_vector& {
    clear();
    dealloc();
    if (other.get_allocator() == get_allocator()) {
        m_blocks = std::move(other.m_blocks);
        m_size = std::exchange(other.m_size, {});
    } else {
        // make sure to construct with other's allocator!
        m_blocks = std::vector<pointer, vec_alloc>(vec_alloc(other.get_allocator()));
        append_everything_from(std::move(other));
    }
    return *this;
}

Here append_everything_from(std::move(other)); potentially allocates memory and any OOM situation immediately crashes straight into a noexcept boundary. There is also some twisted logic going on. After

// make sure to construct with other's allocator!
m_blocks = std::vector<pointer, vec_alloc>(vec_alloc(other.get_allocator()));

the condition other.get_allocator() == get_allocator() would always be true, right? Therefore, after

segmented_vector(segmented_vector&& other, Allocator alloc)
    : segmented_vector(alloc) {
    *this = std::move(other);
}

the contaner will always end up with other's allocator (regardless of whether alloc==other.get_allocator()). There is still a silly edge case in the operator= method I guess: While std::vector<pointer, vec_alloc>(vec_alloc(other.get_allocator())) actually has an allocator that compares equal to other.get_allocator(), it is unclear if this allocator will be propagated through the assignment which depends on pocma, so you can still end up with m_blocks.get_allocator()!=other.m_blocks.get_allocator() after m_blocks = std::vector<pointer, vec_alloc>(vec_alloc(other.get_allocator()));. Propose fix:

auto operator=(segmented_vector&& other) noexcept -> segmented_vector& {
    static_assert(vec_alloc has pocma==true);
    clear();
    dealloc();
    m_blocks = std::move(other.m_blocks); // without pocma m_block could end up with an incompatible allocator
    m_size = std::exchange(other.m_size, {});
    return *this;
}

Here are some other issues regarding allocators that should be rather easy to fix:

Allocator is not propagated as it should be.

segmented_vector(segmented_vector const& other) { // missing initializer :m_blocks(other.m_blocks.get_allocator()) {
    append_everything_from(other);
}

Doesn't propagate the allocator of other:

auto operator=(segmented_vector const& other) -> segmented_vector& {
    if (this == &other) {
        return *this;
    }
    clear();
    append_everything_from(other);
    return *this;
}

This looks off - isn't this a use before init of get_allocator()?

segmented_vector(segmented_vector&& other) noexcept
    : segmented_vector(std::move(other), get_allocator()) {}

When it comes to the excellent (cough) allocator requirements, maybe an honest

static_assert(pocca && pocma && pocs, "Go away!")

would go a long way.

@sh-mochi sh-mochi added the bug Something isn't working label Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants