Add heap_diag_format_triage_line for heap integrity reporting#10
Open
Zaid-maker wants to merge 2 commits intomainfrom
Open
Add heap_diag_format_triage_line for heap integrity reporting#10Zaid-maker wants to merge 2 commits intomainfrom
Zaid-maker wants to merge 2 commits intomainfrom
Conversation
…tegrity reporting
📝 WalkthroughWalkthroughExtracted heap triage string formatting into a new public helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (2)
kernel/tests/heap_diag_test.c (1)
95-106: Consider adding coverage for remaining formatter branches.This test is solid for the
FAILpath. Add a small companion check forok=1andreport=0to lock in the fallback behavior introduced inheap_diag_format_triage_line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/tests/heap_diag_test.c` around lines 95 - 106, Add a small companion test in heap_diag_test.c that constructs an empty struct heap_integrity_report (all zero), calls heap_diag_format_triage_line(buffer, sizeof(buffer), &report, 1) and asserts the produced string matches the expected fallback (e.g. "Heap triage: OK"); this exercises the ok=1/report=0 branch of heap_diag_format_triage_line and locks in the fallback formatting behavior introduced for that case.kernel/src/kernel.c (1)
294-294: Include the declaring header for this call site (heap_diag.h).The
heap_diag_format_triage_line()function is declared inkernel/src/heap_diag.hbut not included in this translation unit. Direct inclusion of the declaring header ensures signature mismatches are caught at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/src/kernel.c` at line 294, Missing declaration include: add `#include` "heap_diag.h" to the top of the translation unit so the call to heap_diag_format_triage_line(...) is checked against its prototype; locate the use of heap_diag_format_triage_line in kernel.c and insert the heap_diag.h include alongside the other kernel headers to ensure signature mismatches are caught at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kernel/src/heap_diag.c`:
- Around line 146-149: The function heap_diag_format_triage_line currently calls
sbuf_reset(buffer) and sbuf_append_str without checking buffer or cap; guard the
start of heap_diag_format_triage_line by checking if buffer is NULL or cap is 0
and return early (or set a no-op behavior) to avoid dereferencing a
null/zero-length buffer, then only call sbuf_reset, sbuf_append_str and use len
when buffer and cap are valid; reference heap_diag_format_triage_line,
sbuf_reset, sbuf_append_str, buffer, cap and len to locate the change.
- Around line 146-161: The heap_diag.c test targets are missing the sbuf
implementation, causing undefined references to sbuf_reset, sbuf_append_str, and
sbuf_append_dec_u32; fix by updating the heap_runtime_test and
heap_integrity_test build targets to also compile/link src/sbuf.c alongside
heap_diag.c (mirror how the main kernel target includes both heap_diag.c and
sbuf.c) so that the sbuf symbols are provided to the tests.
---
Nitpick comments:
In `@kernel/src/kernel.c`:
- Line 294: Missing declaration include: add `#include` "heap_diag.h" to the top
of the translation unit so the call to heap_diag_format_triage_line(...) is
checked against its prototype; locate the use of heap_diag_format_triage_line in
kernel.c and insert the heap_diag.h include alongside the other kernel headers
to ensure signature mismatches are caught at compile time.
In `@kernel/tests/heap_diag_test.c`:
- Around line 95-106: Add a small companion test in heap_diag_test.c that
constructs an empty struct heap_integrity_report (all zero), calls
heap_diag_format_triage_line(buffer, sizeof(buffer), &report, 1) and asserts the
produced string matches the expected fallback (e.g. "Heap triage: OK"); this
exercises the ok=1/report=0 branch of heap_diag_format_triage_line and locks in
the fallback formatting behavior introduced for that case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09e14b55-2e93-462b-876a-1296adfe596a
📒 Files selected for processing (5)
kernel/src/heap_diag.ckernel/src/heap_diag.hkernel/src/kernel.ckernel/tests/heap_diag_test.ckernel/tests/heap_runtime_test.c
Comment on lines
+146
to
+149
| void heap_diag_format_triage_line(char* buffer, uint32_t cap, const struct heap_integrity_report* report, int ok) { | ||
| uint32_t len = 0u; | ||
| sbuf_reset(buffer); | ||
| sbuf_append_str(buffer, cap, &len, "Heap triage: "); |
There was a problem hiding this comment.
Guard buffer/cap before first write.
heap_diag_format_triage_line calls sbuf_reset(buffer) immediately, which dereferences buffer even when buffer == 0 or cap == 0.
Proposed fix
void heap_diag_format_triage_line(char* buffer, uint32_t cap, const struct heap_integrity_report* report, int ok) {
+ if (buffer == 0 || cap == 0u) {
+ return;
+ }
+
uint32_t len = 0u;
sbuf_reset(buffer);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void heap_diag_format_triage_line(char* buffer, uint32_t cap, const struct heap_integrity_report* report, int ok) { | |
| uint32_t len = 0u; | |
| sbuf_reset(buffer); | |
| sbuf_append_str(buffer, cap, &len, "Heap triage: "); | |
| void heap_diag_format_triage_line(char* buffer, uint32_t cap, const struct heap_integrity_report* report, int ok) { | |
| if (buffer == 0 || cap == 0u) { | |
| return; | |
| } | |
| uint32_t len = 0u; | |
| sbuf_reset(buffer); | |
| sbuf_append_str(buffer, cap, &len, "Heap triage: "); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kernel/src/heap_diag.c` around lines 146 - 149, The function
heap_diag_format_triage_line currently calls sbuf_reset(buffer) and
sbuf_append_str without checking buffer or cap; guard the start of
heap_diag_format_triage_line by checking if buffer is NULL or cap is 0 and
return early (or set a no-op behavior) to avoid dereferencing a null/zero-length
buffer, then only call sbuf_reset, sbuf_append_str and use len when buffer and
cap are valid; reference heap_diag_format_triage_line, sbuf_reset,
sbuf_append_str, buffer, cap and len to locate the change.
Comment on lines
+146
to
+161
| void heap_diag_format_triage_line(char* buffer, uint32_t cap, const struct heap_integrity_report* report, int ok) { | ||
| uint32_t len = 0u; | ||
| sbuf_reset(buffer); | ||
| sbuf_append_str(buffer, cap, &len, "Heap triage: "); | ||
| sbuf_append_str(buffer, cap, &len, ok ? "OK" : "FAIL"); | ||
| sbuf_append_str(buffer, cap, &len, " scanned="); | ||
| sbuf_append_dec_u32(buffer, cap, &len, report != 0 ? report->blocks_scanned : 0u); | ||
| sbuf_append_str(buffer, cap, &len, " bad="); | ||
| sbuf_append_dec_u32(buffer, cap, &len, report != 0 ? report->corrupted_headers : 0u); | ||
| sbuf_append_str(buffer, cap, &len, " align="); | ||
| sbuf_append_dec_u32(buffer, cap, &len, report != 0 ? report->split_alignment_issues : 0u); | ||
| sbuf_append_str(buffer, cap, &len, " unmerged="); | ||
| sbuf_append_dec_u32(buffer, cap, &len, report != 0 ? report->adjacent_unmerged_free_pairs : 0u); | ||
| sbuf_append_str(buffer, cap, &len, " next="); | ||
| sbuf_append_dec_u32(buffer, cap, &len, report != 0 ? report->next_pointer_regressions : 0u); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Build manifests =="
fd -HI 'Makefile|makefile|CMakeLists.txt|meson.build|BUILD|BUILD.bazel|*.mk' .
echo
echo "== References to heap_diag.c / sbuf.c in build files =="
rg -n --iglob '*Makefile*' --iglob '*.mk' --iglob 'CMakeLists.txt' --iglob 'meson.build' --iglob 'BUILD*' 'heap_diag\.c|sbuf\.c'
echo
echo "== Source ownership sanity =="
fd -HI '^heap_diag\.c$|^sbuf\.c$' kernel/srcRepository: Zaid-maker/kernel
Length of output: 509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding build configuration files =="
find . -type f \( -name "Makefile" -o -name "CMakeLists.txt" -o -name "meson.build" -o -name "BUILD" -o -name "*.mk" \) | head -20
echo
echo "== Check if sbuf.c and heap_diag.c exist =="
ls -la kernel/src/sbuf.c kernel/src/heap_diag.c 2>&1 || echo "Files not found"
echo
echo "== Search for heap_diag references in build files =="
find . -type f \( -name "Makefile" -o -name "CMakeLists.txt" -o -name "meson.build" -o -name "BUILD" -o -name "*.mk" \) -exec grep -l "heap_diag" {} \;
echo
echo "== Search for sbuf references in build files =="
find . -type f \( -name "Makefile" -o -name "CMakeLists.txt" -o -name "meson.build" -o -name "BUILD" -o -name "*.mk" \) -exec grep -l "sbuf" {} \;
echo
echo "== Check Makefile content if exists =="
if [ -f "Makefile" ]; then cat -n Makefile | head -50; fi
echo
echo "== Check kernel/src directory structure =="
ls -la kernel/src/ | head -30Repository: Zaid-maker/kernel
Length of output: 2066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full kernel/Makefile content =="
cat -n kernel/Makefile
echo
echo "== Check heap_diag.c includes =="
head -20 kernel/src/heap_diag.c
echo
echo "== Check sbuf.h header =="
cat -n kernel/src/sbuf.hRepository: Zaid-maker/kernel
Length of output: 7938
Fix test targets missing sbuf.c dependency.
The main kernel build (line 24) correctly includes both heap_diag.c and sbuf.c. However, test targets heap_runtime_test (line 70) and heap_integrity_test (line 71) compile heap_diag.c without linking sbuf.c, causing undefined reference errors for sbuf_reset, sbuf_append_str, and sbuf_append_dec_u32. Add src/sbuf.c to these two test targets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kernel/src/heap_diag.c` around lines 146 - 161, The heap_diag.c test targets
are missing the sbuf implementation, causing undefined references to sbuf_reset,
sbuf_append_str, and sbuf_append_dec_u32; fix by updating the heap_runtime_test
and heap_integrity_test build targets to also compile/link src/sbuf.c alongside
heap_diag.c (mirror how the main kernel target includes both heap_diag.c and
sbuf.c) so that the sbuf symbols are provided to the tests.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
kernel/Makefile (2)
64-71:⚠️ Potential issue | 🔴 CriticalPropagate
src/sbuf.cto alltestlink commands that includesrc/heap_diag.c.Good fix on Line 69, but Line 70 and Line 71 still link
src/heap_diag.cwithoutsrc/sbuf.c, which explains the linker failure reported at Line 70 in CI.Proposed fix
- $(HOST_CC) $(HOST_CFLAGS) tests/heap_runtime_test.c src/heap.c src/heap_diag.c tests/pmm_stub.c -o $(BUILD_DIR)/heap_runtime_test - $(HOST_CC) $(HOST_CFLAGS) -DHEAP_ENABLE_TEST_HOOKS tests/heap_integrity_test.c src/heap.c src/heap_diag.c tests/pmm_stub.c -o $(BUILD_DIR)/heap_integrity_test + $(HOST_CC) $(HOST_CFLAGS) tests/heap_runtime_test.c src/heap.c src/heap_diag.c src/sbuf.c tests/pmm_stub.c -o $(BUILD_DIR)/heap_runtime_test + $(HOST_CC) $(HOST_CFLAGS) -DHEAP_ENABLE_TEST_HOOKS tests/heap_integrity_test.c src/heap.c src/heap_diag.c src/sbuf.c tests/pmm_stub.c -o $(BUILD_DIR)/heap_integrity_test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/Makefile` around lines 64 - 71, The test link lines for heap_runtime_test and heap_integrity_test include src/heap_diag.c but are missing src/sbuf.c, causing unresolved symbols; update the Makefile test recipe so the commands that build heap_runtime_test and heap_integrity_test also link src/sbuf.c (preserving existing flags like -DHEAP_ENABLE_TEST_HOOKS for heap_integrity_test) so both targets include src/heap.c, src/heap_diag.c and src/sbuf.c during linking.
90-100:⚠️ Potential issue | 🔴 CriticalCoverage link steps are still missing
$(BUILD_DIR)/sbuf_cov.ofor heap targets.Line 98 is correct, but Line 99 and Line 100 also link
heap_diag_cov.oand must includesbuf_cov.o; this matches the CI linker failure at Line 99.Proposed fix
- $(HOST_CC) $(HOST_COVERAGE_CFLAGS) $(BUILD_DIR)/heap_runtime_test_cov.o $(BUILD_DIR)/heap_cov.o $(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/pmm_stub_cov.o -o $(BUILD_DIR)/heap_runtime_test_cov - $(HOST_CC) $(HOST_COVERAGE_CFLAGS) $(BUILD_DIR)/heap_integrity_test_cov.o $(BUILD_DIR)/heap_cov.o $(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/pmm_stub_cov.o -o $(BUILD_DIR)/heap_integrity_test_cov + $(HOST_CC) $(HOST_COVERAGE_CFLAGS) $(BUILD_DIR)/heap_runtime_test_cov.o $(BUILD_DIR)/heap_cov.o $(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/sbuf_cov.o $(BUILD_DIR)/pmm_stub_cov.o -o $(BUILD_DIR)/heap_runtime_test_cov + $(HOST_CC) $(HOST_COVERAGE_CFLAGS) $(BUILD_DIR)/heap_integrity_test_cov.o $(BUILD_DIR)/heap_cov.o $(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/sbuf_cov.o $(BUILD_DIR)/pmm_stub_cov.o -o $(BUILD_DIR)/heap_integrity_test_cov🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kernel/Makefile` around lines 90 - 100, The two coverage link rules for the heap tests (targets producing $(BUILD_DIR)/heap_runtime_test_cov and $(BUILD_DIR)/heap_integrity_test_cov) are missing the sbuf coverage object; update the link lines that currently list $(BUILD_DIR)/heap_cov.o $(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/pmm_stub_cov.o to also include $(BUILD_DIR)/sbuf_cov.o so the final link inputs are $(BUILD_DIR)/heap_cov.o $(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/pmm_stub_cov.o $(BUILD_DIR)/sbuf_cov.o for both the heap_runtime_test_cov and heap_integrity_test_cov targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@kernel/Makefile`:
- Around line 64-71: The test link lines for heap_runtime_test and
heap_integrity_test include src/heap_diag.c but are missing src/sbuf.c, causing
unresolved symbols; update the Makefile test recipe so the commands that build
heap_runtime_test and heap_integrity_test also link src/sbuf.c (preserving
existing flags like -DHEAP_ENABLE_TEST_HOOKS for heap_integrity_test) so both
targets include src/heap.c, src/heap_diag.c and src/sbuf.c during linking.
- Around line 90-100: The two coverage link rules for the heap tests (targets
producing $(BUILD_DIR)/heap_runtime_test_cov and
$(BUILD_DIR)/heap_integrity_test_cov) are missing the sbuf coverage object;
update the link lines that currently list $(BUILD_DIR)/heap_cov.o
$(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/pmm_stub_cov.o to also include
$(BUILD_DIR)/sbuf_cov.o so the final link inputs are $(BUILD_DIR)/heap_cov.o
$(BUILD_DIR)/heap_diag_cov.o $(BUILD_DIR)/pmm_stub_cov.o $(BUILD_DIR)/sbuf_cov.o
for both the heap_runtime_test_cov and heap_integrity_test_cov targets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests