Skip to content
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

Resolve issues for unit tests on 32 bits #1095

Merged
merged 6 commits into from
Mar 21, 2024

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Mar 21, 2024

This PR resolves a few issues we recently saw for running unit tests in our CI.

  • Fix an issue in the tests for byte map mmapper. In the clean up step, we do munmap for a larger area than what we actually mmapped in the test. This caused an issue that we may munmap regions that are used by other code. Segfault in unit tests on 32bits #1092 is caused by this -- the munmap after the tests unmaps the memory used by Rust, and caused Rust to segfault after the tests.
  • Change the test address for mmap tests to reduce the chance that we may fail in mmap in the tests.
  • Make raw_memory_freelist only compiled in 64bits. This mitigates the issue we saw in RawMemoryFreeList can't get more space from mmap #1091 for raw_memory_freelist tests.

@qinsoon qinsoon marked this pull request as ready for review March 21, 2024 05:17
@qinsoon qinsoon requested a review from wks March 21, 2024 05:17
@@ -312,6 +315,7 @@ mod tests {
#[test]
fn protect() {
serial_test(|| {
let test_memory_size = MMAP_CHUNK_BYTES * 2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside the with_cleanup( || { ... }) block, it computes the number of pages to mmap separately (the expression pages_per_chunk * 2). We have to manually make sure this size is the same as MMAP_CHUNK_BYTES * 2 which is the definition of test_memory_size.

It is better if we can hoist the computations before the invocation of with_cleanup, like how you changed other test cases, and compute the number of pages to mmap and mprotect from the test_memory_size. That will make the two values dependent on each other, and make the test case more robust. For example:

let test_memory_bytes = MMAP_CHUNK_BYTES * 2;
let protect_memory_bytes = test_memory_bytes / 2; // Now this depends on test_memory_bytes
let test_memory_pages = test_memory_bytes >> LOG_BYTES_IN_PAGE as usize; // This also depends on test_memory_bytes
let protect_memory_pages = protect_memory_bytes >> LOG_BYTES_IN_PAGE as usize; // This also depends on test_memory_bytes
with_cleanup(|| {
    // ...
    mmapper.ensure_mapped(FIXED_ADDRESS, test_memory_pages).unwrap();
    mmapper.protect(FIXED_ADDRESS, protect_memory_pages);
    // ...
}, || {
    memory::munmap(FIXED_ADDRESS, test_memory_pages).unwrap();
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qinsoon qinsoon enabled auto-merge March 21, 2024 07:14
@qinsoon qinsoon added this pull request to the merge queue Mar 21, 2024
Merged via the queue into mmtk:master with commit 00d316f Mar 21, 2024
23 checks passed
@qinsoon qinsoon deleted the unit-tests-on-32bits branch March 21, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants