docs: GlobalAlloc: completely replace example with one that works

[ijackson]

Since this is an example, this could really do with some review from someone familiar with unsafe stuff!

I made the example no longer no_run since it works for me.

Fixes #81847

[rust-highfive]

r? @sfackler

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

[bluss]

The allocator returns null on allocation failure, it does not abort

[ijackson]

The allocator returns null on allocation failure, it does not abort

Ideally, yes. But with the current structure of the critical section (no conditional other than the one inherent in fetch_update, the counter would eventually overflow after many many repeated allocations.

Although this is only a demo, I think your implication, that this ought to be addressed, is right, because it's supposed to be demoing the GlobalAlloc trait. I will improve this.

[bluss]

I would make it limited (in features), complete and correct. Use a Mutex to simplify the code for the reader. Compute the needed bump for the allocation, if there is no more space, return null, otherwise "commit" to it and update and release the lock. Limited could mean that it only supports one alignment (everything is 16-aligned and larger aligns fail to allocate?) or other realistic restriction, just for this example. just to make the example easier to read and write. (This way we avoid thinking about atomics and arithmetic overflow.)

If Mutex is not available then I guess that's a shame and maybe the example can be simplified using the same idea but with an atomic.

[ijackson]

I like the suggestion of trying to use a Mutex. Unfortunately they can't be constructed in const context (AIUI because of Windows). I think fetch_update is a good primitive to use but I take your point that I could just bung everything relevant into the critical section which would simplify matters considerably.

The alignment comes out much nicer when counting down so it may turn out to be shorter to keep the alignment support than special case it.

[ijackson]

I've updated it quite a lot, after review of the GlobalAlloc docs. Now it counts downwards which gets rid of most of the bit-twiddling. It has more code and comments to handle exceptions and corner cases, to illustrate the subtleties of the API.

[ijackson]

How about this. Moving the error handling into the critical section has considerably simplified it. Almost all the bit-twiddling is gone. I replaced the special arithmetic operations with plain -= etc.

[ijackson]

I'm not sure the comment for align_mask_to_round_down is pulling its weight, but it does discuss an aspect of the GlobalAlloc API. Deleting it might make the code clearer rather than less clear.

[Dylan-DPC-zz]

@sfackler this is ready for review

[Dylan-DPC-zz]

@sfackler this is ready for review

oops

[sfackler]

I'm still concerned that the example both:

• is not a demonstration of an actual "real-world" implementation.
• is 90% atomics and bit-twiddling.

If we want to emphasize that alignment requirements must be respected, then we can talk about it in the docs. I don't think we need to force an arbitrarily artificial example just for its own sake.

[ijackson]

I'm still concerned that the example both:

* is not a demonstration of an actual "real-world" implementation.

I don't agree that this isn't a "real-world" implementation. I think this implementation would be suitable for an embedded environment where only a small number of allocations will ever occur.

I seriously anticipate that someone might c&p this into their microcontroller.

* is 90% atomics and bit-twiddling.

I recognise this is hyperbole but I wanted to pick at it anyway.

The example is 49 lines (l.26 to 74 of the file). Of those, lines 30, 47, 49-52, and 59 (6 lines in all) would be gone in an implementation which didn't care about or check alignment (of which only lines 49,51 and 59 are actually bit-twiddling). Lines 27, 62-64 and arguably half of each of 33, 39 and 72 (call that 6 lines in all) would be gone in an implementation which didn't care about threadsafety.

I make that an absolute outside of 12 lines of "atomics and bit-twiddling" - 25%. I don't think it would be sensible to produce an example which pretended to work but didn't honour alignment, or which wasn't threadsafe, if it saves only a quarter of the size.

The example is full of what one might reasonably think of as "weird shit" - the combination of raw pointers, UnsafeCell and atomics. But that comes with the territory. This "weird shit" density, and the number of available beartraps in that territory, is a big reason why I think a properly worked out example, which gets everything right, is valuable.

If we want to emphasize that alignment requirements must be respected, then we can talk about it in the docs. I don't think we need to force an arbitrarily artificial example just for its own sake.

I think the best options are something like this example; no example; or a very minimal dummy example - eg as suggested by @vitalyd. The latter is still a fair chunk of text, despite not illuminating much more than how to spell and place the #[global_allocator] attribute. If that's all that's wanted perhaps it could be trimmed down by replacing everything with just calls to abort and making it a compile-only test.

If the libs team doesn't feel a proper example is worth the space here then please do feel free to close this MR and leave #81847 open for someone to fix another way. Obviously that would be disappointing to me but it's not like there aren't other things I could be trying to fix :-).

Alternatively, perhaps this particular example, which demonstrates not just GlobalAlloc but also UnsafeCell, raw pointers, and atomics, and might well be deployable as-is in some environments even, would be better placed somewhere else. Then the GlobalAlloc docs could perhaps have a reference to it.

Personally ISTM that Rust nowadays likes to put stuff like that in the docs examples, rather than off in some external place which has to be separately hunted down (and isn't tested in CI, etc.). And it's not like the docs for GlobalAlloc are going to be read by Rust newbies. There's already a large pile of text there and I think a fully worked example is simply commensurate.

Thanks.

[ijackson]

Thanks, folded in your suggestion and squashed.

[Amanieu]

@bors r+

[Amanieu]

RIP bors?

@bors r+

[Amanieu]

@bors r+

[bors]

📌 Commit 07e11e8 has been approved by Amanieu

[bors]

⌛ Testing commit 07e11e8 with merge cf975bbd6418f1571394d60fb875ef5880c24b6e...

[bors]

💔 Test failed - checks-actions

[Amanieu]

2021-07-21T03:30:44.9825882Z ---- src\alloc\global.rs - alloc::global::GlobalAlloc (line 23) stdout ----
2021-07-21T03:30:44.9826661Z unsupported section alignment
2021-07-21T03:30:44.9827523Z UNREACHABLE executed at D:\a\rust\rust\src\llvm-project\llvm\lib\MC\WinCOFFObjectWriter.cpp:297!
2021-07-21T03:30:44.9828342Z Couldn't compile the test.
2021-07-21T03:30:44.9828626Z 
2021-07-21T03:30:44.9828931Z failures:
2021-07-21T03:30:44.9829454Z     src\alloc\global.rs - alloc::global::GlobalAlloc (line 23)

statics don't support alignments greater than 4k: #70022 #70144

[ijackson]

statics don't support alignments greater than 4k: #70022 #70144

Oh dear. I will fix this. Shouldn't be too hard.

[Amanieu]

@bors r+

[bors]

📌 Commit 03d7001 has been approved by Amanieu