From 6024af172c7dc3bea325a760d8198ff046b9e975 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Mon, 18 Dec 2023 14:40:49 +0100 Subject: [PATCH 1/6] add failing test for runtime arena edge case --- tests/core/Makefile | 6 ++- tests/core/build.bat | 5 ++ tests/core/runtime/test_core_runtime.odin | 61 +++++++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 tests/core/runtime/test_core_runtime.odin diff --git a/tests/core/Makefile b/tests/core/Makefile index 6f44b6fc4..3af78b55b 100644 --- a/tests/core/Makefile +++ b/tests/core/Makefile @@ -20,7 +20,8 @@ all: c_libc_test \ reflect_test \ slice_test \ strings_test \ - thread_test + thread_test \ + runtime_test download_test_assets: $(PYTHON) download_assets.py @@ -84,3 +85,6 @@ fmt_test: thread_test: $(ODIN) run thread -out:test_core_thread + +runtime_test: + $(ODIN) run runtime -out:test_core_runtime diff --git a/tests/core/build.bat b/tests/core/build.bat index 54ee5bee6..d0248b813 100644 --- a/tests/core/build.bat +++ b/tests/core/build.bat @@ -95,3 +95,8 @@ echo --- echo Running core:thread tests echo --- %PATH_TO_ODIN% run thread %COMMON% %COLLECTION% -out:test_core_thread.exe || exit /b + +echo --- +echo Running core:runtime tests +echo --- +%PATH_TO_ODIN% run runtime %COMMON% %COLLECTION% -out:test_core_runtime.exe || exit /b diff --git a/tests/core/runtime/test_core_runtime.odin b/tests/core/runtime/test_core_runtime.odin new file mode 100644 index 000000000..5a57be4c3 --- /dev/null +++ b/tests/core/runtime/test_core_runtime.odin @@ -0,0 +1,61 @@ +package test_core_runtime + +import "core:fmt" +import "core:intrinsics" +import "core:mem" +import "core:os" +import "core:reflect" +import "core:runtime" +import "core:testing" + +TEST_count := 0 +TEST_fail := 0 + +when ODIN_TEST { + expect_value :: testing.expect_value +} else { + expect_value :: proc(t: ^testing.T, value, expected: $T, loc := #caller_location) -> bool where intrinsics.type_is_comparable(T) { + TEST_count += 1 + ok := value == expected || reflect.is_nil(value) && reflect.is_nil(expected) + if !ok { + TEST_fail += 1 + fmt.printf("[%v] expected %v, got %v\n", loc, expected, value) + } + return ok + } +} + +main :: proc() { + t := testing.T{} + + test_temp_allocator_alignment_boundary(&t) + test_temp_allocator_big_alloc_and_alignment(&t) + + fmt.printf("%v/%v tests successful.\n", TEST_count - TEST_fail, TEST_count) + if TEST_fail > 0 { + os.exit(1) + } +} + +// Tests that having space for the allocation, but not for the allocation and alignment +// is handled correctly. +@(test) +test_temp_allocator_alignment_boundary :: proc(t: ^testing.T) { + arena: runtime.Arena + context.allocator = runtime.arena_allocator(&arena) + + _, _ = mem.alloc(int(runtime.DEFAULT_ARENA_GROWING_MINIMUM_BLOCK_SIZE)-120) + _, err := mem.alloc(112, 32) + expect_value(t, err, nil) +} + +// Tests that big allocations with big alignments are handled correctly. +@(test) +test_temp_allocator_big_alloc_and_alignment :: proc(t: ^testing.T) { + arena: runtime.Arena + context.allocator = runtime.arena_allocator(&arena) + + mappy: map[[8]int]int + err := reserve(&mappy, 50000) + expect_value(t, err, nil) +} From af962526df0f082083646d9ff8e13bff5bcbc921 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Mon, 18 Dec 2023 14:46:37 +0100 Subject: [PATCH 2/6] switch tests around --- tests/core/runtime/test_core_runtime.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/runtime/test_core_runtime.odin b/tests/core/runtime/test_core_runtime.odin index 5a57be4c3..6ec1e78d9 100644 --- a/tests/core/runtime/test_core_runtime.odin +++ b/tests/core/runtime/test_core_runtime.odin @@ -28,8 +28,8 @@ when ODIN_TEST { main :: proc() { t := testing.T{} - test_temp_allocator_alignment_boundary(&t) test_temp_allocator_big_alloc_and_alignment(&t) + test_temp_allocator_alignment_boundary(&t) fmt.printf("%v/%v tests successful.\n", TEST_count - TEST_fail, TEST_count) if TEST_fail > 0 { From 4ae021cd4cc8fb8a1cf27c0b7c2272ddd025dde0 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Mon, 18 Dec 2023 15:17:27 +0100 Subject: [PATCH 3/6] add other failing test and fix them --- core/runtime/default_allocators_arena.odin | 8 ++++---- tests/core/runtime/test_core_runtime.odin | 13 ++++++++++++- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/core/runtime/default_allocators_arena.odin b/core/runtime/default_allocators_arena.odin index 1c36c4f7c..3e78e7f20 100644 --- a/core/runtime/default_allocators_arena.odin +++ b/core/runtime/default_allocators_arena.odin @@ -102,14 +102,14 @@ arena_alloc :: proc(arena: ^Arena, size, alignment: uint, loc := #caller_locatio if size == 0 { return } - - if arena.curr_block == nil || (safe_add(arena.curr_block.used, size) or_else 0) > arena.curr_block.capacity { - size = align_forward_uint(size, alignment) + + needed := align_forward_uint(size, alignment) + if arena.curr_block == nil || (safe_add(arena.curr_block.used, needed) or_else 0) > arena.curr_block.capacity { if arena.minimum_block_size == 0 { arena.minimum_block_size = DEFAULT_ARENA_GROWING_MINIMUM_BLOCK_SIZE } - block_size := max(size, arena.minimum_block_size) + block_size := max(needed, arena.minimum_block_size) if arena.backing_allocator.procedure == nil { arena.backing_allocator = default_allocator() diff --git a/tests/core/runtime/test_core_runtime.odin b/tests/core/runtime/test_core_runtime.odin index 6ec1e78d9..5ae07ffe2 100644 --- a/tests/core/runtime/test_core_runtime.odin +++ b/tests/core/runtime/test_core_runtime.odin @@ -30,6 +30,7 @@ main :: proc() { test_temp_allocator_big_alloc_and_alignment(&t) test_temp_allocator_alignment_boundary(&t) + test_temp_allocator_returns_correct_size(&t) fmt.printf("%v/%v tests successful.\n", TEST_count - TEST_fail, TEST_count) if TEST_fail > 0 { @@ -56,6 +57,16 @@ test_temp_allocator_big_alloc_and_alignment :: proc(t: ^testing.T) { context.allocator = runtime.arena_allocator(&arena) mappy: map[[8]int]int - err := reserve(&mappy, 50000) + err := reserve(&mappy, 50000) expect_value(t, err, nil) } + +@(test) +test_temp_allocator_returns_correct_size :: proc(t: ^testing.T) { + arena: runtime.Arena + context.allocator = runtime.arena_allocator(&arena) + + bytes, err := mem.alloc_bytes(10, 16) + expect_value(t, err, nil) + expect_value(t, len(bytes), 10) +} From 252de70b0fe30a0892f4ad882c9ff6a485809ee9 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Mon, 18 Dec 2023 15:41:36 +0100 Subject: [PATCH 4/6] fix same problem in virtual arena --- core/mem/virtual/arena.odin | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/mem/virtual/arena.odin b/core/mem/virtual/arena.odin index 082ae3cd8..bc685f0b8 100644 --- a/core/mem/virtual/arena.odin +++ b/core/mem/virtual/arena.odin @@ -98,15 +98,15 @@ arena_alloc :: proc(arena: ^Arena, size: uint, alignment: uint, loc := #caller_l switch arena.kind { case .Growing: - if arena.curr_block == nil || (safe_add(arena.curr_block.used, size) or_else 0) > arena.curr_block.reserved { - size = mem.align_forward_uint(size, alignment) + needed := mem.align_forward_uint(size, alignment) + if arena.curr_block == nil || (safe_add(arena.curr_block.used, needed) or_else 0) > arena.curr_block.reserved { if arena.minimum_block_size == 0 { arena.minimum_block_size = DEFAULT_ARENA_GROWING_MINIMUM_BLOCK_SIZE } - block_size := max(size, arena.minimum_block_size) + block_size := max(needed, arena.minimum_block_size) - new_block := memory_block_alloc(size, block_size, {}) or_return + new_block := memory_block_alloc(needed, block_size, {}) or_return new_block.prev = arena.curr_block arena.curr_block = new_block arena.total_reserved += new_block.reserved From 9a490e4e0d6c50b0f27f7844437115a92cb8f2c1 Mon Sep 17 00:00:00 2001 From: Laytan Date: Mon, 18 Dec 2023 16:38:51 +0100 Subject: [PATCH 5/6] fix big alignment --- core/mem/virtual/arena.odin | 2 +- core/mem/virtual/virtual.odin | 8 ++++---- core/runtime/default_allocators_arena.odin | 12 ++++++------ 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/core/mem/virtual/arena.odin b/core/mem/virtual/arena.odin index bc685f0b8..90379e5e6 100644 --- a/core/mem/virtual/arena.odin +++ b/core/mem/virtual/arena.odin @@ -106,7 +106,7 @@ arena_alloc :: proc(arena: ^Arena, size: uint, alignment: uint, loc := #caller_l block_size := max(needed, arena.minimum_block_size) - new_block := memory_block_alloc(needed, block_size, {}) or_return + new_block := memory_block_alloc(needed, block_size, alignment, {}) or_return new_block.prev = arena.curr_block arena.curr_block = new_block arena.total_reserved += new_block.reserved diff --git a/core/mem/virtual/virtual.odin b/core/mem/virtual/virtual.odin index 1624fae9d..89f0d9c58 100644 --- a/core/mem/virtual/virtual.odin +++ b/core/mem/virtual/virtual.odin @@ -68,7 +68,7 @@ align_formula :: #force_inline proc "contextless" (size, align: uint) -> uint { } @(require_results) -memory_block_alloc :: proc(committed, reserved: uint, flags: Memory_Block_Flags) -> (block: ^Memory_Block, err: Allocator_Error) { +memory_block_alloc :: proc(committed, reserved: uint, alignment: uint, flags: Memory_Block_Flags) -> (block: ^Memory_Block, err: Allocator_Error) { page_size := DEFAULT_PAGE_SIZE assert(mem.is_power_of_two(uintptr(page_size))) @@ -79,8 +79,8 @@ memory_block_alloc :: proc(committed, reserved: uint, flags: Memory_Block_Flags) reserved = align_formula(reserved, page_size) committed = clamp(committed, 0, reserved) - total_size := uint(reserved + size_of(Platform_Memory_Block)) - base_offset := uintptr(size_of(Platform_Memory_Block)) + total_size := uint(reserved + max(alignment, size_of(Platform_Memory_Block))) + base_offset := uintptr(max(alignment, size_of(Platform_Memory_Block))) protect_offset := uintptr(0) do_protection := false @@ -183,4 +183,4 @@ memory_block_dealloc :: proc(block_to_free: ^Memory_Block) { safe_add :: #force_inline proc "contextless" (x, y: uint) -> (uint, bool) { z, did_overflow := intrinsics.overflow_add(x, y) return z, !did_overflow -} \ No newline at end of file +} diff --git a/core/runtime/default_allocators_arena.odin b/core/runtime/default_allocators_arena.odin index 3e78e7f20..4506acb56 100644 --- a/core/runtime/default_allocators_arena.odin +++ b/core/runtime/default_allocators_arena.odin @@ -28,11 +28,11 @@ safe_add :: #force_inline proc "contextless" (x, y: uint) -> (uint, bool) { } @(require_results) -memory_block_alloc :: proc(allocator: Allocator, capacity: uint, loc := #caller_location) -> (block: ^Memory_Block, err: Allocator_Error) { - total_size := uint(capacity + size_of(Memory_Block)) - base_offset := uintptr(size_of(Memory_Block)) +memory_block_alloc :: proc(allocator: Allocator, capacity: uint, alignment: uint, loc := #caller_location) -> (block: ^Memory_Block, err: Allocator_Error) { + total_size := uint(capacity + max(alignment, size_of(Memory_Block))) + base_offset := uintptr(max(alignment, size_of(Memory_Block))) - min_alignment: int = max(16, align_of(Memory_Block)) + min_alignment: int = max(16, align_of(Memory_Block), int(alignment)) data := mem_alloc(int(total_size), min_alignment, allocator, loc) or_return block = (^Memory_Block)(raw_data(data)) end := uintptr(raw_data(data)[len(data):]) @@ -115,7 +115,7 @@ arena_alloc :: proc(arena: ^Arena, size, alignment: uint, loc := #caller_locatio arena.backing_allocator = default_allocator() } - new_block := memory_block_alloc(arena.backing_allocator, block_size, loc) or_return + new_block := memory_block_alloc(arena.backing_allocator, block_size, alignment, loc) or_return new_block.prev = arena.curr_block arena.curr_block = new_block arena.total_capacity += new_block.capacity @@ -134,7 +134,7 @@ arena_init :: proc(arena: ^Arena, size: uint, backing_allocator: Allocator, loc arena^ = {} arena.backing_allocator = backing_allocator arena.minimum_block_size = max(size, 1<<12) // minimum block size of 4 KiB - new_block := memory_block_alloc(arena.backing_allocator, arena.minimum_block_size, loc) or_return + new_block := memory_block_alloc(arena.backing_allocator, arena.minimum_block_size, 0, loc) or_return arena.curr_block = new_block arena.total_capacity += new_block.capacity return nil From baa5ea9258823dfe8b17fe603e2015ecc880cb79 Mon Sep 17 00:00:00 2001 From: Laytan Date: Mon, 18 Dec 2023 16:41:55 +0100 Subject: [PATCH 6/6] fix not passing arg everywhere --- core/mem/virtual/virtual.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/mem/virtual/virtual.odin b/core/mem/virtual/virtual.odin index 89f0d9c58..00a9e6a5d 100644 --- a/core/mem/virtual/virtual.odin +++ b/core/mem/virtual/virtual.odin @@ -68,7 +68,7 @@ align_formula :: #force_inline proc "contextless" (size, align: uint) -> uint { } @(require_results) -memory_block_alloc :: proc(committed, reserved: uint, alignment: uint, flags: Memory_Block_Flags) -> (block: ^Memory_Block, err: Allocator_Error) { +memory_block_alloc :: proc(committed, reserved: uint, alignment: uint = 0, flags: Memory_Block_Flags = {}) -> (block: ^Memory_Block, err: Allocator_Error) { page_size := DEFAULT_PAGE_SIZE assert(mem.is_power_of_two(uintptr(page_size)))