From 21d899272a7fb39a497424e3260ddab773af7983 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Mon, 20 Nov 2017 15:30:04 +0100 Subject: [PATCH 1/3] =?UTF-8?q?alloc=5Fsystem:=20don=E2=80=99t=20assume=20?= =?UTF-8?q?MIN=5FALIGN=20for=20small=20sizes,=20fix=20#45955?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/liballoc/tests/heap.rs | 40 ++++++++++++++++++++++++++++++++++++++ src/liballoc/tests/lib.rs | 4 ++++ src/liballoc_system/lib.rs | 6 +++--- 3 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 src/liballoc/tests/heap.rs diff --git a/src/liballoc/tests/heap.rs b/src/liballoc/tests/heap.rs new file mode 100644 index 0000000000000..9423aabc82b37 --- /dev/null +++ b/src/liballoc/tests/heap.rs @@ -0,0 +1,40 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use alloc_system::System; +use std::heap::{Heap, Alloc, Layout}; + +/// https://github.com/rust-lang/rust/issues/45955 +/// +/// Note that `#[global_allocator]` is not used, +/// so `liballoc_jemalloc` is linked (on some platforms). +#[test] +fn alloc_system_overaligned_request() { + check_overalign_requests(System) +} + +fn check_overalign_requests(mut allocator: T) { + let size = 8; + let align = 16; // greater than size + let iterations = 100; + unsafe { + let pointers: Vec<_> = (0..iterations).map(|_| { + allocator.alloc(Layout::from_size_align(size, align).unwrap()).unwrap() + }).collect(); + for &ptr in &pointers { + assert_eq!((ptr as usize) % align, 0, "Got a pointer less aligned than requested") + } + + // Clean up + for &ptr in &pointers { + allocator.dealloc(ptr, Layout::from_size_align(size, align).unwrap()) + } + } +} diff --git a/src/liballoc/tests/lib.rs b/src/liballoc/tests/lib.rs index 00ebd88d464ec..f1e95883b3827 100644 --- a/src/liballoc/tests/lib.rs +++ b/src/liballoc/tests/lib.rs @@ -10,6 +10,8 @@ #![deny(warnings)] +#![feature(allocator_api)] +#![feature(alloc_system)] #![feature(attr_literals)] #![feature(box_syntax)] #![feature(inclusive_range_syntax)] @@ -29,6 +31,7 @@ #![feature(unboxed_closures)] #![feature(unicode)] +extern crate alloc_system; extern crate std_unicode; extern crate rand; @@ -39,6 +42,7 @@ mod binary_heap; mod btree; mod cow_str; mod fmt; +mod heap; mod linked_list; mod slice; mod str; diff --git a/src/liballoc_system/lib.rs b/src/liballoc_system/lib.rs index 05cacf6e88195..8077ab2063d27 100644 --- a/src/liballoc_system/lib.rs +++ b/src/liballoc_system/lib.rs @@ -132,7 +132,7 @@ mod platform { unsafe impl<'a> Alloc for &'a System { #[inline] unsafe fn alloc(&mut self, layout: Layout) -> Result<*mut u8, AllocErr> { - let ptr = if layout.align() <= MIN_ALIGN { + let ptr = if layout.align() <= MIN_ALIGN && layout.align() <= layout.size() { libc::malloc(layout.size()) as *mut u8 } else { aligned_malloc(&layout) @@ -148,7 +148,7 @@ mod platform { unsafe fn alloc_zeroed(&mut self, layout: Layout) -> Result<*mut u8, AllocErr> { - if layout.align() <= MIN_ALIGN { + if layout.align() <= MIN_ALIGN && layout.align() <= layout.size() { let ptr = libc::calloc(layout.size(), 1) as *mut u8; if !ptr.is_null() { Ok(ptr) @@ -180,7 +180,7 @@ mod platform { }) } - if new_layout.align() <= MIN_ALIGN { + if new_layout.align() <= MIN_ALIGN && new_layout.align() <= new_layout.size(){ let ptr = libc::realloc(ptr as *mut libc::c_void, new_layout.size()); if !ptr.is_null() { Ok(ptr as *mut u8) From 2dd268b652cb42f4ebaa145c9e50cc6509c47d26 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Mon, 20 Nov 2017 15:42:34 +0100 Subject: [PATCH 2/3] =?UTF-8?q?alloc=5Fjemalloc:=20don=E2=80=99t=20assume?= =?UTF-8?q?=20MIN=5FALIGN=20for=20small=20sizes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See previous commit’s message for what is expected of allocators in general, and https://github.com/jemalloc/jemalloc/issues/1072 for discussion of what jemalloc does specifically. --- src/liballoc/tests/heap.rs | 5 +++++ src/liballoc_jemalloc/lib.rs | 22 +++++++++++----------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/liballoc/tests/heap.rs b/src/liballoc/tests/heap.rs index 9423aabc82b37..d3ce12056bb49 100644 --- a/src/liballoc/tests/heap.rs +++ b/src/liballoc/tests/heap.rs @@ -20,6 +20,11 @@ fn alloc_system_overaligned_request() { check_overalign_requests(System) } +#[test] +fn std_heap_overaligned_request() { + check_overalign_requests(Heap) +} + fn check_overalign_requests(mut allocator: T) { let size = 8; let align = 16; // greater than size diff --git a/src/liballoc_jemalloc/lib.rs b/src/liballoc_jemalloc/lib.rs index f060f6d79c17a..e8a844228ba14 100644 --- a/src/liballoc_jemalloc/lib.rs +++ b/src/liballoc_jemalloc/lib.rs @@ -92,8 +92,8 @@ mod contents { a.trailing_zeros() as c_int } - fn align_to_flags(align: usize) -> c_int { - if align <= MIN_ALIGN { + fn align_to_flags(align: usize, size: usize) -> c_int { + if align <= MIN_ALIGN && align <= size { 0 } else { mallocx_align(align) @@ -111,7 +111,7 @@ mod contents { pub unsafe extern fn __rde_alloc(size: usize, align: usize, err: *mut u8) -> *mut u8 { - let flags = align_to_flags(align); + let flags = align_to_flags(align, size); let ptr = mallocx(size as size_t, flags) as *mut u8; if ptr.is_null() { let layout = Layout::from_size_align_unchecked(size, align); @@ -132,7 +132,7 @@ mod contents { pub unsafe extern fn __rde_dealloc(ptr: *mut u8, size: usize, align: usize) { - let flags = align_to_flags(align); + let flags = align_to_flags(align, size); sdallocx(ptr as *mut c_void, size, flags); } @@ -142,7 +142,7 @@ mod contents { min: *mut usize, max: *mut usize) { let layout = &*(layout as *const Layout); - let flags = align_to_flags(layout.align()); + let flags = align_to_flags(layout.align(), layout.size()); let size = nallocx(layout.size(), flags) as usize; *min = layout.size(); if size > 0 { @@ -166,7 +166,7 @@ mod contents { return 0 as *mut u8 } - let flags = align_to_flags(new_align); + let flags = align_to_flags(new_align, new_size); let ptr = rallocx(ptr as *mut c_void, new_size, flags) as *mut u8; if ptr.is_null() { let layout = Layout::from_size_align_unchecked(new_size, new_align); @@ -181,10 +181,10 @@ mod contents { pub unsafe extern fn __rde_alloc_zeroed(size: usize, align: usize, err: *mut u8) -> *mut u8 { - let ptr = if align <= MIN_ALIGN { + let ptr = if align <= MIN_ALIGN && align <= size { calloc(size as size_t, 1) as *mut u8 } else { - let flags = align_to_flags(align) | MALLOCX_ZERO; + let flags = align_to_flags(align, size) | MALLOCX_ZERO; mallocx(size as size_t, flags) as *mut u8 }; if ptr.is_null() { @@ -203,7 +203,7 @@ mod contents { err: *mut u8) -> *mut u8 { let p = __rde_alloc(size, align, err); if !p.is_null() { - let flags = align_to_flags(align); + let flags = align_to_flags(align, size); *excess = nallocx(size, flags) as usize; } return p @@ -220,7 +220,7 @@ mod contents { err: *mut u8) -> *mut u8 { let p = __rde_realloc(ptr, old_size, old_align, new_size, new_align, err); if !p.is_null() { - let flags = align_to_flags(new_align); + let flags = align_to_flags(new_align, new_size); *excess = nallocx(new_size, flags) as usize; } p @@ -244,7 +244,7 @@ mod contents { new_size: usize, new_align: usize) -> u8 { if old_align == new_align { - let flags = align_to_flags(new_align); + let flags = align_to_flags(new_align, new_size); (xallocx(ptr as *mut c_void, new_size, 0, flags) == new_size) as u8 } else { 0 From 43e32b53462e139c560672102724e8a8c859dbf7 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Mon, 20 Nov 2017 15:55:31 +0100 Subject: [PATCH 3/3] Remove comment about a branch being optimized out, fix #45831 Most often, this code is used through the `std::heap::Heap` and `#[gloabal_allocator]` indirection, so this branch is not optimized out anymore. --- src/liballoc_jemalloc/lib.rs | 3 +-- src/liballoc_system/lib.rs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/liballoc_jemalloc/lib.rs b/src/liballoc_jemalloc/lib.rs index e8a844228ba14..d7370ae400dac 100644 --- a/src/liballoc_jemalloc/lib.rs +++ b/src/liballoc_jemalloc/lib.rs @@ -72,8 +72,7 @@ mod contents { const MALLOCX_ZERO: c_int = 0x40; // The minimum alignment guaranteed by the architecture. This value is used to - // add fast paths for low alignment values. In practice, the alignment is a - // constant at the call site and the branch will be optimized out. + // add fast paths for low alignment values. #[cfg(all(any(target_arch = "arm", target_arch = "mips", target_arch = "powerpc")))] diff --git a/src/liballoc_system/lib.rs b/src/liballoc_system/lib.rs index 8077ab2063d27..27259cc31a5ed 100644 --- a/src/liballoc_system/lib.rs +++ b/src/liballoc_system/lib.rs @@ -25,8 +25,7 @@ #![rustc_alloc_kind = "lib"] // The minimum alignment guaranteed by the architecture. This value is used to -// add fast paths for low alignment values. In practice, the alignment is a -// constant at the call site and the branch will be optimized out. +// add fast paths for low alignment values. #[cfg(all(any(target_arch = "x86", target_arch = "arm", target_arch = "mips",