authorgravatar for alex@alexrp.comAlex Rønne Petersen <alex@alexrp.com> 2026-03-25 18:27:03+01:00
committergravatar for alex@alexrp.comAlex Rønne Petersen <alex@alexrp.com> 2026-03-25 18:27:03+01:00
log8a517285cebb007f14d499c611f3a052f45f0683
treeb335e6d238fa34dbfd2d9c67b77c3902ebdb5198
parent94355f1920d880837823812481140270d0dc631e
parent5363a81a57b669e43fc790b3318e1a02f967eb15

Merge pull request 'std.heap.ArenaAllocator/std.heap.FixedBufferAllocator: fix `end_index` memory ordering' (#31647) from justusk/zig:arena-mem-ord into master

Reviewed-on: https://codeberg.org/ziglang/zig/pulls/31647 Reviewed-by: jacobly <jacobly@noreply.codeberg.org>

2 files changed, 99 insertions(+), 72 deletions(-)

lib/std/heap/ArenaAllocator.zig+61-51
...@@ -207,17 +207,20 @@ pub fn reset(arena: *ArenaAllocator, mode: ResetMode) bool {...@@ -207,17 +207,20 @@ pub fn reset(arena: *ArenaAllocator, mode: ResetMode) bool {
207207
208/// Concurrent accesses to node pointers generally have to have acquire/release208/// Concurrent accesses to node pointers generally have to have acquire/release
209/// semantics to guarantee that newly allocated notes are in a valid state when209/// semantics to guarantee that newly allocated notes are in a valid state when
210/// being inserted into a list. Exceptions are possible, e.g. a CAS loop that210/// being inserted into a list. Exceptions are possible, e.g. a cmpxchg loop that
211/// never accesses the node returned on failure can use monotonic semantics on211/// never accesses the node returned on failure can use monotonic semantics on
212/// failure, but must still use release semantics on success to protect the node212/// failure, but must still use release semantics on success to protect the node
213/// it's trying to push.213/// it's trying to push.
214const Node = struct {214const Node = struct {
215 /// Only meant to be accessed indirectly via the methods supplied by this type,215 /// Only meant to be accessed indirectly via the methods supplied by this type,
216 /// except if the node is owned by the thread accessing it.216 /// except if the node is owned by the thread accessing it.
217 /// Must always be an even number to accomodate `resize` bit.217 /// Must always be an even number to accommodate `resize` bit.
218 size: Size,218 size: Size,
219 /// Concurrent accesses to `end_index` can be monotonic as long as its value219 /// Any increase of `end_index` has to use acquire semantics;
220 /// is compared to a version of `size` before using it to access memory.220 /// any decrease of `end_index` that invalidates (formerly) active allocations
221 /// has to use release semantics.
222 /// This guarantees that all accesses to memory that's about to be freed
223 /// happen-before the free is published.
221 /// Since `size` can only grow and never shrink, memory access depending on224 /// Since `size` can only grow and never shrink, memory access depending on
222 /// any `end_index` <= any `size` can never be OOB.225 /// any `end_index` <= any `size` can never be OOB.
223 end_index: usize,226 end_index: usize,
...@@ -319,11 +322,6 @@ fn pushFreeList(arena: *ArenaAllocator, first: *Node, last: *Node) void {...@@ -319,11 +322,6 @@ fn pushFreeList(arena: *ArenaAllocator, first: *Node, last: *Node) void {
319 }322 }
320}323}
321324
322fn sliceContainsSlice(container: []u8, slice: []u8) bool {
323 return @intFromPtr(slice.ptr) >= @intFromPtr(container.ptr) and
324 @intFromPtr(slice.ptr + slice.len) <= @intFromPtr(container.ptr + container.len);
325}
326
327fn alignedIndex(buf_ptr: [*]u8, end_index: usize, alignment: Alignment) usize {325fn alignedIndex(buf_ptr: [*]u8, end_index: usize, alignment: Alignment) usize {
328 // Wrapping arithmetic to avoid overflows since `end_index` isn't bounded by326 // Wrapping arithmetic to avoid overflows since `end_index` isn't bounded by
329 // `size`. This is always ok since the max alignment in byte units is also327 // `size`. This is always ok since the max alignment in byte units is also
...@@ -357,10 +355,17 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u...@@ -357,10 +355,17 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u
357 // with a single cmpxchg afterwards, which may fail.355 // with a single cmpxchg afterwards, which may fail.
358356
359 const alignable = n + alignment.toByteUnits() - 1;357 const alignable = n + alignment.toByteUnits() - 1;
360 const end_index = @atomicRmw(usize, &node.end_index, .Add, alignable, .monotonic);358 const end_index = @atomicRmw(usize, &node.end_index, .Add, alignable, .acquire); // acquire any memory that may have been freed
361 const aligned_index = alignedIndex(buf.ptr, end_index, alignment);359 const aligned_index = alignedIndex(buf.ptr, end_index, alignment);
362 assert(end_index + alignable >= aligned_index + n);360 assert(end_index + alignable >= aligned_index + n);
363 _ = @cmpxchgStrong(usize, &node.end_index, end_index + alignable, aligned_index + n, .monotonic, .monotonic);361 _ = @cmpxchgStrong(
362 usize,
363 &node.end_index,
364 end_index + alignable,
365 aligned_index + n,
366 .monotonic, // no need to release alignment padding; there's no one accessing it!
367 .monotonic,
368 );
364369
365 if (aligned_index + n > buf.len) break :first_node .{ node, buf.len };370 if (aligned_index + n > buf.len) break :first_node .{ node, buf.len };
366 return buf[aligned_index..][0..n].ptr;371 return buf[aligned_index..][0..n].ptr;
...@@ -382,7 +387,7 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u...@@ -382,7 +387,7 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u
382 const new_size = mem.alignForward(usize, @sizeOf(Node) + aligned_index + n, 2);387 const new_size = mem.alignForward(usize, @sizeOf(Node) + aligned_index + n, 2);
383388
384 if (new_size <= allocated_slice.len) {389 if (new_size <= allocated_slice.len) {
385 // a `resize` or `free` call managed to sneak in and we need to390 // A `resize` or `free` call managed to sneak in and we need to
386 // guarantee that `size` is only ever increased; retry!391 // guarantee that `size` is only ever increased; retry!
387 continue :retry;392 continue :retry;
388 }393 }
...@@ -390,14 +395,16 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u...@@ -390,14 +395,16 @@ fn alloc(ctx: *anyopaque, n: usize, alignment: Alignment, ret_addr: usize) ?[*]u
390 if (arena.child_allocator.rawResize(allocated_slice, .of(Node), new_size, @returnAddress())) {395 if (arena.child_allocator.rawResize(allocated_slice, .of(Node), new_size, @returnAddress())) {
391 size = new_size;396 size = new_size;
392397
393 if (@cmpxchgStrong( // strong because a spurious failure could result in suboptimal usage of this node398 // strong because a spurious failure could result in suboptimal
399 // usage of this node
400 if (null == @cmpxchgStrong(
394 usize,401 usize,
395 &node.end_index,402 &node.end_index,
396 end_index,403 end_index,
397 aligned_index + n,404 aligned_index + n,
405 .acquire, // acquire any memory that may have been freed
398 .monotonic,406 .monotonic,
399 .monotonic,407 )) {
400 ) == null) {
401 const new_buf = allocated_slice.ptr[0..new_size][@sizeOf(Node)..];408 const new_buf = allocated_slice.ptr[0..new_size][@sizeOf(Node)..];
402 return new_buf[aligned_index..][0..n].ptr;409 return new_buf[aligned_index..][0..n].ptr;
403 }410 }
...@@ -548,40 +555,48 @@ fn resize(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, r...@@ -548,40 +555,48 @@ fn resize(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, r
548 assert(new_len > 0);555 assert(new_len > 0);
549556
550 const node = arena.loadFirstNode().?;557 const node = arena.loadFirstNode().?;
551 const buf = node.loadBuf();558 const buf_ptr = @as([*]u8, @ptrCast(node)) + @sizeOf(Node);
552
553 if (!sliceContainsSlice(buf, memory)) {
554 // Not within current node.
555 return new_len <= memory.len;
556 }
557559
558 const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic);560 const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic);
559561
560 if (buf.ptr + cur_end_index != memory.ptr + memory.len) {562 if (buf_ptr + cur_end_index != memory.ptr + memory.len) {
561 // It's not the most recent allocation, so it cannot be expanded.563 // It's not the most recent allocation, so it cannot be expanded,
564 // but it's fine if they want to make it smaller.
562 return new_len <= memory.len;565 return new_len <= memory.len;
563 }566 }
564567
565 const new_end_index: usize = new_end_index: {568 if (new_len <= memory.len) {
566 if (memory.len >= new_len) {569 const new_end_index = cur_end_index - (memory.len - new_len);
567 break :new_end_index cur_end_index - (memory.len - new_len);570 assert(buf_ptr + new_end_index == memory.ptr + new_len);
568 }571
569 if (buf.len - cur_end_index >= new_len - memory.len) {572 _ = @cmpxchgStrong(
570 break :new_end_index cur_end_index + (new_len - memory.len);573 usize,
571 }574 &node.end_index,
572 return false;575 cur_end_index,
573 };576 new_end_index,
574 assert(buf.ptr + new_end_index == memory.ptr + new_len);577 .release, // release freed memory
578 .monotonic,
579 );
580 return true; // Shrinking allocations should always succeed.
581 }
575582
576 return null == @cmpxchgStrong(583 // Saturating arithmetic because `end_index` is not guaranteed to be `<= size`.
577 usize,584 // The allocation we're trying to resize *could* belong to a different node!
578 &node.end_index,585 if (node.loadBuf().len -| cur_end_index >= new_len - memory.len) {
579 cur_end_index,586 const new_end_index = cur_end_index + (new_len - memory.len);
580 new_end_index,587 assert(buf_ptr + new_end_index == memory.ptr + new_len);
581 .monotonic,588
582 .monotonic,589 return null == @cmpxchgStrong(
583 ) or590 usize,
584 new_len <= memory.len; // Shrinking allocations should always succeed.591 &node.end_index,
592 cur_end_index,
593 new_end_index,
594 .acquire, // acquire any memory that may have been freed
595 .monotonic,
596 );
597 }
598
599 return false;
585}600}
586601
587fn remap(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, ret_addr: usize) ?[*]u8 {602fn remap(ctx: *anyopaque, memory: []u8, alignment: Alignment, new_len: usize, ret_addr: usize) ?[*]u8 {
...@@ -596,29 +611,24 @@ fn free(ctx: *anyopaque, memory: []u8, alignment: Alignment, ret_addr: usize) vo...@@ -596,29 +611,24 @@ fn free(ctx: *anyopaque, memory: []u8, alignment: Alignment, ret_addr: usize) vo
596 assert(memory.len > 0);611 assert(memory.len > 0);
597612
598 const node = arena.loadFirstNode().?;613 const node = arena.loadFirstNode().?;
599 const buf = node.loadBuf();614 const buf_ptr = @as([*]u8, @ptrCast(node)) + @sizeOf(Node);
600
601 if (!sliceContainsSlice(buf, memory)) {
602 // Not within current node; we cannot free it.
603 return;
604 }
605615
606 const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic);616 const cur_end_index = @atomicLoad(usize, &node.end_index, .monotonic);
607617
608 if (buf.ptr + cur_end_index != memory.ptr + memory.len) {618 if (buf_ptr + cur_end_index != memory.ptr + memory.len) {
609 // Not the most recent allocation; we cannot free it.619 // Not the most recent allocation; we cannot free it.
610 return;620 return;
611 }621 }
612622
613 const new_end_index = cur_end_index - memory.len;623 const new_end_index = cur_end_index - memory.len;
614 assert(buf.ptr + new_end_index == memory.ptr);624 assert(buf_ptr + new_end_index == memory.ptr);
615625
616 _ = @cmpxchgStrong(626 _ = @cmpxchgStrong(
617 usize,627 usize,
618 &node.end_index,628 &node.end_index,
619 cur_end_index,629 cur_end_index,
620 new_end_index,630 new_end_index,
621 .monotonic,631 .release, // release freed memory
622 .monotonic,632 .monotonic,
623 );633 );
624}634}
lib/std/heap/FixedBufferAllocator.zig+38-21
...@@ -137,7 +137,14 @@ fn threadSafeAlloc(ctx: *anyopaque, n: usize, alignment: mem.Alignment, ret_addr...@@ -137,7 +137,14 @@ fn threadSafeAlloc(ctx: *anyopaque, n: usize, alignment: mem.Alignment, ret_addr
137 const adjusted_index = cur_end_index + adjust_off;137 const adjusted_index = cur_end_index + adjust_off;
138 const new_end_index = adjusted_index + n;138 const new_end_index = adjusted_index + n;
139 if (new_end_index > self.buffer.len) return null;139 if (new_end_index > self.buffer.len) return null;
140 cur_end_index = @cmpxchgWeak(usize, &self.end_index, cur_end_index, new_end_index, .monotonic, .monotonic) orelse140 cur_end_index = @cmpxchgWeak(
141 usize,
142 &self.end_index,
143 cur_end_index,
144 new_end_index,
145 .acquire, // acquire any memory that may have been freed
146 .monotonic,
147 ) orelse
141 return self.buffer[adjusted_index..new_end_index].ptr;148 return self.buffer[adjusted_index..new_end_index].ptr;
142 }149 }
143}150}
...@@ -154,26 +161,36 @@ fn threadSafeResize(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, new...@@ -154,26 +161,36 @@ fn threadSafeResize(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, new
154 return new_len <= memory.len;161 return new_len <= memory.len;
155 }162 }
156163
157 const new_end_index: usize = new_end_index: {164 if (new_len <= memory.len) {
158 if (memory.len >= new_len) {165 const new_end_index = cur_end_index - (memory.len - new_len);
159 break :new_end_index cur_end_index - (memory.len - new_len);166 assert(fba.buffer.ptr + new_end_index == memory.ptr + new_len);
160 }167
161 if (fba.buffer.len - cur_end_index >= new_len - memory.len) {168 _ = @cmpxchgStrong(
162 break :new_end_index cur_end_index + (new_len - memory.len);169 usize,
163 }170 &fba.end_index,
164 return false;171 cur_end_index,
165 };172 new_end_index,
166 assert(fba.buffer.ptr + new_end_index == memory.ptr + new_len);173 .release, // release freed memory
174 .monotonic,
175 );
176 return true; // Shrinking allocations should always succeed.
177 }
167178
168 return null == @cmpxchgStrong(179 if (fba.buffer.len - cur_end_index >= new_len - memory.len) {
169 usize,180 const new_end_index = cur_end_index + (new_len - memory.len);
170 &fba.end_index,181 assert(fba.buffer.ptr + new_end_index == memory.ptr + new_len);
171 cur_end_index,182
172 new_end_index,183 return null == @cmpxchgStrong(
173 .monotonic,184 usize,
174 .monotonic,185 &fba.end_index,
175 ) or186 cur_end_index,
176 new_len <= memory.len; // Shrinking allocations should always succeed.187 new_end_index,
188 .acquire, // acquire any memory that may have been freed
189 .monotonic,
190 );
191 }
192
193 return false;
177}194}
178195
179fn threadSafeRemap(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, new_len: usize, ret_addr: usize) ?[*]u8 {196fn threadSafeRemap(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, new_len: usize, ret_addr: usize) ?[*]u8 {
...@@ -201,7 +218,7 @@ fn threadSafeFree(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, ret_a...@@ -201,7 +218,7 @@ fn threadSafeFree(ctx: *anyopaque, memory: []u8, alignment: mem.Alignment, ret_a
201 &fba.end_index,218 &fba.end_index,
202 cur_end_index,219 cur_end_index,
203 new_end_index,220 new_end_index,
204 .monotonic,221 .release, // release freed memory
205 .monotonic,222 .monotonic,
206 );223 );
207}224}