-
Notifications
You must be signed in to change notification settings - Fork 14
Vmmap unit tests #465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vmmap unit tests #465
Conversation
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Yaxuan-w
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this! Left a minor suggestion, and one place I'm a bit confused, would love to get more clarification:
I ran cargo test --lib memory::vmmap and met:
running 22 tests
test memory::vmmap::tests::test_add_entry_strict_no_overlap ... ok
test memory::vmmap::tests::test_add_entry_with_overwrite_partial_overlap ... ok
test memory::vmmap::tests::test_add_entry_with_overwrite_replaces_existing_full_overlap ... ok
test memory::vmmap::tests::test_change_prot_beginning_of_region ... ok
test memory::vmmap::tests::test_change_prot_end_of_region ... FAILED
test memory::vmmap::tests::test_change_prot_exact_boundaries ... FAILED
test memory::vmmap::tests::test_add_entry_with_overwrite_exact_boundaries ... ok
test memory::vmmap::tests::test_change_prot_entire_region ... ok
test memory::vmmap::tests::test_change_prot_multiple_times ... FAILED
test memory::vmmap::tests::test_change_prot_preserves_backing_type ... ok
test memory::vmmap::tests::test_change_prot_preserves_maxprot ... ok
test memory::vmmap::tests::test_change_prot_spanning_multiple_regions ... FAILED
test memory::vmmap::tests::test_change_prot_to_none ... FAILED
test memory::vmmap::tests::test_change_prot_middle_of_region ... FAILED
test memory::vmmap::tests::test_change_prot_to_same_value ... ok
test memory::vmmap::tests::test_find_map_space_with_hint_alignment_in_pages ... FAILED
test memory::vmmap::tests::test_find_map_space_with_hint_large_page_number ... ok
test memory::vmmap::tests::test_find_map_space_with_hint_searches_from_hint_page ... ok
test memory::vmmap::tests::test_find_map_space_with_hint_uses_page_number ... ok
test memory::vmmap::tests::test_find_map_space_with_hint_zero_hint ... ok
test memory::vmmap::tests::test_find_space_returns_none_when_full ... ok
test memory::vmmap::tests::test_add_entry_with_overwrite_removes_completely_covered_entries ... okWe only consider adding tests in this PR, and bug fixes will be included in another PR, right?
src/cage/README.md
Outdated
| ## Test Categories | ||
|
|
||
| ### 1. Memory Protection Change Tests (`change_prot`) | ||
|
|
||
| These tests verify that the `change_prot` function correctly modifies memory protection flags while preserving other entry attributes. | ||
|
|
||
| #### Key Tests: | ||
| - **`test_change_prot_entire_region`** - Verifies that changing protection on an entire region doesn't fragment it | ||
| - **`test_change_prot_middle_of_region`** - Confirms proper splitting into 3 parts when modifying middle pages | ||
| - **`test_change_prot_beginning_of_region`** - Tests splitting into 2 parts when modifying start pages | ||
| - **`test_change_prot_end_of_region`** - Tests splitting into 2 parts when modifying end pages | ||
| - **`test_change_prot_spanning_multiple_regions`** - Verifies correct handling across multiple non-contiguous regions | ||
| - **`test_change_prot_to_same_value`** - Ensures no fragmentation when protection doesn't actually change | ||
| - **`test_change_prot_exact_boundaries`** - Tests single-page modifications and precise boundary handling | ||
| - **`test_change_prot_multiple_times`** - Verifies correct state after successive protection changes | ||
| - **`test_change_prot_to_none`** - Tests setting protection to `PROT_NONE` | ||
| - **`test_change_prot_preserves_backing_type`** - Confirms backing type (Anonymous, SharedMemory, etc.) is preserved | ||
| - **`test_change_prot_preserves_maxprot`** - Ensures `maxprot` field remains unchanged | ||
|
|
||
| **Key Insight**: `change_prot` only modifies the `prot` field while preserving all other entry attributes including `maxprot`, `backing`, `flags`, etc. | ||
|
|
||
| ### 2. Entry Overwrite Behavior Tests (`add_entry_with_overwrite`) | ||
|
|
||
| These tests clarify what "overwrite" actually means in the context of adding memory entries. | ||
|
|
||
| #### Key Tests: | ||
| - **`test_add_entry_with_overwrite_replaces_existing_full_overlap`** - Demonstrates that overlapping entries are completely replaced, not merged | ||
| - **`test_add_entry_with_overwrite_partial_overlap`** - Shows how partial overlaps cause existing entries to be split | ||
| - **`test_add_entry_with_overwrite_removes_completely_covered_entries`** - Verifies that entries fully covered by new entry are removed | ||
| - **`test_add_entry_with_overwrite_exact_boundaries`** - Tests behavior at exact boundaries (adjacent but non-overlapping) | ||
|
|
||
| **Key Insight**: "Overwrite" means existing overlapping entries are replaced or split, not merged. The new entry's attributes completely replace the old entry's attributes in the overlapping region. | ||
|
|
||
| ### 3. Address Space Allocation Tests (`find_map_space_with_hint`) | ||
|
|
||
| These tests clarify parameter expectations and search behavior for finding available address space. | ||
|
|
||
| #### Key Tests: | ||
| - **`test_find_map_space_with_hint_uses_page_number`** - **CRITICAL**: Confirms hint parameter is a PAGE NUMBER, not a byte address | ||
| - **`test_find_map_space_with_hint_searches_from_hint_page`** - Verifies search begins at the hint page | ||
| - **`test_find_map_space_with_hint_zero_hint`** - Shows that hint=0 searches from the beginning | ||
| - **`test_find_map_space_with_hint_large_page_number`** - Confirms page-based behavior with large page numbers | ||
| - **`test_find_map_space_with_hint_alignment_in_pages`** - Verifies alignment (`pages_per_map`) is also in pages | ||
|
|
||
| **Key Insight**: All parameters to `find_map_space_with_hint` are in PAGE UNITS, not byte addresses. This includes: | ||
| - `hint`: starting page number for search | ||
| - `npages`: number of pages needed | ||
| - `pages_per_map`: alignment requirement in pages | ||
| - Return value: interval of page numbers | ||
|
|
||
| ### 4. Additional Function Behavior Tests | ||
|
|
||
| #### Key Tests: | ||
| - **`test_add_entry_strict_no_overlap`** - Demonstrates that `add_entry` (without overwrite) uses strict insertion and rejects overlaps | ||
| - **`test_find_space_returns_none_when_full`** - Verifies proper `None` return when no space is available | ||
|
|
||
| ## Important Clarifications from Tests | ||
|
|
||
| 1. **Page Numbers vs Addresses**: All vmmap operations use page numbers internally, not byte addresses | ||
| 2. **Entry Splitting**: Operations that modify part of an entry create new entries for unchanged portions | ||
| 3. **Attribute Preservation**: Protection changes preserve maxprot, backing type, and other metadata | ||
| 4. **Overwrite Semantics**: "Overwrite" means replace, not merge - new entry attributes completely override old ones | ||
| 5. **Strict vs Overwrite**: `add_entry` rejects overlaps; `add_entry_with_overwrite` handles them by splitting/replacing | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we put these into vmmap.rs? (after implementation, before tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think this probably needs to be paired down a bunch though, a lot of this is overwhelming/unescessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think this probably needs to be paired down a bunch though, a lot of this is overwhelming/unescessary
The tests themselves look good. Do you mean there shouldn't be such an extensive (and seemingly AI generated) README addition? I agree, that the doc here could be summarized in a few sentences.
However, a brief comment before each test about its purpose, etc. seems reasonable, so maybe those comments should move there.
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src/cage/README.md
Outdated
| - Return value: interval of page numbers | ||
|
|
||
|
|
||
| ## Important Clarifications from Unit Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this section is a bit weirdly phrased. we do want to know how this is structured but its phrased like we just found this out?
perhaps cross-reference the docs that talks about memory and make sure its consistent
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src/cage/src/memory/vmmap.rs
Outdated
| vmmap.start_address = 0; | ||
| vmmap.end_address = 1000; | ||
|
|
||
| // Add entries leaving gap at pages 50-99 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the gap is incorrect. Should be: 40-99
pages 10-39 (ends at 40)
pages 100-149 (starts at 100)
ssannkkallpp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks ok to me, I do see the following tests failing on this branch as of now. Is the idea to fix the test errors in a subsequent PR?
memory::vmmap::tests::test_change_prot_end_of_region memory::vmmap::tests::test_change_prot_exact_boundaries memory::vmmap::tests::test_change_prot_middle_of_region memory::vmmap::tests::test_change_prot_multiple_times memory::vmmap::tests::test_change_prot_spanning_multiple_regions memory::vmmap::tests::test_change_prot_to_none memory::vmmap::tests::test_find_map_space_with_hint_alignment_in_pages
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…md documentation Addressed the commenting issue with the test_find_map_space_with_hint_searches_from_hint_page function
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src/cage/src/memory/vmmap.rs
Outdated
| .unwrap(); | ||
|
|
||
| // Change protection to the same value | ||
| vmmap.change_prot(100, 10, PROT_READ | PROT_WRITE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to test there is no fragmentation when protection doesn't change, I think you should use a smaller region rather than same region?
Changed protection on a smaller subrange and verify fragmentation doesn't occur
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This PR creates unit tests for vmmap.rs and closes #400. Documentation of the unit tests and the key findings have been added as part of the cage lib README.
These are the results when running the tests:
failures:
memory::vmmap::tests::test_change_prot_end_of_region
memory::vmmap::tests::test_change_prot_exact_boundaries
memory::vmmap::tests::test_change_prot_middle_of_region
memory::vmmap::tests::test_change_prot_multiple_times
memory::vmmap::tests::test_change_prot_spanning_multiple_regions
memory::vmmap::tests::test_change_prot_to_none
memory::vmmap::tests::test_change_prot_to_same_value
memory::vmmap::tests::test_find_map_space_with_hint_alignment_in_pages
test result: FAILED. 14 passed; 8 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s