Skip to content

/rust

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

Replace {Alloc,GlobalAlloc}::oom with a lang item. #50144

Merged
merged 3 commits into from Apr 22, 2018

Conversation

Reviewers

@eddyb eddyb

@mattico mattico

Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
6 participants
@sfackler @alexcrichton @bors @eddyb @mattico @rust-highfive
@sfackler
Copy link
Member

sfackler commented Apr 21, 2018

The decision of what to do after an allocation fails is orthogonal to the decision of how to allocate the memory, so this PR splits them apart. Alloc::oom and GlobalAlloc::oom have been removed, and a lang item has been added:

#[lang = "oom"]
fn oom() -> !;

It is specifically a weak lang item, like panic_fmt, except that it is required when you depend on liballoc rather than libcore. libstd provides an implementation that aborts with the message fatal runtime error: memory allocation failed, matching the current behavior.

The new implementation is also significantly simpler - it's "just another weak lang item". RFC 2070 specifies a path towards stabilizing panic_fmt, so any complexities around stable weak lang item definition are already being solved.

To bootstrap, oom silently aborts in stage0. alloc_system no longer has a bunch of code to print to stderr, and alloc_jemalloc no longer depends on alloc_system to pull in that code.

One fun note: System's GlobalAlloc implementation didn't override the default implementation of oom, so it currently aborts silently!

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton Apr 21, 2018

@rust-highfive rust-highfive added the S-waiting-on-review label Apr 21, 2018

@sfackler sfackler referenced this pull request Apr 21, 2018

Closed

Tracking issue for the GlobalAlloc trait and related APIs #49668

8 of 8 tasks complete
@mattico

mattico reviewed Apr 21, 2018
View changes

src/librustc_allocator/lib.rs
name: "oom",
inputs: &[],
output: AllocatorTy::Bang,
},

This comment has been minimized.

Sign in to view
Copy link
@mattico

mattico Apr 21, 2018

Contributor

Nit: don't need AllocatorTy::Bang anymore

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Apr 21, 2018

Author Member

Fixed, thanks
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Apr 21, 2018

@bors: r+

That was easier than I thought it would be!
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 21, 2018

📌 Commit b301d05 has been approved by alexcrichton

@bors bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 21, 2018

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 22, 2018

🔒 Merge conflict

@bors bors added S-waiting-on-author and removed S-waiting-on-bors labels Apr 22, 2018

@sfackler sfackler force-pushed the sfackler:oom-lang-item branch from b301d05 to 5969712 Apr 22, 2018

@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member Author

sfackler commented Apr 22, 2018

@bors r=alexcrichton
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 22, 2018

📌 Commit 5969712 has been approved by alexcrichton

@bors bors added S-waiting-on-bors and removed S-waiting-on-author labels Apr 22, 2018

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 22, 2018

⌛️ Testing commit 5969712 with merge e028687...

bors added a commit that referenced this pull request Apr 22, 2018

@bors
Auto merge of #50144 - sfackler:oom-lang-item, r=alexcrichton 
Replace {Alloc,GlobalAlloc}::oom with a lang item.

The decision of what to do after an allocation fails is orthogonal to the decision of how to allocate the memory, so this PR splits them apart. `Alloc::oom` and `GlobalAlloc::oom` have been removed, and a lang item has been added:

```rust
#[lang = "oom"]
fn oom() -> !;
```

It is specifically a weak lang item, like panic_fmt, except that it is required when you depend on liballoc rather than libcore. libstd provides an implementation that aborts with the message `fatal runtime error: memory allocation failed`, matching the current behavior.

The new implementation is also significantly simpler - it's "just another weak lang item". [RFC 2070](https://github.com/rust-lang/rfcs/blob/master/text/2070-panic-implementation.md) specifies a path towards stabilizing panic_fmt, so any complexities around stable weak lang item definition are already being solved.

To bootstrap, oom silently aborts in stage0. alloc_system no longer has a bunch of code to print to stderr, and alloc_jemalloc no longer depends on alloc_system to pull in that code.

One fun note: System's GlobalAlloc implementation didn't override the default implementation of oom, so it currently aborts silently!

r? @alexcrichton
Loading status checks…
e028687
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e028687 to master...

@bors bors merged commit 5969712 into rust-lang:master Apr 22, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@eddyb

eddyb reviewed Apr 25, 2018
View changes

src/liballoc/heap.rs
@@ -59,7 +59,7 @@ unsafe impl<T> Alloc for T where T: CoreAlloc {
}

fn oom(&mut self, _: AllocErr) -> ! {
CoreAlloc::oom(self)
unsafe { ::core::intrinsics::abort() }

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Apr 25, 2018

Member

Why doesn't this use the weak lang item?

This comment has been minimized.

Sign in to view
Copy link
@sfackler

sfackler Apr 25, 2018

Author Member

This is only used for stage0 I believe.
@eddyb

eddyb reviewed Apr 25, 2018
View changes

src/librustc/middle/lang_items.rs
@@ -303,7 +303,8 @@ language_item_table! {

ExchangeMallocFnLangItem, "exchange_malloc", exchange_malloc_fn;
BoxFreeFnLangItem, "box_free", box_free_fn;
DropInPlaceFnLangItem, "drop_in_place", drop_in_place_fn;
DropInPlaceFnLangItem, "drop_in_place", drop_in_place_fn;
OomLangItem, "oom", oom;

This comment has been minimized.

Sign in to view
Copy link
@eddyb

eddyb Apr 25, 2018

Member

Are weak lang items not grouped in here?

@sfackler sfackler deleted the sfackler:oom-lang-item branch Apr 25, 2018

@ekmartin ekmartin referenced this pull request Apr 27, 2018

Merged

Use oom lang item #3

@SimonSapin SimonSapin referenced this pull request Apr 27, 2018

Merged

Upgrade to rustc 1.27.0-nightly (7f3444e1b 2018-04-26) #43

phil-opp added a commit to phil-opp/linked-list-allocator that referenced this pull request Apr 27, 2018

@phil-opp
The oom function is now a lang item 
See rust-lang/rust#50144
f0de36d

@jlamb-at-polysync jlamb-at-polysync referenced this pull request Apr 30, 2018

Closed

Add out-of-memory language item #1

@chocol4te chocol4te referenced this pull request May 6, 2018

Closed

Use OOM lang item #14

@badboy badboy referenced this pull request May 17, 2018

Merged

Update syn and quote, and offload options parsing to darling #120

1 of 1 task complete

@glandium glandium referenced this pull request May 31, 2018

Open

Tracking issue for the OOM hook #51245

1 of 3 tasks complete

@japaric japaric referenced this pull request Jun 20, 2018

Closed

undefined reference to 'rust_begin_unwind': {start,end}-group missing when using an allocator #51647

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.