Avoid zero-sized allocs in ThinBox if T and H are both ZSTs.

[thomcc]

This was surprisingly tricky, and took longer to get right than expected. ThinBox is a surprisingly subtle piece of code. That said, in the end, a lot of this was due to overthinking¹ -- ultimately the fix ended up fairly clean and simple.

Anyway, as a result of spending all that time debugging, I've extended the tests quite a bit, and also added more debug assertions. Many of these helped for subtle bugs I made in the middle (for example, the alloc/drop tracking is because I ended up double-dropping the value in the case where both were ZSTs), they're arguably a bit of overkill at this point, although I imagine they could help in the future too.

Anyway, these tests cover a wide range of size/align cases, nd fully pass under miri². They also do some smoke-check asserting that the value has the correct alignment, although in practice it's totally within the compiler's rights to delete these assertions since we'd have already done UB if they get hit. They have more boilerplate than they really need, but it's not too bad on a per-test basis.

A notable absence from testing is atypical header types, but at the moment it's impossible to manually implement Pointee. It would be really nice to have testing here, since it's not 100% obvious to me that the aligned read/write we use for H are correct in the face of arbitrary combinations of size_of::<H>(), align_of::<H>(), and align_of::<T>(). (That said, I spent a while thinking through it and am pretty sure it's fine -- I'd just feel... better if we could test some cases for non-ZST headers which have unequal and align).

Fixes #96485.

I'd request review @yaahc, but I believe you're taking some time away from reviews, so I'll request from the previous PR's reviewer (I think that the context helps, even if the actual change didn't end up being bad here).

r? @joshtriplett

Footnotes

1. Honestly, for a while I was convinced this couldn't be done without allocations or runtime branches in these cases, but that's obviously untrue. ↩

2. Or at least, they pass under miri if I copy the code and tests into a new crate and run miri on it (after making it less stdlibified). ↩

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[thomcc]

This doesn't touch the public API in any way (aside from fixing the unsoundness in ThinBox<()>).

[thomcc]

To explain why I think that the previous (and current) code is sound despite using aligned read/write for H (and also sort of why I think it's not trivial, or at least is not to me), my reasoning is something like this:

If we consider allocation as returning (base: *mut u8, offset: usize), where offset is the second value returned by Layout::extend, and base is the poiner returned from the allocator. (Note: error handling is ignored below, as is the case where both T and H are ZSTs, although I don't believe it needs special handling here). Then:

1. base is a pointer aligned to max(align(H), align(T)).
2. offset is (size(H) + (align(T) - 1)) & (align(T) - 1)). Or, equivalently, it's the result of rounding size(H) up to align(T).
3. base + offset produces the value pointer (which I'll call val_ptr), which is guaranteed to be aligned to align(T), as it's the result of adding offset (which is a multiple of align(T)) to base (which is aligned to T).
4. Then, val_ptr - size(H) is the pointer to the header (which I'll call hdr_ptr).

(The above is all how the code currently works, although seeing this requires peeking inside the abstractions provided by Layout::extend and such).

As it is, the code relies on hdr_ptr being aligned to align(H). I... think this is guaranteed? Since it's all powers of 2. But it's tricky. And so I added that debug_assert in WithHeader::header, and would eventually like tests for that case.

Anyway, I mostly wanted to write this down, so that I could (possibly) find it in the future. It's essentially unrelated to this patch (beyond, erm, the fact that I broke it in the middle of writing this patch, but fixed it by the end)

[yaahc]

I'm down to review code I'm familiar with still! Just taking a break from the rotation, not reviews in general.

Which I guess is my way of saying I want to review this :D

[yaahc]

Looks great, thanks for showing ThinBox some love ^_^

@bors r+

[bors]

📌 Commit 25164b4 has been approved by yaac

[thomcc]

(Sorry, bors missed it the first time, then I typoed your name 😓)

@bors r=yaahc

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: Remove all json handling from rustc_serialize #85993

[bors]

📌 Commit 25164b4 has been approved by yaahc