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.

Sign up for GitHub

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

Jump to bottom

Document that heap allocations are not guaranteed to happen, even if explicitly performed in the code #79045

Merged
merged 10 commits into from
Dec 26, 2020
Merged

Document that heap allocations are not guaranteed to happen, even if explicitly performed in the code #79045

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a2f938a
Document that heap allocations are not guaranteed to happen, even if …
oli-obk Nov 19, 2020
173892c
Note that there are other optimizations than the one showcased
oli-obk Nov 19, 2020
3d1f676
Fix tidy
oli-obk Nov 19, 2020
dfca61a
Elaborate on side effects
oli-obk Nov 19, 2020
58d62b8
Inform tidy about the reason for the ignored rust code
oli-obk Nov 19, 2020
fda4c8d
Update documentation to review comments
oli-obk Dec 4, 2020
48c8ff5
Update library/core/src/alloc/global.rs
oli-obk Dec 26, 2020
714feab
Update library/core/src/alloc/mod.rs
oli-obk Dec 26, 2020
fba17e3
Adjust markdown text to be more like the rendered text
oli-obk Dec 26, 2020
efcd8a9
DIrect invocations of `AllocRef::alloc` cannot get optimized away
oli-obk Dec 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions library/core/src/alloc/global.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,26 @@ use crate::ptr;
/// * `Layout` queries and calculations in general must be correct. Callers of
/// this trait are allowed to rely on the contracts defined on each method,
/// and implementors must ensure such contracts remain true.
///
/// * You may not rely on allocations actually happening, even if there are explicit
/// heap allocations in the source. The optimizer may detect allocation/deallocation
/// pairs that it can instead move to stack allocations/deallocations and thus never
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
/// invoke the allocator here.
/// More concretely, the following code example is unsound, irrespective of whether your
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
/// custom allocator allows counting how many allocations have happened.
///
/// ```rust,ignore (unsound and has placeholders)
/// drop(Box::new(42));
/// let number_of_heap_allocs = /* call private allocator API */;
/// unsafe { std::intrinsics::assume(number_of_heap_allocs > 0); }
/// ```
///
/// Note that allocation/deallocation pairs being moved to the stack is not the only
/// optimization that can be applied. You may generally not rely on heap allocations
/// happening, if they can be removed without changing program behaviour.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

in the case of a program like

use std::alloc::Layout;
use std::alloc::{alloc, dealloc};

fn main() {
    unsafe {
        let layout = Layout::from_size_align(
            0xffff_ffff_ffff_ffff, // big data
            1
        ).unwrap();
        let ptr = alloc(layout);
        if ptr.is_null() {
            panic!("allocation failed");
        }
        dealloc(ptr, layout);
    };
}

is removing this heap allocation (and ptr.is_null() becoming a constant false) considered changing program behavior? the truthiness of ptr.is_null() is not a consequence of the allocation happening, but the allocator implementation.

i'm not sure if this paragraph should be widened to say that in some circumstances program behavior can change, or the above main becoming a no-op is a rustc bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that 99.9% of allocation is done via ptr::NonNull::new(alloc(layout)).unwrap_or_else(|| handle_alloc_error(layout)) (Box does nearly exactly this), if the allocation removal optimization is ever allowed to apply, it needs to be able to optimize the allocation out even when doing an arbitrary -> ! on allocation failure (as handle_alloc_error calls the dynamic alloc error hook).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording to apply here would be to say (somehow) that allocation removal optimization happens assuming that allocation is infallible, or iow that an optimized out allocation always succeeds, or iow that "without changing program behavior when allocation succeeds" or similar.

Copy link
Member

@RalfJung RalfJung Dec 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

is removing this heap allocation (and ptr.is_null() becoming a constant false) considered changing program behavior?

I wish I knew the answer.^^ In practice, LLVM will optimize away even such huge allocations that can never succeed. So the LLVM devs consider this to not be changing program behavior.

However, I know of no formal model that would actually explain this, and it might even be inconsistent. Somehow the formal model needs to say that the allocation can succeed even if it is too big to fit into the finite memory, and then proceed in a consistent way even if the program sanity-checks the integer addresses that it got (since those checks can be optimized away if their result is unused) and so on.

I consider this to be one of the hardest unanswered questions in formal semantics for low-level languages: to either show that this makes sense, or to show that the optimization is broken and leads to miscompilations.

/// Whether allocations happen or not is not part of the program behaviour, even if it
/// could be detected via an allocator that tracks allocations by printing or otherwise
/// having side effects.
#[stable(feature = "global_alloc", since = "1.28.0")]
pub unsafe trait GlobalAlloc {
/// Allocate memory as described by the given `layout`.
Expand Down
12 changes: 12 additions & 0 deletions library/core/src/alloc/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,18 @@ pub unsafe trait AllocRef {
/// The returned block may have a larger size than specified by `layout.size()`, and may or may
/// not have its contents initialized.
///
/// Note that you may not rely on this method actually getting called, even if there are calls
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
/// to it in the source. The optimizer may detect allocation/deallocation pairs that it can
/// instead move to stack allocations/deallocations and thus never invoke the allocator here.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
/// More concretely, the following code example is unsound, irrespective of whether your
/// custom allocator allows counting how many allocations have happened.
///
/// ```rust,ignore (unsound and has placeholders)
/// Global::dealloc(Global::alloc(some_layout));
/// let number_of_heap_allocs = /* call private allocator API */;
/// unsafe { std::intrinsics::assume(number_of_heap_allocs > 0); }
/// ```
///
/// # Errors
///
/// Returning `Err` indicates that either memory is exhausted or `layout` does not meet
Expand Down