Brand new memory allocator. #1856
Conversation
WalkthroughThis pull request introduces changes across multiple modules related to memory management. In CRT initialization, a conditional call to a memory sanitization function ( Changes
Sequence Diagram(s)sequenceDiagram
participant Main as cxxmain
participant MSan as pcsx_msanInit
Main->>Main: Begin initialization
alt USE_PCSXMSAN defined
Main->>MSan: Call pcsx_msanInit()
else Not defined
Main->>Main: Skip memory sanitization
end
Main->>Main: Proceed with further initialization
sequenceDiagram
participant App as Application
participant Alloc as multi_malloc
participant Heap as Memory Heap Manager
App->>Alloc: Request memory allocation (size)
Alloc->>Heap: Search free list (best-fit)
alt Block Found
Alloc->>App: Return pointer to allocated block
else No Suitable Block
Alloc->>Heap: Extend heap or report error
Alloc->>App: Return pointer or error message
end
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/mips/openbios/kernel/alloc.c (2)
79-148: Implementation ofmulti_malloc.
The best-fit search and block splitting are clearly implemented. However, the function contains multiple conditional paths and manual pointer arithmetic, which can grow unwieldy.Consider breaking this up into smaller helper functions, such as finding the best fit, initializing the heap (if needed), and splitting blocks, to reduce complexity and improve maintainability.
150-218: Implementation ofmulti_free.
The free logic with coalescing adjacent blocks is correct, but it significantly raises cyclomatic complexity.You might extract repeated logic (e.g., adjacency checks, merging) into separate helpers to improve readability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 150-218: ❌ Getting worse: Complex Method
multi_free increases in cyclomatic complexity from 14 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[notice] 150-218: ✅ Getting better: Bumpy Road Ahead
multi_free decreases from 5 to 4 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.src/mips/psyqo/src/alloc.c (2)
237-335:psyqo_malloccomplexity.
Implements a best-fit allocator with on-demand lazy initialization. The logic is correct, but the function is quite large.Consider extracting the best-fit loop and block splitting logic into separate helpers to reduce the nesting.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 237-335: ❌ New issue: Complex Method
psyqo_malloc has a cyclomatic complexity of 11, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 237-335: ❌ New issue: Bumpy Road Ahead
psyqo_malloc has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
339-461:psyqo_freecomplexity.
Manual block coalescing logic is correct but significantly increases nesting.Pull out repeated adjacency checks into helper routines to improve readability and keep the function smaller.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 339-461: ❌ New issue: Complex Method
psyqo_free has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 339-461: ❌ New issue: Bumpy Road Ahead
psyqo_free has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.src/mips/common/crt0/cxxglue.c (1)
51-53: Add documentation for memory sanitization feature.Consider adding a comment block explaining:
- The purpose of memory sanitization
- When to enable the
USE_PCSXMSANfeature- Any implications or requirements for using this feature
Add documentation above the ifdef block:
+ // Memory sanitizer initialization for development builds + // Enable USE_PCSXMSAN to track memory issues during development #ifdef USE_PCSXMSAN pcsx_msanInit(); #endif
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mips/common/crt0/cxxglue.c(1 hunks)src/mips/openbios/kernel/alloc.c(4 hunks)src/mips/psyqo/alloc.h(1 hunks)src/mips/psyqo/src/alloc.c(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/alloc.c
[warning] 463-639: ❌ Getting worse: Complex Method
psyqo_realloc increases in cyclomatic complexity from 10 to 23, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 166-225: ❌ New issue: Complex Method
check_integrity has a cyclomatic complexity of 15, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 339-461: ❌ New issue: Complex Method
psyqo_free has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 237-335: ❌ New issue: Complex Method
psyqo_malloc has a cyclomatic complexity of 11, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 604-604: ❌ New issue: Complex Conditional
psyqo_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 463-639: ❌ Getting worse: Bumpy Road Ahead
psyqo_realloc increases from 2 to 6 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 237-335: ❌ New issue: Bumpy Road Ahead
psyqo_malloc has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 339-461: ❌ New issue: Bumpy Road Ahead
psyqo_free has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 166-225: ❌ New issue: Bumpy Road Ahead
check_integrity has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 463-639: ❌ New issue: Deep, Nested Complexity
psyqo_realloc has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
src/mips/openbios/kernel/alloc.c
[warning] 221-353: ❌ Getting worse: Complex Method
multi_realloc increases in cyclomatic complexity from 11 to 29, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 150-218: ❌ Getting worse: Complex Method
multi_free increases in cyclomatic complexity from 14 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 326-326: ❌ New issue: Complex Conditional
multi_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[notice] 150-218: ✅ Getting better: Bumpy Road Ahead
multi_free decreases from 5 to 4 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 221-353: ❌ New issue: Deep, Nested Complexity
multi_realloc has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: build-openbios
- GitHub Check: asan
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: aur-build
🔇 Additional comments (24)
src/mips/openbios/kernel/alloc.c (11)
5-5: License year update acknowledged.
This update looks correct and consistent with the rest of the project’s license headers.
47-47: Additional comment block.
This is simply an ending comment marker; no issues found.
49-50: Reference to psyqo project.
Referencing the existing allocator in the psyqo project is helpful for maintainers; no issues found.
51-52: Alignment macros definition.
These macros are concise for alignment on pointer boundaries. Implementation looks correct.
54-57: Definition ofempty_block.
Struct is straightforward and well-documented.
59-63: Definition ofallocated_block.
Struct mirrorsempty_blocksize requirements; logic is consistent.
64-65: Compile-time size checks.
Static assertions ensure alignment constraints are enforced. Good use of_Static_assert.
72-73: Initialization of user_heap_head / kern_heap_head.
Introducing separate heap heads for user and kernel is clear. No issues at this point.
75-75: Definition of themarker.
Marker usage is a classic technique to avoid NULL checks on the free list terminator. Looks good.
364-364: Re-initializing user_heap_head.
Settinguser_heap_head = NULLon init is correct.
375-375: Re-initializing kern_heap_head.
Similarly, settingkern_heap_head = NULLon init is consistent.src/mips/psyqo/src/alloc.c (11)
5-5: License year update acknowledged.
Consistent with other license updates.
32-33: New includes.
These headers (pcsxhw.h,xprintf.h) seem appropriate for debugging and hardware interaction.
35-51: Extended allocator documentation.
Excellent detail that clarifies the allocator’s behavior and constraints.
52-56: Alignment macros.
Similar approach to openbios code, no issues found.
57-65: Global extern references for __heap_start and __stack_start.
References are consistent with typical embedded linker script usage, no issues here.
66-76: Comments on memory block layout.
Clear explanation of how the empty/allocated blocks are structured.
78-86: Allocated block definition.
Matches the concept inempty_block, with alignment enforced.
88-92: Compile-time asserts.
Ensures blocks remain aligned and consistent in size.
94-108: Global pointers for the heap structures & marker.
Introducinghead,bottom,top, andmarkerclarifies the free list boundary.
232-236: USE_PCSXMSAN block.
Overriding standard allocator calls with Msan instrumentation is a neat approach for memory sanitization.
658-659: Heap address accessors.
psyqo_heap_start()andpsyqo_heap_end()returningbottomandtopis consistent with the new design.src/mips/psyqo/alloc.h (1)
93-94: Clarification ofpsyqo_heap_end.
Documenting that it will always return the same pointer once non-NULL is returned is a helpful detail for consumers of the API.src/mips/common/crt0/cxxglue.c (1)
51-53:❓ Verification inconclusive
Verify memory sanitizer initialization.
The memory sanitizer initialization is correctly placed before any other initialization, but there are a few considerations:
- Ensure that
pcsx_msanInit()is properly declared in the included headers.- Consider adding error handling for the initialization.
Run the following script to verify the function declaration:
🏁 Script executed:
#!/bin/bash # Description: Check for pcsx_msanInit declaration # Test: Search for the function declaration in header files rg -t h "pcsx_msanInit"Length of output: 23
Verify memory sanitizer initialization and declaration.
The memory sanitizer initialization is correctly placed before other initialization routines. However, our initial header-only search did not reveal any declaration for
pcsx_msanInit(), so please verify whether this function is declared or defined elsewhere in the codebase. Additionally, consider adding error handling for the initialization call.
🏁 Script executed:
#!/bin/bash # Description: Search for pcsx_msanInit declaration or definition across the entire repository rg "pcsx_msanInit"Length of output: 70
Attention: Verify Memory Sanitizer Declaration and Error Handling Needed
The memory sanitizer initialization appears correctly placed before the rest of the initialization routines. However, our search only found its usage in
src/mips/common/crt0/cxxglue.cwithout any accompanying declaration in the headers. Please manually verify thatpcsx_msanInit()is properly declared (likely by ensuring its declaration is included via the appropriate header, such ascommon/hardware/pcsxhw.hor another related header). Additionally, consider adding error handling around the initialization call to guard against potential failures.
src/mips/openbios/kernel/alloc.c
Outdated
| static __attribute__((section(".ramtext"))) void *multi_realloc(void *ptr_, size_t size_, enum heap heap) { | ||
| if (ptr_ == NULL) { | ||
| return multi_malloc(size_, heap); | ||
| } | ||
|
|
||
| heap_t *head = heap == HEAP_USER ? user_head : kern_head; | ||
| if (size_ == 0) { | ||
| multi_free(ptr_, heap); | ||
| return NULL; | ||
| } | ||
|
|
||
| if (!ptr || !head) return; | ||
| size_t size = ALIGN_TO(size_ + sizeof(empty_block)); | ||
| empty_block *block = (empty_block *)ptr_; | ||
| size_t old_size = (--block)->size; | ||
|
|
||
| // First block; bumping head ahead. | ||
| if (ptr == head->ptr) { | ||
| size = head->size + (size_t)(head->ptr - (void *)head); | ||
| head = head->next; | ||
| if (size == old_size) { | ||
| return ptr_; | ||
| } | ||
|
|
||
| if (head) { | ||
| empty_block * const head = heap == HEAP_USER ? user_heap_head : kern_heap_head; | ||
| if (head == &marker) { | ||
| if (size < old_size) { | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| new_block->next = ▮ | ||
| new_block->size = old_size - size; | ||
| if (heap == HEAP_USER) { | ||
| user_head = head; | ||
| user_heap_head = new_block; | ||
| } else { | ||
| kern_head = head; | ||
| kern_heap_head = new_block; | ||
| } | ||
| block->size = size; | ||
| return ptr_; | ||
| } | ||
| return NULL; | ||
| } | ||
|
|
||
| if (block < head) { | ||
| if (size < old_size) { | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| if (head == (empty_block *)((char *)block + size)) { | ||
| new_block->next = head->next; | ||
| new_block->size = head->size + (old_size - size); | ||
| } else { | ||
| new_block->next = head; | ||
| new_block->size = old_size - size; | ||
| } | ||
| head->prev = NULL; | ||
| } else { | ||
| if (heap == HEAP_USER) { | ||
| user_tail = NULL; | ||
| user_heap_base = NULL; | ||
| user_head = NULL; | ||
| user_heap_bottom = NULL; | ||
| user_heap_head = new_block; | ||
| } else { | ||
| kern_tail = NULL; | ||
| kern_heap_base = NULL; | ||
| kern_head = NULL; | ||
| kern_heap_bottom = NULL; | ||
| kern_heap_head = new_block; | ||
| } | ||
| block->size = size; | ||
| return ptr_; | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // Finding the proper block | ||
| cur = head; | ||
| for (cur = head; ptr != cur->ptr; cur = cur->next) { | ||
| if (!cur->next) return; | ||
| } | ||
|
|
||
| if (cur->next) { | ||
| // In the middle, just unlink it | ||
| cur->next->prev = cur->prev; | ||
| } else { | ||
| // At the end, shrink heap | ||
| if (heap == HEAP_USER) { | ||
| user_tail = cur->prev; | ||
| if (!user_tail) { | ||
| user_heap_base = NULL; | ||
| user_head = NULL; | ||
| user_heap_bottom = NULL; | ||
| if (((char *)block + old_size) == (char *)head) { | ||
| size_t delta = size - old_size; | ||
| if (head->size >= delta) { | ||
| // If it has exactly the right amount of space, we can just remove | ||
| // the first block from the list. | ||
| if (head->size == delta) { | ||
| if (heap == HEAP_USER) { | ||
| user_heap_head = head->next; | ||
| } else { | ||
| kern_heap_head = head->next; | ||
| } | ||
| } else { | ||
| // Otherwise, we need to create a new empty block after what we are re-allocating. | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| new_block->next = head; | ||
| new_block->size = delta; | ||
| if (heap == HEAP_USER) { | ||
| user_heap_head = new_block; | ||
| } else { | ||
| kern_heap_head = new_block; | ||
| } | ||
| } | ||
| block->size = size; | ||
| return ptr_; | ||
| } | ||
| } else { | ||
| kern_tail = cur->prev; | ||
| if (!kern_tail) { | ||
| kern_heap_base = NULL; | ||
| kern_head = NULL; | ||
| kern_heap_bottom = NULL; | ||
| } | ||
| } else { | ||
| empty_block *curr = head; | ||
| empty_block *next = NULL; | ||
| while ((next = curr->next) != NULL) { | ||
| if ((next <= block) && (next != &marker)) { | ||
| curr = next; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| top = sbrk(0, heap); | ||
| size = (top - cur->prev->ptr) - cur->prev->size; | ||
| sbrk(-size, heap); | ||
| } | ||
|
|
||
| cur->prev->next = cur->next; | ||
| } | ||
|
|
||
| static __attribute__((section(".ramtext"))) void *multi_realloc(void *ptr, size_t size, enum heap heap) { | ||
| heap_t *prev; | ||
| void *new = NULL; | ||
|
|
||
| if (!size && ptr) { | ||
| multi_free(ptr, heap); | ||
| return NULL; | ||
| } | ||
|
|
||
| if (!ptr) return multi_malloc(size, heap); | ||
|
|
||
| size_t block_size = (size + sizeof(heap_t) + 7) & ~7; | ||
|
|
||
| prev = (heap_t *)((uintptr_t)ptr - sizeof(heap_t)); | ||
|
|
||
| // New memory block shorter ? | ||
| if (prev->size >= block_size) { | ||
| prev->size = block_size; | ||
| if (!prev->next) sbrk(ptr + block_size - sbrk(0, heap), heap); | ||
| if (size < old_size) { | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| if ((next != &marker) && (((char *)block + size) == (char *)next)) { | ||
| new_block->next = next->next; | ||
| new_block->size = old_size - size + next->size; | ||
| } else { | ||
| new_block->next = next; | ||
| new_block->size = old_size - size; | ||
| } | ||
| curr->next = new_block; | ||
| block->size = size; | ||
| return ptr_; | ||
| } | ||
|
|
||
| return ptr; | ||
| size_t delta = size - old_size; | ||
| if ((next != &marker) && (((char *)block + old_size) == (char *)next) && (next->size >= delta)) { | ||
| if (next->size == delta) { | ||
| curr->next = next->next; | ||
| } else { | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| new_block->next = next->next; | ||
| new_block->size = next->size - delta; | ||
| curr->next = new_block; | ||
| } | ||
| block->size = size; | ||
| return ptr_; | ||
| } | ||
| } | ||
|
|
||
| // New memory block larger | ||
| // Is it the last one ? | ||
| if (!prev->next) { | ||
| new = sbrk(block_size - prev->size, heap); | ||
| if (!new) return NULL; | ||
|
|
||
| prev->size = block_size; | ||
| return ptr; | ||
| void *new_ptr = multi_malloc(size_, heap); | ||
| if (new_ptr == NULL) { | ||
| return NULL; | ||
| } | ||
|
|
||
| // Do we have free memory after it ? | ||
| if ((prev->next->ptr - ptr) > block_size) { | ||
| prev->size = block_size; | ||
| return ptr; | ||
| //__builtin_memcpy(new_ptr, ptr_, old_size - sizeof(empty_block)); | ||
| uint32_t * src = (uint32_t *)ptr_; | ||
| uint32_t * dst = (uint32_t *)new_ptr; | ||
| uint32_t size_to_copy = old_size - sizeof(empty_block); | ||
| while (size_to_copy > 0) { | ||
| *dst++ = *src++; | ||
| size_to_copy -= sizeof(uint32_t); | ||
| } | ||
|
|
||
| // No luck. | ||
| new = multi_malloc(size, heap); | ||
| if (!new) return NULL; | ||
|
|
||
| uint32_t *src = (uint32_t *)ptr; | ||
| uint32_t *dst = (uint32_t *)new; | ||
| size = prev->size / 4; | ||
| for (size_t i = 0; i < size; i++) *dst++ = *src++; | ||
| multi_free(ptr, heap); | ||
| return new; | ||
| multi_free(ptr_, heap); | ||
| return new_ptr; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implementation of multi_realloc.
This function’s complexity stands out (static analysis indicates 29). Multiple branching paths for shrinking, growing, coalescing, and corner cases make it difficult to maintain.
Strongly consider extracting specialized subroutines (e.g., “attempt_grow_in_place”, “shrink_in_place”, “create_new_block”) and reduce deeply nested conditionals. This will enhance clarity and long-term maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 221-353: ❌ Getting worse: Complex Method
multi_realloc increases in cyclomatic complexity from 11 to 29, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 326-326: ❌ New issue: Complex Conditional
multi_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 221-353: ❌ New issue: Deep, Nested Complexity
multi_realloc has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
| dprintf("psyqo_realloc(%p, %u)\n", ptr_, size_); | ||
| // If the pointer is NULL, we can just call malloc. | ||
| if (ptr_ == NULL) { | ||
| dprintf("psyqo_realloc(%p, %u) -> malloc\n", ptr_, size_); | ||
| return psyqo_malloc(size_); | ||
| } | ||
|
|
||
| // Do we have free memory after it ? | ||
| if ((prev->next->ptr - ptr) > block_size) { | ||
| prev->size = block_size; | ||
| return ptr; | ||
| // If the size is 0, we can just call free. | ||
| if (size_ == 0) { | ||
| dprintf("psyqo_realloc(%p, %u) -> free\n", ptr_, size_); | ||
| psyqo_free(ptr_); | ||
| return NULL; | ||
| } | ||
|
|
||
| // No luck. | ||
| new = psyqo_malloc(size); | ||
| if (!new) return NULL; | ||
| size_t size = ALIGN_TO(size_ + sizeof(empty_block)); | ||
| dprintf("psyqo_realloc(%p, %u) -> %u\n", ptr_, size_, size); | ||
| // Get the current size of the block. | ||
| empty_block *block = (empty_block *)ptr_; | ||
| size_t old_size = (--block)->size; | ||
|
|
||
| __builtin_memcpy(new, ptr, prev->size); | ||
| psyqo_free(ptr); | ||
| return new; | ||
| } | ||
| // If the new size is the same as the old size, we can just return the pointer. | ||
| if (size == old_size) { | ||
| dprintf("psyqo_realloc(%p, %u) -> same\n", ptr_, size_); | ||
| return ptr_; | ||
| } | ||
|
|
||
| void psyqo_free(void *ptr) { | ||
| heap_t *cur; | ||
| void *top; | ||
| size_t size; | ||
| // Is our memory already completely full? | ||
| if (head == &marker) { | ||
| // If we're shrinking the allocation, then we can | ||
| // create a new empty block after what we are re-allocating, | ||
| // and re-create our list. | ||
| if (size < old_size) { | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| new_block->next = ▮ | ||
| new_block->size = old_size - size; | ||
| head = new_block; | ||
| block->size = size; | ||
| dprintf("psyqo_realloc(%p, %u) -> %p\n", ptr_, size_, ptr_); | ||
| check_integrity(); | ||
| return ptr_; | ||
| } | ||
| // Otherwise, we're out of luck, and we need to error out | ||
| // with a NULL pointer signalling we're out of memory. | ||
| return NULL; | ||
| } | ||
|
|
||
| if (!ptr || !head) return; | ||
| // Special case: is the allocated block before the head? | ||
| if (block < head) { | ||
| // Are we shrinking? | ||
| if (size < old_size) { | ||
| // If we are, we can just create a new empty block after what we are re-allocating. | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| // Is it adjacent to our head? | ||
| if (head == (empty_block *)((char *)block + size)) { | ||
| // Yes, we can merge them. | ||
| new_block->next = head->next; | ||
| new_block->size = head->size + (old_size - size); | ||
| } else { | ||
| // No, we need to create a new empty block after what we are re-allocating. | ||
| new_block->next = head; | ||
| new_block->size = old_size - size; | ||
| } | ||
| head = new_block; | ||
| block->size = size; | ||
| dprintf("psyqo_realloc(%p, %u) -> %p\n", ptr_, size_, ptr_); | ||
| check_integrity(); | ||
| return ptr_; | ||
| } | ||
| // We are growing. Is the first block adjacent to the block we're re-allocating? | ||
| // If the first block is adjacent to the block we're re-allocating, | ||
| // and it has enough space to hold the new size, we can just grow | ||
| // the allocation. | ||
| if (((char *)block + old_size) == (char *)head) { | ||
| size_t delta = size - old_size; | ||
| if (head->size >= delta) { | ||
| // If it has exactly the right amount of space, we can just remove | ||
| // the first block from the list. | ||
| if (head->size == delta) { | ||
| head = head->next; | ||
| } else { | ||
| // Otherwise, we need to create a new empty block after what we are re-allocating. | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| new_block->next = head; | ||
| new_block->size = delta; | ||
| head = new_block; | ||
| } | ||
| block->size = size; | ||
| dprintf("psyqo_realloc(%p, %u) -> %p\n", ptr_, size_, ptr_); | ||
| check_integrity(); | ||
| return ptr_; | ||
| } | ||
| } | ||
| } else { | ||
| // We need to locate where in the list the pointer is. To do this, | ||
| // we need to walk the list until we find the block that is right before | ||
| // the block we're re-allocating. | ||
| empty_block *curr = head; | ||
| empty_block *next = NULL; | ||
| while ((next = curr->next) != NULL) { | ||
| dprintf("psyqo_realloc: curr: "); | ||
| print_block(curr); | ||
| // Is the next block after the block we're re-allocating? | ||
| if ((next <= block) && (next != &marker)) { | ||
| // Nope, we're not there yet. | ||
| curr = next; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // First block; bumping head ahead. | ||
| if (ptr == head->ptr) { | ||
| size = head->size + (size_t)(head->ptr - (void *)head); | ||
| head = head->next; | ||
| // Here, curr points to the empty block before the block we're re-allocating, | ||
| // and next points to the empty block after the block we're re-allocating, or | ||
| // to marker if we're at the end of the list. | ||
|
|
||
| // Are we shrinking the allocation? | ||
| if (size < old_size) { | ||
| // We're going to create a new block at the end of what we are re-allocating. | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| // Is the next block adjacent to the block we're re-allocating? | ||
| if ((next != &marker) && (((char *)block + size) == (char *)next)) { | ||
| // Yes, we can merge them. | ||
| new_block->next = next->next; | ||
| new_block->size = old_size - size + next->size; | ||
| } else { | ||
| // No. Create a new empty block after what we are re-allocating. | ||
| new_block->next = next; | ||
| new_block->size = old_size - size; | ||
| } | ||
| curr->next = new_block; | ||
| block->size = size; | ||
| dprintf("psyqo_realloc(%p, %u) -> %p\n", ptr_, size_, ptr_); | ||
| check_integrity(); | ||
| return ptr_; | ||
| } | ||
|
|
||
| if (head) { | ||
| head->prev = NULL; | ||
| } else { | ||
| tail = NULL; | ||
| sbrk(-size); | ||
| // If we're growing the allocation, we need to check if the next block | ||
| // is adjacent to the block we're re-allocating, and if it has enough | ||
| // space to hold the new size. | ||
|
|
||
| size_t delta = size - old_size; | ||
| if ((next != &marker) && (((char *)block + old_size) == (char *)next) && (next->size >= delta)) { | ||
| // If it does, we can just grow the allocation. | ||
| // Do we have exactly the right amount of space available? | ||
| if (next->size == delta) { | ||
| // Yes? Then we can just remove the next block from the list. | ||
| curr->next = next->next; | ||
| } else { | ||
| // No? Then we need to create a new block after what we are re-allocating. | ||
| empty_block *new_block = (empty_block *)((char *)block + size); | ||
| new_block->next = next->next; | ||
| new_block->size = next->size - delta; | ||
| curr->next = new_block; | ||
| } | ||
| block->size = size; | ||
| dprintf("psyqo_realloc(%p, %u) -> %p\n", ptr_, size_, ptr_); | ||
| check_integrity(); | ||
| return ptr_; | ||
| } | ||
|
|
||
| return; | ||
| // Technically at this point, we have one last recourse before going for | ||
| // allocating memory elsewhere: we could try to probe if the block before | ||
| // the block we're re-allocating is adjacent to it, and if it is, we could | ||
| // try to merge it if it has enough space. This would require walking the | ||
| // list again, or use double-linked lists, and it requires memmove. Pain. | ||
| // So let's just ignore this case. | ||
| } | ||
|
|
||
| // Finding the proper block | ||
| cur = head; | ||
| for (cur = head; ptr != cur->ptr; cur = cur->next) | ||
| if (!cur->next) return; | ||
|
|
||
| if (cur->next) { | ||
| // In the middle, just unlink it | ||
| cur->next->prev = cur->prev; | ||
| } else { | ||
| // At the end, shrink heap | ||
| tail = cur->prev; | ||
| top = sbrk(0); | ||
| size = (top - cur->prev->ptr) - cur->prev->size; | ||
| sbrk(-size); | ||
| void *new_ptr = psyqo_malloc(size_); | ||
| if (new_ptr == NULL) { | ||
| dprintf("psyqo_realloc(%p, %u) -> NULL\n", ptr_, size_); | ||
| return NULL; | ||
| } | ||
|
|
||
| cur->prev->next = cur->next; | ||
| __builtin_memcpy(new_ptr, ptr_, old_size - sizeof(empty_block)); | ||
| psyqo_free(ptr_); | ||
| dprintf("psyqo_realloc(%p, %u) -> %p\n", ptr_, size_, new_ptr); | ||
| return new_ptr; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
psyqo_realloc complexity.
Static analysis flags it as very high in cyclomatic complexity. Multiple conditions for shrinking, coalescing, or growing hamper maintainability.
Splitting up the logic (grow in place, shrink in place, move & free, etc.) would reduce the code smell and facilitate future modifications or bug fixes.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 463-639: ❌ Getting worse: Complex Method
psyqo_realloc increases in cyclomatic complexity from 10 to 23, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 604-604: ❌ New issue: Complex Conditional
psyqo_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 463-639: ❌ Getting worse: Bumpy Road Ahead
psyqo_realloc increases from 2 to 6 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 463-639: ❌ New issue: Deep, Nested Complexity
psyqo_realloc has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/mips/openbios/kernel/alloc.c (1)
203-336: 🛠️ Refactor suggestionRefactor complex reallocation logic.
The
multi_reallocfunction has high cyclomatic complexity (29) and deep nesting (depth of 5). This makes the code harder to maintain and understand.Consider breaking down the function into smaller, focused functions:
- Extract shrink logic:
+static __attribute__((section(".ramtext"))) void *shrink_block(empty_block *block, size_t new_size, size_t old_size, empty_block *next, const enum heap heap) { + empty_block *new_block = (empty_block *)((char *)block + new_size); + if ((next != &marker) && (((char *)block + new_size) == (char *)next)) { + new_block->next = next->next; + new_block->size = old_size - new_size + next->size; + } else { + new_block->next = next; + new_block->size = old_size - new_size; + } + block->size = new_size; + return (void *)(block + 1); +}
- Extract grow logic:
+static __attribute__((section(".ramtext"))) void *grow_block(empty_block *block, size_t new_size, size_t old_size, empty_block *next, const enum heap heap) { + size_t delta = new_size - old_size; + if ((next != &marker) && (((char *)block + old_size) == (char *)next) && (next->size >= delta)) { + if (next->size == delta) { + curr->next = next->next; + } else { + empty_block *new_block = (empty_block *)((char *)block + new_size); + new_block->next = next->next; + new_block->size = next->size - delta; + curr->next = new_block; + } + block->size = new_size; + return (void *)(block + 1); + } + return NULL; +}🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 203-335: ❌ Getting worse: Complex Method
multi_realloc increases in cyclomatic complexity from 11 to 29, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 308-308: ❌ New issue: Complex Conditional
multi_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 203-335: ❌ New issue: Deep, Nested Complexity
multi_realloc has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (2)
src/mips/openbios/kernel/alloc.c (2)
5-5: Update copyright year.The copyright year should be 2024 as we are currently in February 2024.
-Copyright (c) 2025 PCSX-Redux authors +Copyright (c) 2024 PCSX-Redux authors
132-201: Consider extracting coalescing logic.The
multi_freefunction's complexity (16) exceeds the threshold of 9. The coalescing logic for merging adjacent free blocks could be extracted into a separate function for better maintainability.Consider extracting the coalescing logic into a separate function:
+static __attribute__((section(".ramtext"))) void coalesce_blocks(empty_block *curr, empty_block *block, size_t size) { + empty_block *next = curr->next; + if (((char *)curr + curr->size) == (char *)block) { + curr->size += size; + if (((char *)curr + curr->size) == (char *)next) { + curr->size += next->size; + curr->next = next->next; + } + } else if (((char *)block + size) == (char *)next) { + block->next = next->next; + block->size = size + next->size; + curr->next = block; + } else { + block->next = next; + curr->next = block; + } +}🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 132-200: ❌ Getting worse: Complex Method
multi_free increases in cyclomatic complexity from 14 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[notice] 132-200: ✅ Getting better: Bumpy Road Ahead
multi_free decreases from 5 to 4 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/openbios/kernel/alloc.c(4 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/openbios/kernel/alloc.c
[warning] 203-335: ❌ Getting worse: Complex Method
multi_realloc increases in cyclomatic complexity from 11 to 29, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 132-200: ❌ Getting worse: Complex Method
multi_free increases in cyclomatic complexity from 14 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[notice] 74-129: ✅ Getting better: Complex Method
multi_malloc decreases in cyclomatic complexity from 17 to 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 308-308: ❌ New issue: Complex Conditional
multi_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[notice] 132-200: ✅ Getting better: Bumpy Road Ahead
multi_free decreases from 5 to 4 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 203-335: ❌ New issue: Deep, Nested Complexity
multi_realloc has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: coverage
- GitHub Check: asan
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
- GitHub Check: build-openbios
🔇 Additional comments (4)
src/mips/openbios/kernel/alloc.c (4)
51-71: LGTM! Well-designed data structures.The memory block structures are well-designed with several notable benefits:
- Aligned to power-of-2 boundary for optimal performance
- Same size for both empty and allocated blocks
- Static assertions ensure correct sizing
74-130: LGTM! Efficient best-fit allocation strategy.The
multi_mallocimplementation uses a best-fit strategy which is a good balance between memory utilization and performance. The code has improved in complexity from 17 to 13 according to static analysis.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[notice] 74-129: ✅ Getting better: Complex Method
multi_malloc decreases in cyclomatic complexity from 17 to 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
343-347: LGTM! Proper heap initialization.The heap initialization functions correctly align the base address and size, ensuring proper memory alignment for optimal performance.
Also applies to: 354-358
326-333:❓ Verification inconclusive
Optimize memory copy operation.
The commented out
__builtin_memcpymight be more efficient than the manual word-by-word copy. Consider measuring the performance difference.
🏁 Script executed:
#!/bin/bash # Search for other memcpy implementations in the codebase rg -g '*.{c,h}' -A 5 'memcpy|__builtin_memcpy'Length of output: 66551
Subject: Revisit the Memory Copy Routine for Potential Performance Gains
The manual word‐by‐word copy implemented here may be less efficient than using compiler-optimized routines. Notably, similar code throughout the codebase leverages
__builtin_memcpyfor copying, which suggests it could deliver better performance. Please consider the following:
- Benchmark Both Approaches: Measure and compare the performance of the current loop versus re-enabling
__builtin_memcpy(new_ptr, ptr_, old_size - sizeof(empty_block)).- Evaluate Alignment Considerations: Ensure that switching to
__builtin_memcpydoes not introduce issues related to alignment or other side effects.- Document the Decision: Depending on the performance findings and any constraints, either update the code to use
__builtin_memcpyor add a comment explaining why the manual copy loop remains necessary.// Consider re-enabling __builtin_memcpy(new_ptr, ptr_, old_size - sizeof(empty_block)); uint32_t * src = (uint32_t *)ptr_; uint32_t * dst = (uint32_t *)new_ptr; uint32_t size_to_copy = old_size - sizeof(empty_block); while (size_to_copy > 0) { *dst++ = *src++; size_to_copy -= sizeof(uint32_t); }
nicolasnoble
force-pushed
the
newallocator
branch
from
eca1013 to
8b9df48
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/mips/openbios/kernel/alloc.c (1)
203-335:⚠️ Potential issueCritical: multi_realloc needs significant refactoring.
The function's complexity has increased significantly (cyclomatic complexity from 11 to 29) with deep nesting (depth of 5).
As highlighted in the previous review, consider breaking down the function into smaller, focused subroutines:
attempt_grow_in_placeshrink_in_placecreate_new_blockThis will improve maintainability and make the code easier to understand.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 203-334: ❌ Getting worse: Complex Method
multi_realloc increases in cyclomatic complexity from 11 to 29, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 308-308: ❌ New issue: Complex Conditional
multi_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[warning] 203-334: ❌ New issue: Deep, Nested Complexity
multi_realloc has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🧹 Nitpick comments (3)
src/mips/openbios/kernel/alloc.c (3)
49-49: Consider adding more detailed documentation.The comment references the psyqo project for implementation details. It would be helpful to include a brief overview of the allocator's design and key characteristics directly in this file.
132-201: Consider refactoring multi_free to reduce complexity.The function's cyclomatic complexity has increased from 14 to 16, making it harder to maintain.
Consider extracting the coalescing logic into a separate function:
+static __attribute__((section(".ramtext"))) void coalesce_blocks(empty_block *curr, empty_block *block, size_t size) { + empty_block *next = curr->next; + if (((char *)curr + curr->size) == (char *)block) { + curr->size += size; + if (((char *)curr + curr->size) == (char *)next) { + curr->size += next->size; + curr->next = next->next; + } + } else if (((char *)block + size) == (char *)next) { + block->next = next->next; + block->size = size + next->size; + curr->next = block; + } else { + block->next = next; + curr->next = block; + } +} static __attribute__((section(".ramtext"))) void multi_free(void *ptr_, const enum heap heap) { // ... existing code ... - if (((char *)curr + curr->size) == (char *)block) { - curr->size += size; - if (((char *)curr + curr->size) == (char *)next) { - curr->size += next->size; - curr->next = next->next; - } - } else if (((char *)block + size) == (char *)next) { - block->next = next->next; - block->size = size + next->size; - curr->next = block; - } else { - block->next = next; - curr->next = block; - } + coalesce_blocks(curr, block, size); // ... rest of the code ... }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 132-200: ❌ Getting worse: Complex Method
multi_free increases in cyclomatic complexity from 14 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[notice] 132-200: ✅ Getting better: Bumpy Road Ahead
multi_free decreases from 5 to 4 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
326-332: Consider using memcpy for better performance.The manual word-by-word copy could be replaced with a more efficient
memcpycall.- uint32_t * src = (uint32_t *)ptr_; - uint32_t * dst = (uint32_t *)new_ptr; - uint32_t size_to_copy = old_size - sizeof(empty_block); - while (size_to_copy > 0) { - *dst++ = *src++; - size_to_copy -= sizeof(uint32_t); - } + size_t size_to_copy = old_size - sizeof(empty_block); + memcpy(new_ptr, ptr_, size_to_copy);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/openbios/kernel/alloc.c(4 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/openbios/kernel/alloc.c
[warning] 203-334: ❌ Getting worse: Complex Method
multi_realloc increases in cyclomatic complexity from 11 to 29, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 132-200: ❌ Getting worse: Complex Method
multi_free increases in cyclomatic complexity from 14 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[notice] 74-129: ✅ Getting better: Complex Method
multi_malloc decreases in cyclomatic complexity from 17 to 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
[warning] 308-308: ❌ New issue: Complex Conditional
multi_realloc has 1 complex conditionals with 2 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
[notice] 132-200: ✅ Getting better: Bumpy Road Ahead
multi_free decreases from 5 to 4 logical blocks with deeply nested code, threshold is one single block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 203-334: ❌ New issue: Deep, Nested Complexity
multi_realloc has a nested complexity depth of 5, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-openbios
- GitHub Check: cross-arm64
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: build
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: coverage
🔇 Additional comments (3)
src/mips/openbios/kernel/alloc.c (3)
51-71: LGTM! Well-structured memory block definitions.The memory block structures are well-designed with:
- Proper alignment considerations
- Size assertions to ensure compatibility
- Clear separation between empty and allocated blocks
74-130: LGTM! Improved multi_malloc implementation.The best-fit allocation strategy is well-implemented with reduced complexity compared to the previous version (cyclomatic complexity decreased from 17 to 13).
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[notice] 74-129: ✅ Getting better: Complex Method
multi_malloc decreases in cyclomatic complexity from 17 to 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
342-346: LGTM! Consistent heap initialization.The initialization functions for both user and kernel heaps follow the same pattern and properly handle alignment.
Also applies to: 353-357
1 participant
No description provided.