allocators: don’t assume MIN_ALIGN for small sizes #46117
Conversation
…5955 The GNU C library (glibc) is documented to always allocate with an alignment of at least 8 or 16 bytes, on 32-bit or 64-bit platforms: https://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html This matches our use of `MIN_ALIGN` before this commit. However, even when libc is glibc, the program might be linked with another allocator that redefines the `malloc` symbol and friends. (The `alloc_jemalloc` crate does, in some cases.) So `alloc_system` doesn’t know which allocator it calls, and needs to be conservative in assumptions it makes. The C standard says: https://port70.net/%7Ensz/c/c11/n1570.html#7.22.3 > The pointer returned if the allocation succeeds is suitably aligned > so that it may be assigned to a pointer to any type of object > with a fundamental alignment requirement https://port70.net/~nsz/c/c11/n1570.html#6.2.8p2 > A fundamental alignment is represented by an alignment less than > or equal to the greatest alignment supported by the implementation > in all contexts, which is equal to `_Alignof (max_align_t)`. `_Alignof (max_align_t)` depends on the ABI and doesn’t seem to have a clear definition, but it seems to match our `MIN_ALIGN` in practice. However, the size of objects is rounded up to the next multiple of their alignment (since that size is also the stride used in arrays). Conversely, the alignment of a non-zero-size object is at most its size. So for example it seems ot be legal for `malloc(8)` to return a pointer that’s only 8-bytes-aligned, even if `_Alignof (max_align_t)` is 16.
See previous commit’s message for what is expected of allocators in general, and jemalloc/jemalloc#1072 for discussion of what jemalloc does specifically.
Most often, this code is used through the `std::heap::Heap` and `#[gloabal_allocator]` indirection, so this branch is not optimized out anymore.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @bluss (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
I’d appreciate review of not only the diff, but also the assumptions. In particular, should We might also want to reconsider how useful this In |
|
So this does not fix |
kennytm
added
the
S-waiting-on-review
|
Awesome, thanks! I'd personally think that the Unfortunately I think it's basically just impossible to fix |
|
@alexcrichton Let’s move the relevant discussion to #45966. |
|
@bors: r+ |
|
📌 Commit 43e32b5 has been approved by |
kennytm
added
S-waiting-on-bors
Same as rust-lang/rust#46117 See also discussion of jemalloc’s behavior at jemalloc/jemalloc#1072
allocators: don’t assume MIN_ALIGN for small sizes See individual commit messages.
|
☀️ Test successful - status-appveyor, status-travis |
See individual commit messages.