Skip to content

std: fix sentinel handling in Allocator interface #23023

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

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 [email protected]
References: #19984
References: #22706
References: #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().

@alexrp
Copy link
Member

alexrp commented Mar 26, 2025

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

Is that the only thing blocking this PR? If so, I can go ahead and do an update since I'd also like to start using @disableIntrinsics() in some compiler-rt and std code.

@ifreund
Copy link
Member Author

ifreund commented Mar 26, 2025

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

Is that the only thing blocking this PR? If so, I can go ahead and do an update since I'd also like to start using @disableIntrinsics() in some compiler-rt and std code.

Indeed, the wasm blob update is the only blocker I'm aware of. (Though it seems I also have a conflict to resolve)

@alexrp
Copy link
Member

alexrp commented Mar 27, 2025

@ifreund #23370 is in master now.

ifreund added 2 commits March 27, 2025 21:49
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 <[email protected]>
References: ziglang#19984
References: ziglang#22706
References: ziglang#23020
@ifreund
Copy link
Member Author

ifreund commented Mar 27, 2025

@ifreund #23370 is in master now.

Thanks, rebased!

@alexrp
Copy link
Member

alexrp commented Apr 15, 2025

The new CI failures look related FWIW.

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.

4 participants