Merge pull request #5344 from Feoramund/fix-2694

Review `core/mem/allocators.odin`

base/runtime/core_builtin.odin

@@ -67,7 +67,7 @@ init_global_temporary_allocator :: proc(size: int, backup_allocator := context.a
 // Prefer the procedure group `copy`.
 @builtin
 copy_slice :: proc "contextless" (dst, src: $T/[]$E) -> int {
-	n := max(0, min(len(dst), len(src)))
+	n := min(len(dst), len(src))
 	if n > 0 {
 		intrinsics.mem_copy(raw_data(dst), raw_data(src), n*size_of(E))
 	}
@@ -80,7 +80,7 @@ copy_slice :: proc "contextless" (dst, src: $T/[]$E) -> int {
 // Prefer the procedure group `copy`.
 @builtin
 copy_from_string :: proc "contextless" (dst: $T/[]$E/u8, src: $S/string) -> int {
-	n := max(0, min(len(dst), len(src)))
+	n := min(len(dst), len(src))
 	if n > 0 {
 		intrinsics.mem_copy(raw_data(dst), raw_data(src), n)
 	}

base/runtime/default_temp_allocator_arena.odin

@@ -1,7 +1,7 @@
 package runtime
 
 import "base:intrinsics"
-import "base:sanitizer"
+// import "base:sanitizer"
 
 DEFAULT_ARENA_GROWING_MINIMUM_BLOCK_SIZE :: uint(DEFAULT_TEMP_ALLOCATOR_BACKING_SIZE)
 
@@ -44,7 +44,7 @@ memory_block_alloc :: proc(allocator: Allocator, capacity: uint, alignment: uint
 	block.base = ([^]byte)(uintptr(block) + base_offset)
 	block.capacity = uint(end - uintptr(block.base))
 
-	sanitizer.address_poison(block.base, block.capacity)
+	// sanitizer.address_poison(block.base, block.capacity)
 
 	// Should be zeroed
 	assert(block.used == 0)
@@ -55,7 +55,7 @@ memory_block_alloc :: proc(allocator: Allocator, capacity: uint, alignment: uint
 memory_block_dealloc :: proc(block_to_free: ^Memory_Block, loc := #caller_location) {
 	if block_to_free != nil {
 		allocator := block_to_free.allocator
-		sanitizer.address_unpoison(block_to_free.base, block_to_free.capacity)
+		// sanitizer.address_unpoison(block_to_free.base, block_to_free.capacity)
 		mem_free(block_to_free, allocator, loc)
 	}
 }
@@ -87,7 +87,7 @@ alloc_from_memory_block :: proc(block: ^Memory_Block, min_size, alignment: uint)
 		return
 	}
 	data = block.base[block.used+alignment_offset:][:min_size]
-	sanitizer.address_unpoison(block.base[block.used:block.used+size])
+	// sanitizer.address_unpoison(block.base[block.used:block.used+size])
 	block.used += size
 	return
 }
@@ -167,7 +167,7 @@ arena_free_all :: proc(arena: ^Arena, loc := #caller_location) {
 	if arena.curr_block != nil {
 		intrinsics.mem_zero(arena.curr_block.base, arena.curr_block.used)
 		arena.curr_block.used = 0
-		sanitizer.address_poison(arena.curr_block.base, arena.curr_block.capacity)
+		// sanitizer.address_poison(arena.curr_block.base, arena.curr_block.capacity)
 	}
 	arena.total_used = 0
 }
@@ -232,7 +232,7 @@ arena_allocator_proc :: proc(allocator_data: rawptr, mode: Allocator_Mode,
 					// grow data in-place, adjusting next allocation
 					block.used = uint(new_end)
 					data = block.base[start:new_end]
-					sanitizer.address_unpoison(data)
+					// sanitizer.address_unpoison(data)
 					return
 				}
 			}
@@ -306,7 +306,7 @@ arena_temp_end :: proc(temp: Arena_Temp, loc := #caller_location) {
 			assert(block.used >= temp.used, "out of order use of arena_temp_end", loc)
 			amount_to_zero := block.used-temp.used
 			intrinsics.mem_zero(block.base[temp.used:], amount_to_zero)
-			sanitizer.address_poison(block.base[temp.used:block.capacity])
+			// sanitizer.address_poison(block.base[temp.used:block.capacity])
 			block.used = temp.used
 			arena.total_used -= amount_to_zero
 		}

core/mem/rollback_stack_allocator.odin

@@ -1,7 +1,7 @@
 package mem
 
 import "base:runtime"
-import "base:sanitizer"
+// import "base:sanitizer"
 
 /*
 Rollback stack default block size.
@@ -134,7 +134,7 @@ rb_free_all :: proc(stack: ^Rollback_Stack) {
 	stack.head.next_block = nil
 	stack.head.last_alloc = nil
 	stack.head.offset = 0
-	sanitizer.address_poison(stack.head.buffer)
+	// sanitizer.address_poison(stack.head.buffer)
 }
 
 /*
@@ -241,7 +241,7 @@ rb_alloc_bytes_non_zeroed :: proc(
 			block.offset = cast(uintptr)len(block.buffer)
 		}
 		res := ptr[:size]
-		sanitizer.address_unpoison(res)
+		// sanitizer.address_unpoison(res)
 		return res, nil
 	}
 	return nil, .Out_Of_Memory
@@ -338,7 +338,7 @@ rb_resize_bytes_non_zeroed :: proc(
 					block.offset += cast(uintptr)size - cast(uintptr)old_size
 				}
 				res := (ptr)[:size]
-				sanitizer.address_unpoison(res)
+				// sanitizer.address_unpoison(res)
 				#no_bounds_check return res, nil
 			}
 		}

core/mem/tlsf/tlsf_internal.odin

@@ -10,7 +10,7 @@
 package mem_tlsf
 
 import "base:intrinsics"
-import "base:sanitizer"
+// import "base:sanitizer"
 import "base:runtime"
 
 // log2 of number of linear subdivisions of block sizes.
@@ -210,7 +210,7 @@ alloc_bytes_non_zeroed :: proc(control: ^Allocator, size: uint, align: uint) ->
 				return nil, .Out_Of_Memory
 			}
 
-			sanitizer.address_poison(new_pool_buf)
+			// sanitizer.address_poison(new_pool_buf)
 
 			// Allocate a new link in the `control.pool` tracking structure.
 			new_pool := new_clone(Pool{
@@ -277,7 +277,7 @@ free_with_size :: proc(control: ^Allocator, ptr: rawptr, size: uint) {
 
 	block := block_from_ptr(ptr)
 	assert(!block_is_free(block), "block already marked as free") // double free
-	sanitizer.address_poison(ptr, block.size)
+	// sanitizer.address_poison(ptr, block.size)
 	block_mark_as_free(block)
 	block = block_merge_prev(control, block)
 	block = block_merge_next(control, block)
@@ -321,7 +321,7 @@ resize :: proc(control: ^Allocator, ptr: rawptr, old_size, new_size: uint, align
 
 	block_trim_used(control, block, adjust)
 	res = ([^]byte)(ptr)[:new_size]
-	sanitizer.address_unpoison(res)
+	// sanitizer.address_unpoison(res)
 
 	if min_size < new_size {
 		to_zero := ([^]byte)(ptr)[min_size:new_size]
@@ -789,7 +789,7 @@ block_prepare_used :: proc(control: ^Allocator, block: ^Block_Header, size: uint
 		block_trim_free(control, block, size)
 		block_mark_as_used(block)
 		res = ([^]byte)(block_to_ptr(block))[:size]
-		sanitizer.address_unpoison(res)
+		// sanitizer.address_unpoison(res)
 	}
 	return
 }

core/mem/virtual/arena.odin

@@ -3,7 +3,7 @@ package mem_virtual
 import "core:mem"
 import "core:sync"
 
-import "base:sanitizer"
+// import "base:sanitizer"
 
 Arena_Kind :: enum uint {
 	Growing = 0, // Chained memory blocks (singly linked list).
@@ -55,7 +55,7 @@ arena_init_growing :: proc(arena: ^Arena, reserved: uint = DEFAULT_ARENA_GROWING
 	if arena.minimum_block_size == 0 {
 		arena.minimum_block_size = reserved
 	}
-	sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
+	// sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
 	return
 }
 
@@ -68,7 +68,7 @@ arena_init_static :: proc(arena: ^Arena, reserved: uint = DEFAULT_ARENA_STATIC_R
 	arena.curr_block     = memory_block_alloc(commit_size, reserved, {}) or_return
 	arena.total_used     = 0
 	arena.total_reserved = arena.curr_block.reserved
-	sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
+	// sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
 	return
 }
 
@@ -82,7 +82,7 @@ arena_init_buffer :: proc(arena: ^Arena, buffer: []byte) -> (err: Allocator_Erro
 
 	arena.kind = .Buffer
 
-	sanitizer.address_poison(buffer[:])
+	// sanitizer.address_poison(buffer[:])
 
 	block_base := raw_data(buffer)
 	block := (^Memory_Block)(block_base)
@@ -163,7 +163,7 @@ arena_alloc :: proc(arena: ^Arena, size: uint, alignment: uint, loc := #caller_l
 		arena.total_used = arena.curr_block.used
 	}
 
-	sanitizer.address_unpoison(data)
+	// sanitizer.address_unpoison(data)
 	return
 }
 
@@ -182,7 +182,7 @@ arena_static_reset_to :: proc(arena: ^Arena, pos: uint, loc := #caller_location)
 			mem.zero_slice(arena.curr_block.base[arena.curr_block.used:][:prev_pos-pos])
 		}
 		arena.total_used = arena.curr_block.used
-		sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
+		// sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
 		return true
 	} else if pos == 0 {
 		arena.total_used = 0
@@ -200,7 +200,7 @@ arena_growing_free_last_memory_block :: proc(arena: ^Arena, loc := #caller_locat
 		arena.total_reserved -= free_block.reserved
 
 		arena.curr_block = free_block.prev
-		sanitizer.address_poison(free_block.base[:free_block.committed])
+		// sanitizer.address_poison(free_block.base[:free_block.committed])
 		memory_block_dealloc(free_block)
 	}
 }
@@ -219,9 +219,9 @@ arena_free_all :: proc(arena: ^Arena, loc := #caller_location) {
 		if arena.curr_block != nil {
 			curr_block_used := int(arena.curr_block.used)
 			arena.curr_block.used = 0
-			sanitizer.address_unpoison(arena.curr_block.base[:curr_block_used])
+			// sanitizer.address_unpoison(arena.curr_block.base[:curr_block_used])
 			mem.zero(arena.curr_block.base, curr_block_used)
-			sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
+			// sanitizer.address_poison(arena.curr_block.base[:arena.curr_block.committed])
 		}
 		arena.total_used = 0
 	case .Static, .Buffer:
@@ -349,7 +349,7 @@ arena_allocator_proc :: proc(allocator_data: rawptr, mode: mem.Allocator_Mode,
 			if size < old_size {
 				// shrink data in-place
 				data = old_data[:size]
-				sanitizer.address_poison(old_data[size:old_size])
+				// sanitizer.address_poison(old_data[size:old_size])
 				return
 			}
 
@@ -363,7 +363,7 @@ arena_allocator_proc :: proc(allocator_data: rawptr, mode: mem.Allocator_Mode,
 					_ = alloc_from_memory_block(block, new_end - old_end, 1, default_commit_size=arena.default_commit_size) or_return
 					arena.total_used += block.used - prev_used
 					data = block.base[start:new_end]
-					sanitizer.address_unpoison(data)
+					// sanitizer.address_unpoison(data)
 					return
 				}
 			}
@@ -374,7 +374,7 @@ arena_allocator_proc :: proc(allocator_data: rawptr, mode: mem.Allocator_Mode,
 			return
 		}
 		copy(new_memory, old_data[:old_size])
-		sanitizer.address_poison(old_data[:old_size])
+		// sanitizer.address_poison(old_data[:old_size])
 		return new_memory, nil
 	case .Query_Features:
 		set := (^mem.Allocator_Mode_Set)(old_memory)

core/mem/virtual/virtual.odin

@@ -2,7 +2,7 @@ package mem_virtual
 
 import "core:mem"
 import "base:intrinsics"
-import "base:sanitizer"
+// import "base:sanitizer"
 import "base:runtime"
 _ :: runtime
 
@@ -22,7 +22,7 @@ reserve :: proc "contextless" (size: uint) -> (data: []byte, err: Allocator_Erro
 
 @(no_sanitize_address)
 commit :: proc "contextless" (data: rawptr, size: uint) -> Allocator_Error {
-	sanitizer.address_unpoison(data, size)
+	// sanitizer.address_unpoison(data, size)
 	return _commit(data, size)
 }
 
@@ -35,13 +35,13 @@ reserve_and_commit :: proc "contextless" (size: uint) -> (data: []byte, err: All
 
 @(no_sanitize_address)
 decommit :: proc "contextless" (data: rawptr, size: uint) {
-	sanitizer.address_poison(data, size)
+	// sanitizer.address_poison(data, size)
 	_decommit(data, size)
 }
 
 @(no_sanitize_address)
 release :: proc "contextless" (data: rawptr, size: uint) {
-	sanitizer.address_unpoison(data, size)
+	// sanitizer.address_unpoison(data, size)
 	_release(data, size)
 }
 
@@ -179,7 +179,7 @@ alloc_from_memory_block :: proc(block: ^Memory_Block, min_size, alignment: uint,
 
 	data = block.base[block.used+alignment_offset:][:min_size]
 	block.used += size
-	sanitizer.address_unpoison(data)
+	// sanitizer.address_unpoison(data)
 	return
 }
 

tests/core/mem/test_core_mem.odin

@@ -193,3 +193,111 @@ fail_if_allocations_overlap :: proc(t: ^testing.T, a, b: []byte) {
 		testing.fail_now(t, "Allocations overlapped")
 	}
 }
+
+
+// This merely does a few simple operations to test basic sanity.
+//
+// A serious test of an allocator would require hooking it up to a benchmark or
+// a large, complicated program in order to get all manner of usage patterns.
+basic_sanity_test :: proc(t: ^testing.T, allocator: mem.Allocator, limit: int, loc := #caller_location) -> bool {
+	context.allocator = allocator
+
+	{
+		a := make([dynamic]u8)
+		for i in 0..<limit {
+			append(&a, u8(i))
+		}
+		testing.expect_value(t, len(a), limit, loc) or_return
+		for i in 0..<limit {
+			testing.expect_value(t, a[i], u8(i), loc) or_return
+		}
+		delete(a)
+	}
+
+	{
+		v := make([]u8, limit)
+		testing.expect_value(t, len(v), limit, loc) or_return
+		for i in 0..<limit {
+			v[i] = u8(i)
+			testing.expect_value(t, v[i], u8(i), loc) or_return
+		}
+		delete(v)
+	}
+
+	{
+		for i in 0..<limit {
+			v := make([]u8, 1)
+			v[0] = u8(i)
+			testing.expect_value(t, v[0], u8(i), loc) or_return
+			delete(v)
+		}
+	}
+
+	return true
+}
+
+@test
+test_scratch :: proc(t: ^testing.T) {
+	N :: 4096
+	sa: mem.Scratch_Allocator
+	mem.scratch_init(&sa, N)
+	defer mem.scratch_destroy(&sa)
+	basic_sanity_test(t, mem.scratch_allocator(&sa), N / 4)
+	basic_sanity_test(t, mem.scratch_allocator(&sa), N / 4)
+}
+
+@test
+test_stack :: proc(t: ^testing.T) {
+	N :: 4096
+	buf: [N]u8
+
+	sa: mem.Stack
+	mem.stack_init(&sa, buf[:])
+	basic_sanity_test(t, mem.stack_allocator(&sa), N / 4)
+	basic_sanity_test(t, mem.stack_allocator(&sa), N / 4)
+}
+
+@test
+test_small_stack :: proc(t: ^testing.T) {
+	N :: 4096
+	buf: [N]u8
+
+	ss: mem.Small_Stack
+	mem.small_stack_init(&ss, buf[:])
+	basic_sanity_test(t, mem.small_stack_allocator(&ss), N / 4)
+	// The test cannot be run a second time on top of the last for a Small
+	// Stack because the dynamic array inside will resize and leave a gap, thus
+	// limiting the amount of space.
+	basic_sanity_test(t, mem.small_stack_allocator(&ss), N / 8)
+}
+
+@test
+test_dynamic_arena :: proc(t: ^testing.T) {
+	da: mem.Dynamic_Arena
+	mem.dynamic_arena_init(&da)
+	defer mem.dynamic_arena_destroy(&da)
+	basic_sanity_test(t, mem.dynamic_arena_allocator(&da), da.block_size / 4)
+	basic_sanity_test(t, mem.dynamic_arena_allocator(&da), da.block_size / 4)
+}
+
+@test
+test_buddy :: proc(t: ^testing.T) {
+	N :: 4096
+	buf: [N]u8
+
+	ba: mem.Buddy_Allocator
+	mem.buddy_allocator_init(&ba, buf[:], align_of(u8))
+	basic_sanity_test(t, mem.buddy_allocator(&ba), N / 8)
+	basic_sanity_test(t, mem.buddy_allocator(&ba), N / 8)
+}
+
+@test
+test_rollback :: proc(t: ^testing.T) {
+	N :: 4096
+	buf: [N]u8
+
+	rb: mem.Rollback_Stack
+	mem.rollback_stack_init(&rb, buf[:])
+	basic_sanity_test(t, mem.rollback_stack_allocator(&rb), N / 8)
+	basic_sanity_test(t, mem.rollback_stack_allocator(&rb), N / 8)
+}

tests/issues/run.bat

@@ -16,6 +16,7 @@ set COMMON=-define:ODIN_TEST_FANCY=false -file -vet -strict-style
 ..\..\..\odin test ..\test_issue_2615.odin %COMMON%  || exit /b
 ..\..\..\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_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

@@ -17,6 +17,7 @@ $ODIN test ../test_issue_2466.odin $COMMON
 $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_4210.odin $COMMON
 $ODIN test ../test_issue_4364.odin $COMMON
 $ODIN test ../test_issue_4584.odin $COMMON

tests/issues/test_issue_2694.odin

@@ -0,0 +1,42 @@
+package test_issues
+
+import "core:fmt"
+import "core:encoding/json"
+import "core:log"
+import "core:mem"
+import "core:testing"
+
+// This is a minimal reproduction of the code in #2694.
+// It exemplifies the original problem as briefly as possible.
+
+SAMPLE_JSON :: `
+{
+	"foo": 0,
+	"things": [
+		{ "a": "ZZZZ"},
+	]
+}
+`
+
+@test
+test_issue_2694 :: proc(t: ^testing.T) {
+	into: struct {
+		foo: int,
+		things: []json.Object,
+	}
+
+	scratch := new(mem.Scratch_Allocator)
+	defer free(scratch)
+	if mem.scratch_allocator_init(scratch, 4 * mem.Megabyte) != .None {
+		log.error("unable to initialize scratch allocator")
+		return
+	}
+	defer mem.scratch_allocator_destroy(scratch)
+
+	err := json.unmarshal_string(SAMPLE_JSON, &into, allocator = mem.scratch_allocator(scratch))
+	testing.expect(t, err == nil)
+
+	output := fmt.tprintf("%v", into)
+	expected := `{foo = 0, things = [map[a="ZZZZ"]]}`
+	testing.expectf(t, output == expected, "\n\texpected: %q\n\tgot:      %q", expected, output)
+}