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

Suggestion: debug-assert that the allocator returns aligned memory #131189

Open
emilk opened this issue Oct 3, 2024 · 0 comments
Open

Suggestion: debug-assert that the allocator returns aligned memory #131189

emilk opened this issue Oct 3, 2024 · 0 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@emilk
Copy link

emilk commented Oct 3, 2024

If the global allocator returns unaligned memory, the standard library catches it very late. Take this code:

#[global_allocator]
static GLOBAL: AccountingAllocator<mimalloc::MiMalloc> =
    AccountingAllocator::new(mimalloc::MiMalloc);

#[repr(C, align(256))]
pub struct BigStruct {}

fn collect() {
    assert_eq!(align_of::<BigStruct>(), 256);
    assert_eq!(size_of::<BigStruct>(), 256);
    let vec: Vec<T> = std::iter::once(BigStruct{}).collect();
    dbg!(vec.as_ptr() as usize % std::mem::align_of::<T>()); // Sometimes prints 64
    let slice = vec.as_slice(); // panics in debug builds with:
    // 'unsafe precondition(s) violated: slice::from_raw_parts requires the pointer to be aligned and non-null, and the total size of the slice not to exceed `isize::MAX`'
}

Because of a bug in MiMalloc, the Vec gets non-aligned memory.
This is not discovered until vec.as_slice calls slice::from_raw_parts which has a debug-assert.

I spent a lot of time looking for this bug before I realized it was MiMalloc that was to blame.

I suggest we add a debug-assert the standard library to check that the allocator returns aligned memory, i.e. here:

fn try_allocate_in(
capacity: usize,
init: AllocInit,
alloc: A,
elem_layout: Layout,
) -> Result<Self, TryReserveError> {
// We avoid `unwrap_or_else` here because it bloats the amount of
// LLVM IR generated.
let layout = match layout_array(capacity, elem_layout) {
Ok(layout) => layout,
Err(_) => return Err(CapacityOverflow.into()),
};
// Don't allocate here because `Drop` will not deallocate when `capacity` is 0.
if layout.size() == 0 {
return Ok(Self::new_in(alloc, elem_layout.align()));
}
if let Err(err) = alloc_guard(layout.size()) {
return Err(err);
}
let result = match init {
AllocInit::Uninitialized => alloc.allocate(layout),
#[cfg(not(no_global_oom_handling))]
AllocInit::Zeroed => alloc.allocate_zeroed(layout),
};
let ptr = match result {
Ok(ptr) => ptr,
Err(_) => return Err(AllocError { layout, non_exhaustive: () }.into()),
};
// Allocators currently return a `NonNull<[u8]>` whose length
// matches the size requested. If that ever changes, the capacity
// here should change to `ptr.len() / mem::size_of::<T>()`.
Ok(Self { ptr: Unique::from(ptr.cast()), cap: unsafe { Cap(capacity) }, alloc })
}

That would then report "The global allocator returned non-aligned memory", directly pointing the user at the problem, saving a lol of time for the next person.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 3, 2024
Wumpf added a commit to rerun-io/rerun that referenced this issue Oct 3, 2024
### What
* Required for #7298

### TODO
* [x] Fix the debug-assertion
* [x] Fix all new clippy lints
* [x] Add more clippy lints thats been added in 1.77, 1.78, 1.79


### MiMalloc bug
Debug assertions are now part of stdlib:
https://blog.rust-lang.org/2024/05/02/Rust-1.78.0.html#asserting-unsafe-preconditions,
making us hit this bug in MiMalloc:

* purpleprotocol/mimalloc_rust#128

I also opened this issue on the standard library:
* rust-lang/rust#131189

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7563?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7563?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/7563)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.

---------

Co-authored-by: Andreas Reich <r_andreas2@web.de>
@saethlin saethlin added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants