Fix asserts #3074
Fix asserts #3074
Conversation
WalkthroughThe pull request introduces debug enhancements across multiple core memory management and garbage collection files. The changes focus on improving error reporting and validation checks in the CLR (Common Language Runtime) memory management system. Specifically, the modifications add more detailed debug print statements and boundary checks in methods responsible for heap block validation, memory relocation, and cluster validation. These changes aim to provide more comprehensive logging and error detection during memory operations without altering the fundamental control flow of the existing code. Changes
Sequence DiagramsequenceDiagram
participant HeapCluster as Heap Cluster
participant GarbageCollector as Garbage Collector
participant MemoryBlock as Memory Block
HeapCluster->>MemoryBlock: Validate Block
MemoryBlock-->>HeapCluster: Validation Results
GarbageCollector->>MemoryBlock: Check Boundaries
GarbageCollector->>MemoryBlock: Check Overlaps
MemoryBlock-->>GarbageCollector: Validation Status
Possibly related PRs
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/CLR/Core/GarbageCollector_Info.cpp (1)
114-117: Use Consistent Format Specifiers for Pointer PrintingIn the debug print statements, you're using different format specifiers based on the
_WIN64definition. To ensure cross-platform compatibility and correct formatting on both 32-bit and 64-bit systems, consider using thePRIxPTRformat specifier uniformly and casting pointers to(uintptr_t).Apply this diff:
#ifdef _WIN64 CLR_Debug::Printf("Block exceeds cluster boundary: 0x%016" PRIxPTR "\r\n", (uintptr_t)ptr); #else - CLR_Debug::Printf("Block exceeds cluster boundary: %08x\r\n", ptr); + CLR_Debug::Printf("Block exceeds cluster boundary: 0x%08" PRIxPTR "\r\n", (uintptr_t)ptr); #endif // Similarly update other print statements: - CLR_Debug::Printf("Overlapping blocks detected: Next block of %08x is overlapping it.\r\n", ptr); + CLR_Debug::Printf("Overlapping blocks detected: Next block of 0x%08" PRIxPTR " is overlapping it.\r\n", (uintptr_t)ptr); - CLR_Debug::Printf("Overlapping blocks detected: Previous block of %08x is overlapping it.\r\n", ptr); + CLR_Debug::Printf("Overlapping blocks detected: Previous block of 0x%08" PRIxPTR " is overlapping it.\r\n", (uintptr_t)ptr);Also applies to: 134-140, 155-163
src/CLR/Core/CLR_RT_HeapCluster.cpp (2)
339-356: Standardize Pointer Printing Across PlatformsIn the
ValidateBlockmethod, debug print statements use different formats for 32-bit and 64-bit systems. To maintain consistency and avoid potential issues, usePRIxPTRwith(uintptr_t)casting for pointer values on all platforms.Apply this diff:
#ifdef _WIN64 CLR_Debug::Printf( "Block beyond cluster limits: 0x%016" PRIxPTR " [0x%016" PRIxPTR " : 0x%016" PRIxPTR "-0x%016" PRIxPTR "]\r\n", (uintptr_t)ptr, (uintptr_t)this, (uintptr_t)m_payloadStart, (uintptr_t)m_payloadEnd); #else - CLR_Debug::Printf( - "Block beyond cluster limits: %08x [%08x : %08x-%08x]\r\n", - (size_t)ptr, - (size_t)this, - (size_t)m_payloadStart, - (size_t)m_payloadEnd); + CLR_Debug::Printf( + "Block beyond cluster limits: 0x%08" PRIxPTR " [0x%08" PRIxPTR " : 0x%08" PRIxPTR "-0x%08" PRIxPTR "]\r\n", + (uintptr_t)ptr, + (uintptr_t)this, + (uintptr_t)m_payloadStart, + (uintptr_t)m_payloadEnd); #endif // Similarly update other print statements for 'Bad Block Type', 'Bad Block null-size', and 'Bad Block size'.Also applies to: 363-371, 386-393, 407-414
386-393: Handle Zero DataSize with Detailed DiagnosticsA
DataSizeof zero is an anomaly that could indicate memory corruption. Instead of just breaking and invokingNANOCLR_DEBUG_STOP(), provide detailed diagnostics to aid in troubleshooting.Consider adding information about the block's state and surrounding memory to help identify the cause.
src/CLR/Core/GarbageCollector_Compaction.cpp (2)
479-480: Add Null Pointer Check Before DereferencingIn the
Heap_Relocate_Passmethod:_ASSERTE(ptr >= hc->m_payloadStart && ptr <= hc->m_payloadEnd);Ensure that
ptris notnullptrbefore performing the assertion and dereferencingptrinptr->DataSize(). Althoughptris expected to be valid within this context, adding a null check enhances safety.
539-548: Streamline Null Reference Handling in Heap_RelocateThe check for
dst == nullptris enclosed within a debug trace condition. To maintain consistent behavior regardless of the debug trace settings, move the null check outside of any conditional compilation blocks.Apply this diff:
{ -#if defined(NANOCLR_TRACE_MEMORY_STATS) if (dst == nullptr) { // nothing to do here - CLR_Debug::Printf("\r\nGC: Skipping relocation as referenced object is null.\r\n"); return; } - else - { - CLR_Debug::Printf("\r\nGC: Relocating Heap\r\n"); - } -#endif + // Proceed with relocation only if dst is not null + if (dst == nullptr) + { + // Nothing to relocate + return; + } }This ensures that null references are handled correctly in all builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CLR/Core/CLR_RT_HeapCluster.cpp(1 hunks)src/CLR/Core/GarbageCollector_Compaction.cpp(4 hunks)src/CLR/Core/GarbageCollector_Info.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (2)
src/CLR/Core/GarbageCollector_Info.cpp (1)
125-142: Review Overlapping Block Checks for CompletenessThe overlapping block checks added for the
nextPtrandprevPtraim to detect overlaps in memory blocks. However, the current conditions may not cover all edge cases of memory corruption or misalignment.
- Data Types: Limiting the checks to blocks that are not of
DATATYPE_VALUETYPEorDATATYPE_CLASSmight exclude other data types that could also cause overlaps if corrupted.- Boundary Conditions: Ensure that when comparing
nextPtrandprevPtrtoptr, all possibilities of overlapping due to incorrectDataSize()or pointer arithmetic errors are accounted for.Consider expanding the checks to include all relevant data types and thoroughly verify that no overlapping occurs due to any form of memory corruption. This enhances the robustness of the validation.
Also applies to: 146-165
src/CLR/Core/GarbageCollector_Compaction.cpp (1)
261-265: Ensure Debug Assertions are Active in Debug BuildsThe added
_ASSERTEstatements are crucial for validating memory boundaries during compaction. Ensure that theDEBUGmacro is defined during debug builds so these assertions are active. If custom build configurations are used, verify that_ASSERTEis correctly mapped to an assert function.
- Rework some asserts. - Add missing assert variations for 64 bits build. - Add new asserts. - Remove unnecessary asserts.
josesimoes
force-pushed
the
fix-asserts-code-style
branch
from
630d0af to
3d349dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/CLR/Core/GarbageCollector_Info.cpp (2)
122-142: Consider enhancing debug messages with block sizes.The overlap detection logic is solid. However, when overlaps are detected, including the sizes of the overlapping blocks in the debug message would help in debugging.
- CLR_Debug::Printf( - "Overlapping blocks detected. Next block of 0x%016" PRIxPTR " is overlapping it.\r\n", - (uintptr_t)ptr); + CLR_Debug::Printf( + "Overlapping blocks detected. Next block of 0x%016" PRIxPTR " (size: %d) is overlapping current block (size: %d).\r\n", + (uintptr_t)ptr, + nextPtr->DataSize(), + ptr->DataSize());
144-165: Maintain consistency with the previous enhancement.The previous block overlap checks mirror the next block checks perfectly. Apply the same debug message enhancement here for consistency.
- CLR_Debug::Printf( - "Overlapping blocks detected: Previous block of 0x%016" PRIxPTR " is overlapping it.\r\n", - (uintptr_t)ptr); + CLR_Debug::Printf( + "Overlapping blocks detected: Previous block of 0x%016" PRIxPTR " (size: %d) is overlapping current block (size: %d).\r\n", + (uintptr_t)ptr, + prevPtr->DataSize(), + ptr->DataSize());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/CLR/Core/CLR_RT_HeapCluster.cpp(1 hunks)src/CLR/Core/GarbageCollector_Compaction.cpp(4 hunks)src/CLR/Core/GarbageCollector_Info.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CLR/Core/GarbageCollector_Compaction.cpp
- src/CLR/Core/CLR_RT_HeapCluster.cpp
🧰 Additional context used
📓 Learnings (1)
src/CLR/Core/GarbageCollector_Info.cpp (2)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3074
File: src/CLR/Core/GarbageCollector_Info.cpp:107-167
Timestamp: 2025-01-22T03:38:57.394Z
Learning: In nanoFramework's memory management code, DataSize() validation is comprehensively handled through CLR_RT_HeapCluster::ValidateBlock() and other caller code. Additional size checks in ValidateCluster() are redundant as the validation is already performed at multiple levels.
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3074
File: src/CLR/Core/GarbageCollector_Info.cpp:107-167
Timestamp: 2025-01-22T03:38:57.394Z
Learning: In CLR_RT_GarbageCollector::ValidateCluster, DataSize() validation is already handled by ValidateBlock() and other caller code, making additional size checks redundant.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (2)
src/CLR/Core/GarbageCollector_Info.cpp (2)
104-106: LGTM! Good practice to validate blocks first.The early validation of blocks helps detect memory corruption before proceeding with further checks.
107-120: Well-implemented boundary checks with proper architecture support.The boundary validation is essential for memory safety, and the implementation correctly handles both 32-bit and 64-bit architectures with appropriate debug messages.
***NO_CI*** (cherry picked from commit a74f248)
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
These changes focus on improving the robustness and reliability of memory management within the system's core runtime components.