alloc: Add unstable Cfg feature `no_global_oom_handling

[Ericson2314]

For certain sorts of systems, programming, it's deemed essential that
all allocation failures be explicitly handled where they occur. For
example, see Linus Torvald's opinion in 1. Merely not calling global
panic handlers, or always try_reserving first (for vectors), is not
deemed good enough, because the mere presence of the global OOM handlers
is burdens static analysis.

One option for these projects to use rust would just be to skip alloc,
rolling their own allocation abstractions. But this would, in my
opinion be a real shame. alloc has a few try_* methods already, and
we could easily have more. Features like custom allocator support also
demonstrate and existing to support diverse use-cases with the same
abstractions.

A natural way to add such a feature flag would a Cargo feature, but
there are currently uncertainties around how std library crate's Cargo
features may or not be stable, so to avoid any risk of stabilizing by
mistake we are going with a more low-level "raw cfg" token, which
cannot be interacted with via Cargo alone.

Note also that since there is no notion of "default cfg tokens" outside
of Cargo features, we have to invert the condition from
global_oom_handling to to not(no_global_oom_handling). This breaks
the monotonicity that would be important for a Cargo feature (i.e.
turning on more features should never break compatibility), but it
doesn't matter for raw cfg tokens which are not intended to be
"constraint solved" by Cargo or anything else.

To support this use-case we create a new feature, "global-oom-handling",
on by default, and put the global OOM handler infra and everything else
it that depends on it behind it. By default, nothing is changed, but
users concerned about global handling can make sure it is disabled, and
be confident that all OOM handling is local and explicit.

For this first iteration, non-flat collections are outright disabled.
Vec and String don't yet have try_* allocation methods, but are
kept anyways since they can be oom-safely created "from parts", and we
hope to add those try_ methods in the future.

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[Ericson2314]

I added the WIP because something in alloc is requiring exchange_malloc, yet I don't see any box expressions except for in tests, and when I dumped MIR for the module I didn't see it used.

[Ericson2314]

Also not sure why that failure happened, because I don't see any default-features = false or similar for alloc.

[bjorn3]

I added the WIP because something in alloc is requiring exchange_malloc, yet I don't see any box expressions except for in tests, and when I dumped MIR for the module I didn't see it used.

Box::new needs it.

[the8472]

This is a lot of cfg annotations. Would it make sense to group functions those into internal modules instead? E.g. vec::fallible and vec::infallible both containing impl blocks for the same types and then putting the cfg on the whole module.

Of course that'll require a lot of code motion now, but but it provides clearer separation and it'll be more obvious in the future when people add things in the wrong place.

[phil-opp]

Also not sure why that failure happened, because I don't see any default-features = false or similar for alloc.

The test seems to use rustc directly. I think that (default) features are normally handled by cargo and only passed to rustc as --cfg argument, so we probably need to adjust the test to enable the global_oom_handling feature explicitly.

[Aaron1011]

What happens if someone forgets to put #[cfg(feature = "global-oom-handling")] on an API that depends on global OOM handling? Will compilation fail with the feature disabled, or will the user get a weird runtime problem?

[bjorn3]

It will fail with a compilation error as handle_alloc_error is cfg'ed away.

[Ericson2314]

Indeed that is what's happening with right now.

I should have those 2 things fixed in a bit. Thanks for the tips, everyone.

[Ericson2314]

@the8472 It is a lot of cfg, but I think the "boundaries" between e.g. vec::fallible and vec::infallible would change a lot as more try_* methods are created. That sort of churn will cause massive headaches with PR conflicts, so I think gratuitous cfg is in fact the far lesser evil, at least until we decide how many try_* methods we want and stabilize around that.

[Ericson2314]

The PR is passing, but it would be nice to have a test that the build with global-oom-handling disabled doesn't break. How ought I to do that?

[bjorn3]

This test doesn't seem to test anything anymore: https://github.com/rust-lang/rust/search?q=external_crate

[bjorn3]

By the way I think you can use the same pattern to test liballoc without the global-oom-handling feature.

[Ericson2314]

Agreed it does seem that way.

[the8472]

@the8472 It is a lot of cfg, but I think the "boundaries" between e.g. vec::fallible and vec::infallible would change a lot as more try_* methods are created. That sort of churn will cause massive headaches with PR conflicts, so I think gratuitous cfg is in fact the far lesser evil, at least until we decide how many try_* methods we want and stabilize around that.

I'm not sure what you're concerned about. Potential churn in the long-term once those modules are established or the way to creating those modules?

If it's about the latter then we can probably minimize the conflict potential by only creating a fallible module and moving the try_ code and its fallible callees there. That should be a fairly small API surface right now and thus not need too much code movement. Once that is done we only need to slap a cfg on top of the impl blocks which contain infallible APIs to disable them.

[Ericson2314]

I'm not sure what you're concerned about. Potential churn in the long-term once those modules are established or the way to creating those modules?

Churn in the short term why try_ methods are being created.

If it's about the latter then we can probably minimize the conflict potential by only creating a fallible module and moving the try_ code and its fallible callees there. That should be a fairly small API surface right now and thus not need too much code movement. Once that is done we only need to slap a cfg on top of the impl blocks which contain infallible APIs to disable them.

When a try_ method is made, usually what happens is the existing implementation is refactored into the try_ version, and then the old one becomes a small wrapper around that. So with even just fallible module, we have a bunch of churn when the implementation gets refactored and moved from the original module to fallible one. And git handles refactor + moves extremely poorly.

[the8472]

Churn in the short term why try_ methods are being created.

I don't understand what you mean by that. The try methods have to be created either way. They're not just created for a short term reason and then removed later. So "churn in the short term" cannot be their raison d'être.

When a try_ method is made, usually what happens is the existing implementation is refactored into the try_ version, and then the old one becomes a small wrapper around that. So with even just fallible module, we have a bunch of churn when the implementation gets refactored and moved from the original module to fallible one. And git handles refactor + moves extremely poorly.

I don't see the difference here. You need to create a new method which is sortof but not quite copy-pasted from the old one. And then redirect the old one to the new one. This has to happen either way whether they live in different files or not and whether you annotate on the function level or not. And if necessary can be split into multiple commits to make it easier for git to follow the movement.

E.g. you could implement the try method first in the base module and then once it is done use a separate commit to move it into the fallible module.

Yes, in the short-term this means a little more work, but in the long-term it makes it clearer which part belongs to the restricted fallible API set and which to the more lax infallible APIs.

[bjorn3]

And if necessary can be split into multiple commits to make it easier for git to follow the movement.

Git blame doesn't track moves between files at all unless you pass it -C. The github ui doesn't have any option to enable this and most people likely don't know about it at all.

I don't see the difference here. You need to create a new method which is sortof but not quite copy-pasted from the old one. And then redirect the old one to the new one. This has to happen either way whether they live in different files or not and whether you annotate on the function level or not.

When doing it within the same file, git will consider most lines of non-try function in the previous version to be part of the try function in the current version. Take for example 92bfcd2#diff-0e10470fe128e30b8710381cb322bfe266884aa8d969619a9920fc6970cdceffL406-R450:

@@ -403,7 +405,9 @@ impl<T, A: Alloc> RawVec<T, A> {
     /// # Aborts
     ///
     /// Aborts on OOM
-    pub fn reserve_exact(&mut self, used_cap: usize, needed_extra_cap: usize) {
+    pub fn try_reserve_exact(&mut self, used_cap: usize, needed_extra_cap: usize)
+           -> Result<(), CollectionAllocErr> {
+
         unsafe {
             // NOTE: we don't early branch on ZSTs here because we want this
             // to actually catch "asking for more than usize::MAX" in that case.
@@ -413,16 +417,15 @@ impl<T, A: Alloc> RawVec<T, A> {
             // Don't actually need any more capacity.
             // Wrapping in case they gave a bad `used_cap`.
             if self.cap().wrapping_sub(used_cap) >= needed_extra_cap {
-                return;
+                return Ok(());
             }
 
             // Nothing we can really do about these checks :(
-            let new_cap = used_cap.checked_add(needed_extra_cap).expect("capacity overflow");
-            let new_layout = match Layout::array::<T>(new_cap) {
-                Some(layout) => layout,
-                None => panic!("capacity overflow"),
-            };
-            alloc_guard(new_layout.size());
+            let new_cap = used_cap.checked_add(needed_extra_cap).ok_or(CapacityOverflow)?;
+            let new_layout = Layout::array::<T>(new_cap).ok_or(CapacityOverflow)?;
+
+            alloc_guard(new_layout.size())?;
+
             let res = match self.current_layout() {
                 Some(layout) => {
                     let old_ptr = self.ptr.as_ptr() as *mut u8;
@@ -430,26 +433,34 @@ impl<T, A: Alloc> RawVec<T, A> {
                 }
                 None => self.a.alloc(new_layout),
             };
-            let uniq = match res {
-                Ok(ptr) => Unique::new_unchecked(ptr as *mut T),
-                Err(e) => self.a.oom(e),
-            };
-            self.ptr = uniq;
+
+            self.ptr = Unique::new_unchecked(res? as *mut T);
             self.cap = new_cap;
+
+            Ok(())
         }
     }
 
+    pub fn reserve_exact(&mut self, used_cap: usize, needed_extra_cap: usize) {
+        match self.try_reserve_exact(used_cap, needed_extra_cap) {
+            Err(CapacityOverflow) => panic!("capacity overflow"),
+            Err(AllocErr(e)) => self.a.oom(e),
+            Ok(()) => { /* yay */ }
+         }
+     }
+

[Ericson2314]

Thanks @bjorn3 for laying this out much more clearly than I would have.

[the8472]

Git blame doesn't track moves between files at all unless you pass it -C. The github ui doesn't have any option to enable this and most people likely don't know about it at all.

I assume you're using -C already, otherwise the occasional refactor that splits large modules into smaller ones (as happend not too long ago with Vec) would cause the very same issues.
And since there's no ban on such split operations (in fact they're encouraged by tidy's file length limit) why not do another one to isolate the fallible methods?

When doing it within the same file, git will consider most lines of non-try function in the previous version to be part of the try function in the current version.

Yes, hence do that in one commit, then do the motion in another so that git history browsing can follow it more easily.

[Ericson2314]

@the8472 I'm not against ever rearranging code --- quite in support in fact. But I imagine the try_ methods will be added incrementally over a long period of time. (We could do them all at once, but I'm afraid it's rather controversial, so I don't think that will happen.)

Assuming I'm correct in saying there will be many commits adding try_ methods, I rather have one big reorg at the end, then constant reorging, because it's much less a burden on others.

[the8472]

Ok, that might work. But on the other hand the current approach also means people have to reason about fallibility on a per-method level. And if methods are added incrementally that would also keep church limited in impact, so moving wouldn't be that bad either?

[safinaskar]

Important news! HashMap got method try_insert ( https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.try_insert ), where try_ means something completely different to Box::try_new. Please, prevent somehow this try_insert from becoming stable