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

std: fix sentinel handling in Allocator interface #23023

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

Conversation

ifreund
Copy link
Member

@ifreund ifreund commented Feb 27, 2025

Currently the only function that handles sentinel terminated slices
properly is free. All other uses of mem.sliceAsBytes() in the allocator
interface lack proper handling of a possible sentinel.

This commit changes the Allocator interface to use @ptrCast() plus
the new mem.absorbSentinel() instead.

This also makes incorrectly passing a pointer to array to
Allocator.free() a compile error. The proper function to free a pointer
to an array is Allocator.destroy().

Reported-by: David Vanderson david.vanderson@gmail.com
References: #19984
References: #22706
References: #23020

This will allow replacing (currently buggy) mem.sliceAsBytes() usage in
the mem.Allocator implementation with, for example:

const bytes: []u8 = @constcast(@ptrCast(mem.absorbSentinel(allocation)));

References: ziglang#22706
Currently the only function that handles sentinel terminated slices
properly is free. All other uses of mem.sliceAsBytes() in the allocator
interface lack proper handling of a possible sentinel.

This commit changes the Allocator interface to use @ptrCast() plus
the new mem.absorbSentinel() instead.

This also makes incorrectly passing a pointer to array to
Allocator.free() a compile error. The proper function to free a pointer
to an array is Allocator.destroy().

Reported-by: David Vanderson <david.vanderson@gmail.com>
References: ziglang#19984
References: ziglang#22706
References: ziglang#23020
@ifreund
Copy link
Member Author

ifreund commented Feb 27, 2025

Ah, it looks like zig1.wasm hasn't been updated with the new @ptrCast() features yet.

@Fri3dNstuff
Copy link
Contributor

Should std.mem.absorbSentinel keep the mutability of pointers? A returned mutable slice allows accessing the position of, and overriding the sentinel - which is probably unwanted...
Though, the code below works, and prints { 1, 1, 1, 4 }, so maybe it's not that big a deal? (or even a positive)

pub fn main() !void {
    var a: [3:0]u8 = @splat(1);
    a[3] = 4;
    std.debug.print("{any}\n", .{a[0..4]});
}

@castholm
Copy link
Contributor

Is forbidding passing pointer-to-arrays to Allocator.free() (#23020 (comment)) really the correct resolution for #23019?

Language design-wise, a *[n]T is treated as a specialized []T with a comptime-known length and the language goes great lengths to make using *[n]T in place of []T painless via coercion and synthesizing the len field, which helps avoid issues where bytes[0..end] may return a pointer-to-array or a slice depending on the comptime-knownness of end.

Allocator.free() defines the parameter as memory: anytype because there's no other viable way of expressing "any slice with any qualifiers" with the current type system. Had it been typed as memory: []T (disregarding mismatching qualifiers) you would have been able to pass *[n]T to it without issue.

@ifreund
Copy link
Member Author

ifreund commented Feb 28, 2025

Should std.mem.absorbSentinel keep the mutability of pointers? A returned mutable slice allows accessing the position of, and overriding the sentinel - which is probably unwanted...

On the contrary, this is intentional behavior.

Is forbidding passing pointer-to-arrays to Allocator.free() (#23020 (comment)) really the correct resolution for #23019?

In my mind, the answer of whether Allocator.destroy() or Allocator.free() should be used for a given allocation is extremely straightforward. If the allocation is created using Allocator.create(), use Allocator.destroy(). If the allocation is created with Allocator.alloc(), use Allocator.free(). This is well documented in the doc comments.

A pointer to an array can only be directly allocated with create(), therefore it is incorrect for free() to support freeing such a pointer. If, for example, the user casts the slice returned by alloc() into a pointer to an array, I don't think it is unreasonable for them to need to cast it back to the exact slice returned by alloc() in order to use free().

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.

3 participants