std: Mark allocation functions as nounwind #44049

alexcrichton · 2017-08-22T22:12:23Z

This commit flags all allocation-related functions in liballoc as "this can't
unwind" which should largely resolve the size-related issues found on #42808.
The documentation on the trait was updated with such a restriction (they can't
panic) as well as some other words about the relative instability about
implementing a bullet-proof allocator.

Closes #42808

rust-highfive · 2017-08-22T22:12:33Z

r? @BurntSushi

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

alexcrichton · 2017-08-22T22:13:28Z

This is perhaps best exemplified with this test which has no invoke instructions on stable and has invoke instructions on nightly. After this PR there are no invoke instructions either.

[beta] Final round of beta backports Includes: * [x] #43723 * [x] #43830 * [x] #43844 * [x] #44013 * [x] #44049 * [x] #43948

retep998 · 2017-08-23T21:20:53Z

Windows required unwind tables for all functions because anything can unwind due to an SEH exception. Does this PR make sure to not break that?

alexcrichton · 2017-08-23T21:39:18Z

That's uwtable, not nounwind

petrochenkov · 2017-08-24T00:02:05Z

src/liballoc/heap.rs

@@ -27,36 +27,46 @@ pub mod __core {

 extern "Rust" {
    #[allocator]
+    #[allocator_nounwind]


Hmm, this is a new ungated attribute, right?
Since it's internal and the name doesn't matter much, rustc_allocator_nounwind can be used instead, it'll be gated automatically.

Sure yeah, I can change the name (currently this is just allowed under "custom_attributes")

pnkfelix · 2017-08-24T08:41:26Z

src/liballoc/allocator.rs

+///   retain their validity until at least the instance of `Alloc` is dropped
+///   itself.
+///
+/// * Implementations of `Alloc` are not currently memory safe if they unwind.


Wait, I'm not clear on how to parse this. The way it reads now, I interpret this as saying that any implementation of Alloc is not memory safe if it has a method that unwinds (panics), which AFAICT would even include the fn oom method...

But if I'm understanding the change here correctly, the memory unsafety only arises for implementations that are installed as global allocators, right?

(Maybe we do need to make a unsafe trait GlobalAllocator: Allocator { } just to track this extra-constraint on the implementations then...?)

Note that if I am wrong, and that this somehow is relevant to all implementations of Alloc, including ones that cannot be installed as global allocators, then I think more of the documentation for this trait needs to be revised.

E.g. text like this on fn alloc implies that an implementor may panic (even if it is not recommended):

Implementations are encouraged to return Err on memory exhaustion rather than panicking or aborting, but this is not a strict requirement.

Oh right yeah, I intended to make this specific to just global allocators

alexcrichton · 2017-08-24T14:21:33Z

Updated

bluss · 2017-08-24T15:13:48Z

src/liballoc/allocator.rs

+///   in the future, but currently an panic from any of these functions may lead
+///   to memory unsafety. Note that as of the time of this writing allocators
+///   *not* intending to be global allocators can still in their implementation
+///   without violating memory safety.


There's a word missing in this sentence: panic/unwind 😃

arielb1 · 2017-08-28T12:16:59Z

src/liballoc/allocator.rs

+///   itself.
+///
+/// * Implementations of `Alloc` intended to be used for global allocators are
+///   not currently memory safe if they unwind.  This restriction may be lifted


I would prefer "it's undefined behavior if a global allocator unwinds" to "not currently memory safe".

"not memory safe" implies there's an edge case in which UB can happen, but allocators unwinding is always UB.

alexcrichton · 2017-08-28T14:51:59Z

Beta-nominating again as this needs to be backported again.

BurntSushi

I don't have a ton of context on the allocator stuff, so I'm probably not the best person to review this, but it LGTM.

BurntSushi · 2017-08-28T14:55:08Z

src/liballoc/allocator.rs

+///   itself.
+///
+/// * It's undefined behavior if global allocators unwind.  This restriction may
+///   be lifted in the future, but currently an panic from any of these


This commit flags all allocation-related functions in liballoc as "this can't unwind" which should largely resolve the size-related issues found on rust-lang#42808. The documentation on the trait was updated with such a restriction (they can't panic) as well as some other words about the relative instability about implementing a bullet-proof allocator. Closes rust-lang#42808

alexcrichton · 2017-08-28T15:07:04Z

@bors: r=BurntSushi

bors · 2017-08-28T15:07:05Z

📌 Commit b6f554b has been approved by BurntSushi

bors · 2017-08-29T00:58:25Z

⌛ Testing commit b6f554b with merge d2d5069...

std: Mark allocation functions as nounwind This commit flags all allocation-related functions in liballoc as "this can't unwind" which should largely resolve the size-related issues found on #42808. The documentation on the trait was updated with such a restriction (they can't panic) as well as some other words about the relative instability about implementing a bullet-proof allocator. Closes #42808

bors · 2017-08-29T03:35:08Z

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

[beta] Boostrap from released compilers No need to use dev any more! Also contains backports for: * #44049 * #44121 * #44144

bjorn3 · 2017-09-07T16:47:16Z

src/liballoc/allocator.rs

+///
+/// * Pointers returned from allocation functions must point to valid memory and
+///   retain their validity until at least the instance of `Alloc` is dropped
+///   itself.


This seems to imply that dealloc doesn't make the pointer invalid, as it doesn't drop the Alloc instance

rust-highfive assigned BurntSushi Aug 22, 2017

alexcrichton mentioned this pull request Aug 22, 2017

Binary size blowup presumably because of allocator integration #42808

Closed

alexcrichton added beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 22, 2017

alexcrichton force-pushed the nounwind-allocators branch from b04f587 to 021135c Compare August 22, 2017 22:36

alexcrichton mentioned this pull request Aug 23, 2017

[beta] Final round of beta backports #44062

Merged

6 tasks

bors added a commit that referenced this pull request Aug 23, 2017

Auto merge of #44062 - alexcrichton:beta-next, r=alexcrichton

dcf532b

[beta] Final round of beta backports Includes: * [x] #43723 * [x] #43830 * [x] #43844 * [x] #44013 * [x] #44049 * [x] #43948

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 23, 2017

alexcrichton mentioned this pull request Aug 23, 2017

Tracking issue for oom=panic (RFC 2116) #43596

Open

6 tasks

petrochenkov reviewed Aug 24, 2017

View reviewed changes

pnkfelix reviewed Aug 24, 2017

View reviewed changes

alexcrichton force-pushed the nounwind-allocators branch from 021135c to d37b501 Compare August 24, 2017 14:21

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2017

alexcrichton force-pushed the nounwind-allocators branch from d37b501 to 956c2c0 Compare August 24, 2017 14:27

bluss reviewed Aug 24, 2017

View reviewed changes

alexcrichton force-pushed the nounwind-allocators branch from 956c2c0 to a4bfc4d Compare August 24, 2017 18:20

arielb1 reviewed Aug 28, 2017

View reviewed changes

alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 28, 2017

alexcrichton force-pushed the nounwind-allocators branch from a4bfc4d to 3947bd2 Compare August 28, 2017 14:53

BurntSushi approved these changes Aug 28, 2017

View reviewed changes

alexcrichton force-pushed the nounwind-allocators branch from 3947bd2 to b6f554b Compare August 28, 2017 15:06

bors merged commit b6f554b into rust-lang:master Aug 29, 2017

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 1, 2017

alexcrichton mentioned this pull request Sep 1, 2017

[beta] Boostrap from released compilers #44235

Merged

bors added a commit that referenced this pull request Sep 1, 2017

Auto merge of #44235 - alexcrichton:beta-next, r=alexcrichton

12ac2b6

[beta] Boostrap from released compilers No need to use dev any more! Also contains backports for: * #44049 * #44121 * #44144

alexcrichton deleted the nounwind-allocators branch September 1, 2017 23:05

bors added a commit that referenced this pull request Sep 2, 2017

Auto merge of #44235 - alexcrichton:beta-next, r=alexcrichton

133aefc

[beta] Boostrap from released compilers No need to use dev any more! Also contains backports for: * #44049 * #44121 * #44144

bjorn3 reviewed Sep 7, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std: Mark allocation functions as nounwind #44049

std: Mark allocation functions as nounwind #44049

alexcrichton commented Aug 22, 2017

rust-highfive commented Aug 22, 2017

alexcrichton commented Aug 22, 2017

retep998 commented Aug 23, 2017

alexcrichton commented Aug 23, 2017

petrochenkov Aug 24, 2017

alexcrichton Aug 24, 2017

pnkfelix Aug 24, 2017

pnkfelix Aug 24, 2017

alexcrichton Aug 24, 2017

alexcrichton commented Aug 24, 2017

bluss Aug 24, 2017

arielb1 Aug 28, 2017

arielb1 Aug 28, 2017

alexcrichton commented Aug 28, 2017

BurntSushi left a comment

BurntSushi Aug 28, 2017

alexcrichton commented Aug 28, 2017

bors commented Aug 28, 2017

bors commented Aug 29, 2017

bors commented Aug 29, 2017

bjorn3 Sep 7, 2017

std: Mark allocation functions as nounwind #44049

std: Mark allocation functions as nounwind #44049

Conversation

alexcrichton commented Aug 22, 2017

rust-highfive commented Aug 22, 2017

alexcrichton commented Aug 22, 2017

retep998 commented Aug 23, 2017

alexcrichton commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Aug 28, 2017

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Aug 28, 2017

bors commented Aug 28, 2017

bors commented Aug 29, 2017

bors commented Aug 29, 2017

Choose a reason for hiding this comment