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

Implement #[alloc_error_handler] #52110

Closed
wants to merge 3 commits into from
Closed

Implement #[alloc_error_handler] #52110

wants to merge 3 commits into from

Conversation

SimonSapin
Copy link
Contributor

This to-be-stable attribute is equivalent to #[lang = "oom"]. It is required when using the alloc crate without the std crate. It is called by handle_alloc_error, which is in turned called by "infallible" allocations APIs such as Vec::push.

Tracking issue: #51540

@SimonSapin
Add a test for the default allocation error hook
b555e0f
@SimonSapin
Implement #[alloc_error_handler]
8101344 
This to-be-stable attribute is equivalent to `#[lang = "oom"]`.
It is required when using the alloc crate without the std crate.
It is called by `handle_alloc_error`, which is in turned called
by "infallible" allocations APIs such as `Vec::push`.
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 6, 2018
@SimonSapin
Copy link
Contributor Author

CC @alexcrichton, @japaric, @glandium, @Amanieu

@petrochenkov
Copy link
Contributor

r? @japaric

@rust-highfive rust-highfive assigned japaric and unassigned petrochenkov Jul 6, 2018
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 6, 2018

📌 Commit 8101344 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2018
@glandium
Copy link
Contributor

glandium commented Jul 6, 2018

Random thought. If you can't use no_std + alloc without global_allocator and alloc_error_handler, why not put the allocation error handling in the GlobalAlloc trait, like it (kind of) used to be (because that was on the Alloc trait before GlobalAlloc existed). That would even allow a default to exist!

@alexcrichton
Copy link
Member

@glandium the original thinking is that they're separable concerns especially at the libstd layer. Right now this implementation of handling OOM is different from libstd's allocator implementation in that it doesn't provide a fallback automatically if you haven't otherwise provided one. For libstd it makes sense to define how to allocate without defining how to implement OOM, and the same could perhaps one day also be true of alloc with enough work going into this attribute.

@SimonSapin SimonSapin mentioned this pull request Jul 6, 2018
Open
2 tasks
@bors
Copy link
Contributor

bors commented Jul 8, 2018

⌛ Testing commit 8101344 with merge 36adfae3c10ac58714c6a0d93b949ceac314f1d1...

@bors
Copy link
Contributor

bors commented Jul 8, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 8, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-distcheck of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
[01:01:50] test [run-pass] run-pass/default-method-simple.rs ... ok
[01:01:51] test [run-pass] run-pass/defaults-well-formedness.rs ... ok
[01:01:51] test [run-pass] run-pass/default-method-supertrait-vtable.rs ... ok
[01:01:51] test [run-pass] run-pass/deprecation-in-force-unstable.rs ... ok
[01:01:51] test [run-pass] run-pass/default-alloc-error-hook.rs ... ok
[01:01:51] test [run-pass] run-pass/deref-mut-on-ref.rs ... ok
[01:01:51] test [run-pass] run-pass/deref-on-ref.rs ... ok
[01:01:52] test [run-pass] run-pass/deref.rs ... ok
[01:01:52] test [run-pass] run-pass/deref-rc.rs ... ok
---
[01:51:14] warning: spurious network error (2 tries remaining): curl error: Couldn't resolve host 'github.com'
[01:51:14] ; class=Net (12)
[01:52:10] warning: spurious network error (1 tries remaining): curl error: Couldn't resolve host 'github.com'
[01:52:10] ; class=Net (12)
[01:53:06] error: failed to load source for a dependency on `rand`
[01:53:06] Caused by:
[01:53:06]   Unable to update registry `https://github.com/rust-lang/crates.io-index`
[01:53:06] 
[01:53:06] Caused by:
[01:53:06] Caused by:
[01:53:06]   failed to fetch `https://github.com/rust-lang/crates.io-index`
[01:53:06] 
[01:53:06] Caused by:
[01:53:06]   curl error: Couldn't resolve host 'github.com'
[01:53:06] ; class=Net (12)
[01:53:06] 
[01:53:06] 
[01:53:06] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "generate-lockfile" "--manifest-path" "/checkout/obj/build/tmp/distcheck-src/rust-src/lib/rustlib/src/rust/src/libstd/Cargo.toml"
[01:53:06] 
[01:53:06] 
[01:53:06] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test distcheck
[01:53:06] Build completed unsuccessfully in 1:49:57
---
travis_time:end:302fa327:start=1531032879579873433,finish=1531032879586380030,duration=6506597
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:036578be
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1197db33
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

ollie27
ollie27 reviewed Jul 8, 2018
View reviewed changes
src/libstd/alloc.rs Outdated
@@ -125,7 +125,8 @@ fn default_alloc_error_hook(layout: Layout) {

#[cfg(not(test))]
#[doc(hidden)]
#[lang = "oom"]
#[cfg_attr(stage0, lang = "oom")]
#[cfg_attr(not(stage0), alloc_error_handler)]
#[unstable(feature = "alloc_internals", issue = "0")]
pub extern fn rust_oom(layout: Layout) -> ! {
Copy link
Member

Choose a reason for hiding this comment

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

What abi is this function supposed to have? Here it's "C" but elsewhere it seems to be "Rust".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I’ve removed extern to make it Rust ABI.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the abi be checked as part of the signature check?

@SimonSapin
Copy link
Contributor Author

Failure above was curl error: Couldn't resolve host 'github.com'

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 8, 2018

📌 Commit c37a4a0 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2018
@alexcrichton alexcrichton deleted the alloc_error_handler branch July 8, 2018 15:47
@alexcrichton
Copy link
Member

Er sorry, was cleaning out branches on the rust-lang repo and accidentally closed this, can the PR be resent from your own repo @SimonSapin?

@SimonSapin SimonSapin mentioned this pull request Jul 9, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ollie27 ollie27 ollie27 left review comments

Assignees

@japaric japaric

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@SimonSapin @rust-highfive @petrochenkov @alexcrichton @bors @glandium @ollie27 @japaric