Move OOM handling to liballoc and remove the oom lang item
#51607
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Why is all this difficult dance of compiler generated functions necessary when a plain function definition with weak linkage would do the job (I think)? |
src/liballoc/alloc.rs
Outdated
| pub fn take_oom_hook() -> fn(Layout) { | ||
| let hook = HOOK.swap(ptr::null_mut(), Ordering::SeqCst); | ||
| if hook.is_null() { | ||
| default_oom_hook |
There was a problem hiding this comment.
This does not "take" the default hook, though. Should be reflected in the documentation.
There was a problem hiding this comment.
Would you expect that "taking" the default would replace it with a no-op? This is a behavior change not directly related to this PR and is maybe better discussed on the tracking issue: #51245
|
Does weak linkage of Rust symbols actually work on all platforms? How is it used? |
|
r? @nagisa @alexcrichton |
|
☔ The latest upstream changes (presumably #51543) made this pull request unmergeable. Please resolve the merge conflicts. |
SimonSapin
force-pushed
the
oom-hook
branch
3 times, most recently
from
46d3b1b
to
d70a0cb
Compare
|
AFAIK, weak linkage is not really a thing with MSVC. The closest would be |
|
is the HOOK stuff a public interface? Just curious: I trust this PR doesn't change the publicness of that. I only recall user-defined and mutable panic/unwinding handlers. |
|
@Ericson2314 Yes, |
|
Thanks for the info. |
|
I think that this will break liballoc on thumbv6m-none-eabi since that target does not support atomic operations at all. See #37492 |
|
@Amanieu If |
|
I’ve verified that |
SimonSapin
force-pushed
the
oom-hook
branch
2 times, most recently
from
6e239a9
to
9a46f59
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This removes the need for `no_std` programs to define that lang item themselves: Fixes rust-lang#51540
|
Is the plan to stabilize not having an "oom" lang item for the edition release? Or do we have time My preference would be to keep the "oom" lang item and stabilize some
Is this PR just for convenience then? The |
|
@japaric There is a RFC to stabilize the alloc crate: rust-lang/rfcs#2480 Also see #51846 for a similar concern regarding lang items used by liballoc. |
I believe it is the only remaining blocker, in addition to allowing to import the crate itself: rust-lang/rfcs#2480 |
Is this cost really significant? The "mandatory" code simplifies to: pub fn handle_alloc_error(layout: Layout) -> ! {
let hook = HOOK.load(Ordering::SeqCst);
if hook.is_null() {
// No-op when std is not linked
__rust_maybe_default_alloc_error_hook(layout.size(), layout.align())
} else {
let hook: fn(Layout) = unsafe { mem::transmute(hook) }
hook(layout);
}
// No-op when std is not linked
__rust_maybe_abort_internal();
intrinsics::abort()
}
static HOOK: AtomicPtr<()> = …;Or, with LTO and without pub fn handle_alloc_error(layout: Layout) -> ! {
let hook = HOOK.load(Ordering::SeqCst);
if !hook.is_null() {
let hook: fn(Layout) = unsafe { mem::transmute(hook) }
hook(layout);
}
intrinsics::abort()
}
static HOOK: AtomicPtr<()> = …;… which is one pointer-sized |
Is doubles the size of an allocator written for minimal footprint considered significant? This program: #[global_allocator]
static ALLOC: Alloc = Alloc;
fn main() {
unsafe `{
Alloc::init();
let mut x = Box::new(0);
ptr::write_volatile(&mut *x, 1);
}
}Compiled for ARMv7-M with opt-level=s produces:
|
|
rust-lang/rfcs#2492 provides a more general way to defer the definition of the OOM hook (along with the other global resources defined in |
There was a problem hiding this comment.
Thanks for the @SimonSapin! On the technical side of things this looks pretty good to me.
One thing I'm not sure about, though, is the story around liballoc and stabilization in general. In that sense this is more related to rust-lang/rfcs#2480, but I think that this PR may want to hold off on landing until rust-lang/rfcs#2480 is decided on.
My main concern is how we've got this weird #![default_lib_allocator] attribute which is the only way to customize the default method of oom printing/aborting. This seems like it's sort of a hack to get extern crate alloc with no other definitions continuing to work tomorrow as it does today. One possible route we may want to take, for example, is to forbid extern crate alloc unless the crate graph somehow defines what to do on OOM or how to abort.
In any case I can comment with more info on the RFC!
| // This function is generated by the compiler | ||
| // and calls __rust_abort_internal in src/libstd/alloc.rs | ||
| // if the `std` crate is linked. | ||
| fn __rust_maybe_abort_internal(); |
There was a problem hiding this comment.
Initially reading this I was a little confused by this about why both the hook and the abort mechanism were configurable. I think, though, it's to do sys::abort_internal, right? (as opposed to intrinsics::abort)
If that's the case, mind adding a small comment here alluding to that?
There was a problem hiding this comment.
it's to do sys::abort_internal, right? (as opposed to intrinsics::abort)
Yes, exactly
| }; | ||
|
|
||
| for method in OOM_HANDLING_METHODS { | ||
| let callee = if has_plaftom_functions { |
I definitely view
Yes, this is an alternative being discussed in the tracking issue #51540. I find @japaric’s arguments for static dispatch (rather than dynamic hooks with an atomic function pointer) to be convincing, so I was tempted to close this PR. (I’m less decided on deliberately not providing a default, though.)
Please do comment with your thoughts on rust-lang/rfcs#2480, but I think that it is fairly orthogonal to this. Stabilizing the crate with OOM handling still unstable already unblocks the use case of libraries that want to be both Stable-compatible and |
|
FWIW an attribute like |
|
Alright. Closing in favor of something statically-dispatched. Let’s discuss in #51540. |
This removes the need for
no_stdprograms to define that lang item themselves: fixes #51540