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

Fix GeneralPurposeAllocator crash when deallocating metadata #20000

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Frojdholm
Copy link
Contributor

@Frojdholm Frojdholm commented May 19, 2024

Fixes #19977

This makes the following test not crash

const std = @import("std");

test "retain metadata and never unmap" {
    var gpa = std.heap.GeneralPurposeAllocator(.{
        .safety = true,
        .never_unmap = true,
        .retain_metadata = true,
    }){};
    defer std.debug.assert(gpa.deinit() == .ok);
    const allocator = gpa.allocator();

    const alloc = try allocator.alloc(u8, 8);
    allocator.free(alloc);

    const alloc2 = try allocator.alloc(u8, 8);
    allocator.free(alloc2);
}

The getMin/getMax -> remove pattern could be encapsulated in a function popMin/popMax that return the removed key.

@Frojdholm Frojdholm force-pushed the fix-gpa-crash-when-deallocating-metadata branch from 063732f to db1b540 Compare May 19, 2024 21:43
@Frojdholm Frojdholm marked this pull request as ready for review May 19, 2024 21:52
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test case. Can it be added to the file as a unit test?

@Frojdholm
Copy link
Contributor Author

Nice test case. Can it be added to the file as a unit test?

Sure, added 👍

However, I was a bit confused that the "double frees" test wasn't already failing since it's using the same setup, but with a backing allocator of another GPA.

@Frojdholm Frojdholm requested a review from andrewrk May 20, 2024 16:36
@Frojdholm Frojdholm force-pushed the fix-gpa-crash-when-deallocating-metadata branch from 4591066 to d526a2c Compare May 21, 2024 17:09
@Frojdholm
Copy link
Contributor Author

Test failure was a flaky windows test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GeneralPurposeAllocator: assertion failure when retain_metadata and never_unmap are used
2 participants