Merge pull request #5369 from Feoramund/fix-3435

Fix #3435

.gitignore

@@ -277,6 +277,7 @@ odin
 *.bin
 demo.bin
 libLLVM*.so*
+*.a
 
 # shared collection
 shared/

check_all.bat

@@ -1,9 +1,9 @@
 @echo off
 
 if "%1" == "" (
-	echo Checking darwin_amd64 - expect vendor:cgtlf panic
+	echo Checking darwin_amd64 - expect vendor:cgltf panic
 	odin check examples\all -vet -vet-tabs -strict-style -vet-style -warnings-as-errors -disallow-do -target:darwin_amd64
-	echo Checking darwin_arm64 - expect vendor:cgtlf panic
+	echo Checking darwin_arm64 - expect vendor:cgltf panic
 	odin check examples\all -vet -vet-tabs -strict-style -vet-style -warnings-as-errors -disallow-do -target:darwin_arm64
 	echo Checking linux_i386
 	odin check examples\all -vet -vet-tabs -strict-style -vet-style -warnings-as-errors -disallow-do -target:linux_i386

check_all.sh

@@ -45,9 +45,9 @@ wasm)
 	;;
 
 *)
-	echo Checking darwin_amd64 - expect vendor:cgtlf panic
+	echo Checking darwin_amd64 - expect vendor:cgltf panic
 	odin check examples/all -vet -vet-tabs -strict-style -vet-style -warnings-as-errors -disallow-do -target:darwin_amd64
-	echo Checking darwin_arm64 - expect vendor:cgtlf panic
+	echo Checking darwin_arm64 - expect vendor:cgltf panic
 	odin check examples/all -vet -vet-tabs -strict-style -vet-style -warnings-as-errors -disallow-do -target:darwin_arm64
 	echo Checking linux_i386
 	odin check examples/all -vet -vet-tabs -strict-style -vet-style -warnings-as-errors -disallow-do -target:linux_i386

core/mem/allocators.odin

@@ -2239,6 +2239,7 @@ buddy_allocator_init :: proc(b: ^Buddy_Allocator, data: []byte, alignment: uint,
 	b.head.is_free = true
 	b.tail = buddy_block_next(b.head)
 	b.alignment = alignment
+	assert(uint(len(data)) >= 2 * buddy_block_size_required(b, 1), "The size of the backing buffer must be large enough to hold at least two 1-byte allocations given the alignment requirements, otherwise it cannot split.", loc)
 	// sanitizer.address_poison(data)
 }
 
@@ -2247,12 +2248,14 @@ Get required block size to fit in the allocation as well as the alignment paddin
 */
 @(require_results)
 buddy_block_size_required :: proc(b: ^Buddy_Allocator, size: uint) -> uint {
-	size := size
-	actual_size := b.alignment
-	size += size_of(Buddy_Block)
-	size = align_forward_uint(size, b.alignment)
-	for size > actual_size {
-		actual_size <<= 1
+	assert(size > 0)
+	// NOTE: `size_of(Buddy_Block)` will be accounted for in `b.alignment`.
+	// This calculation is also previously guarded against being given a `size`
+	// 0 by `buddy_allocator_alloc_bytes_non_zeroed` checking for that.
+	actual_size := b.alignment + size
+	if intrinsics.count_ones(actual_size) != 1 {
+		// We're not a power of two. Let's fix that.
+		actual_size = 1 << (size_of(uint) * 8 - intrinsics.count_leading_zeros(actual_size))
 	}
 	return actual_size
 }
@@ -2315,7 +2318,7 @@ buddy_allocator_alloc_bytes_non_zeroed :: proc(b: ^Buddy_Allocator, size: uint)
 	if size != 0 {
 		actual_size := buddy_block_size_required(b, size)
 		found := buddy_block_find_best(b.head, b.tail, actual_size)
-		if found != nil {
+		if found == nil {
 			// Try to coalesce all the free buddy blocks and then search again
 			buddy_block_coalescence(b.head, b.tail)
 			found = buddy_block_find_best(b.head, b.tail, actual_size)

tests/issues/run.bat

@@ -17,6 +17,7 @@ set COMMON=-define:ODIN_TEST_FANCY=false -file -vet -strict-style
 ..\..\..\odin test ..\test_issue_2637.odin %COMMON%  || exit /b
 ..\..\..\odin test ..\test_issue_2666.odin %COMMON%  || exit /b
 ..\..\..\odin test ..\test_issue_2694.odin %COMMON%  || exit /b
+..\..\..\odin test ..\test_issue_3435.odin %COMMON%  || exit /b
 ..\..\..\odin test ..\test_issue_4210.odin %COMMON%  || exit /b
 ..\..\..\odin test ..\test_issue_4364.odin %COMMON%  || exit /b
 ..\..\..\odin test ..\test_issue_4584.odin %COMMON%  || exit /b

tests/issues/run.sh

@@ -18,6 +18,7 @@ $ODIN test ../test_issue_2615.odin $COMMON
 $ODIN test ../test_issue_2637.odin $COMMON
 $ODIN test ../test_issue_2666.odin $COMMON
 $ODIN test ../test_issue_2694.odin $COMMON
+$ODIN test ../test_issue_3435.odin $COMMON
 $ODIN test ../test_issue_4210.odin $COMMON
 $ODIN test ../test_issue_4364.odin $COMMON
 $ODIN test ../test_issue_4584.odin $COMMON

tests/issues/test_issue_3435.odin

@@ -0,0 +1,38 @@
+package main
+
+import "base:runtime"
+import "core:mem"
+import "core:testing"
+import "core:time"
+
+@test
+test_issue_3435 :: proc(t: ^testing.T) {
+	testing.set_fail_timeout(t, time.Second)
+	allocator: mem.Buddy_Allocator
+	data := runtime.make_aligned([]byte, 64, 32)
+	defer delete(data)
+
+	// mem.buddy_allocator_init(&allocator, data, 32)
+
+	// Bypass the assertion that would normally keep this from happening by
+	// manually putting the allocator together.
+	allocator.head = cast(^mem.Buddy_Block)raw_data(data)
+	allocator.head.size = len(data)
+	allocator.head.is_free = true
+	allocator.tail = mem.buddy_block_next(allocator.head)
+	allocator.alignment = 32
+
+	context.allocator = mem.buddy_allocator(&allocator)
+
+	// Three allocations in the space above is all that's needed to reproduce
+	// the bug seen in #3435; this is the most minimal reproduction possible.
+	a := make([]u8, 1)
+	testing.expect(t, len(a) == 1)
+	b := make([]u8, 1)
+	testing.expect(t, len(b) == 0)
+	c := make([]u8, 1)
+	testing.expect(t, len(c) == 0)
+
+	// With the bugfix in place, the allocator should be sensible enough to not
+	// fall into an infinite loop anymore, even if the assertion is disabled.
+}