Skip to content

Document that the ptr field of Allocator/Random should not be compared and remove existing comparison #22691

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Document that the ptr field of Allocator/Random should not be compared and remove existing comparison #22691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b46f994
process.Child.collectOutput: Switch to Allocator/ArrayListUnmanaged
squeek502 Jan 31, 2025
4041cc0
Allocator/Random: document that comparing `ptr` may result in illegal…
squeek502 Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/std/Random.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ pub const RomuTrio = @import("Random/RomuTrio.zig");
pub const SplitMix64 = @import("Random/SplitMix64.zig");
pub const ziggurat = @import("Random/ziggurat.zig");

/// Any comparison of this field may result in illegal behavior, since it may be set to
/// `undefined` in cases where the random implementation does not have any associated
/// state.
ptr: *anyopaque,
fillFn: *const fn (ptr: *anyopaque, buf: []u8) void,

Expand Down
5 changes: 4 additions & 1 deletion lib/std/mem/Allocator.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ const builtin = @import("builtin");
pub const Error = error{OutOfMemory};
pub const Log2Align = math.Log2Int(usize);

// The type erased pointer to the allocator implementation
/// The type erased pointer to the allocator implementation.
/// Any comparison of this field may result in illegal behavior, since it may be set to
/// `undefined` in cases where the allocator implementation does not have any associated
/// state.
ptr: *anyopaque,
vtable: *const VTable,

Expand Down
53 changes: 24 additions & 29 deletions lib/std/process/Child.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,15 +344,17 @@ pub const RunResult = struct {
stderr: []u8,
};

fn fifoToOwnedArrayList(fifo: *std.io.PollFifo) std.ArrayList(u8) {
fn writeFifoDataToArrayList(allocator: Allocator, list: *std.ArrayListUnmanaged(u8), fifo: *std.io.PollFifo) !void {
if (fifo.head != 0) fifo.realign();
const result = std.ArrayList(u8){
.items = fifo.buf[0..fifo.count],
.capacity = fifo.buf.len,
.allocator = fifo.allocator,
};
fifo.* = std.io.PollFifo.init(fifo.allocator);
return result;
if (list.capacity == 0) {
list.* = .{
.items = fifo.buf[0..fifo.count],
.capacity = fifo.buf.len,
};
fifo.* = std.io.PollFifo.init(fifo.allocator);
} else {
try list.appendSlice(allocator, fifo.buf[0..fifo.count]);
}
}

/// Collect the output from the process's stdout and stderr. Will return once all output
Expand All @@ -362,21 +364,16 @@ fn fifoToOwnedArrayList(fifo: *std.io.PollFifo) std.ArrayList(u8) {
/// The process must be started with stdout_behavior and stderr_behavior == .Pipe
pub fn collectOutput(
child: ChildProcess,
stdout: *std.ArrayList(u8),
stderr: *std.ArrayList(u8),
/// Used for `stdout` and `stderr`.
allocator: Allocator,
Copy link
Member

Choose a reason for hiding this comment

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

Since this is going to break some people's code, let's put a little effort into making it more future-proof. My suggestion also happens to be more familiar to existing callsites.

  • Make them both std.ArrayListUnmanaged
  • Make the function only append to them, not clear them

Sure, this implementation uses that fifo abstraction but that's not necessarily how it has to work. And append-only array list is generally a very flexible API from the caller's perspective.

I'll note also that this patch introduces more error handling that can be deleted with the above suggestion.

Copy link
Collaborator Author

@squeek502 squeek502 Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Pushed a simple version of this API while I look into improving the implementation (the version I just pushed still uses std.io.poll and then just calls appendSlice to copy the data from the FIFO to the ArrayListUnmanaged).

Unsure how long it'll take to write the improved version; one option would be to leave improving the implementation as a follow-up issue.

Copy link
Collaborator Author

@squeek502 squeek502 Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Pushed an attempt at making std.io.Poller not FIFO-specific and then using the ArrayListUnmanageds directly here:

squeek502@b8c7327

Let me know if it looks like I'm vaguely on the right track and I'll add it to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't look at your branch yet, I'll try to get to that later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No worries, it's only a minor improvement after the list.capacity == 0 optimization from #22691 (comment)

stdout: *std.ArrayListUnmanaged(u8),
stderr: *std.ArrayListUnmanaged(u8),
max_output_bytes: usize,
) !void {
assert(child.stdout_behavior == .Pipe);
assert(child.stderr_behavior == .Pipe);

// we could make this work with multiple allocators but YAGNI
if (stdout.allocator.ptr != stderr.allocator.ptr or
stdout.allocator.vtable != stderr.allocator.vtable)
{
unreachable; // ChildProcess.collectOutput only supports 1 allocator
}

var poller = std.io.poll(stdout.allocator, enum { stdout, stderr }, .{
var poller = std.io.poll(allocator, enum { stdout, stderr }, .{
.stdout = child.stdout.?,
.stderr = child.stderr.?,
});
Expand All @@ -389,8 +386,8 @@ pub fn collectOutput(
return error.StderrStreamTooLong;
}

stdout.* = fifoToOwnedArrayList(poller.fifo(.stdout));
stderr.* = fifoToOwnedArrayList(poller.fifo(.stderr));
try writeFifoDataToArrayList(allocator, stdout, poller.fifo(.stdout));
try writeFifoDataToArrayList(allocator, stderr, poller.fifo(.stderr));
}

pub const RunError = posix.GetCwdError || posix.ReadError || SpawnError || posix.PollError || error{
Expand Down Expand Up @@ -420,22 +417,20 @@ pub fn run(args: struct {
child.expand_arg0 = args.expand_arg0;
child.progress_node = args.progress_node;

var stdout = std.ArrayList(u8).init(args.allocator);
var stderr = std.ArrayList(u8).init(args.allocator);
errdefer {
stdout.deinit();
stderr.deinit();
}
var stdout: std.ArrayListUnmanaged(u8) = .empty;
errdefer stdout.deinit(args.allocator);
var stderr: std.ArrayListUnmanaged(u8) = .empty;
errdefer stderr.deinit(args.allocator);

try child.spawn();
errdefer {
_ = child.kill() catch {};
}
try child.collectOutput(&stdout, &stderr, args.max_output_bytes);
try child.collectOutput(args.allocator, &stdout, &stderr, args.max_output_bytes);

return RunResult{
.stdout = try stdout.toOwnedSlice(),
.stderr = try stderr.toOwnedSlice(),
.stdout = try stdout.toOwnedSlice(args.allocator),
.stderr = try stderr.toOwnedSlice(args.allocator),
.term = try child.wait(),
};
}
Expand Down
Loading