Fix DST size_of_val and align_of_val computations #27351
Conversation
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
|
I want to stress this point i just added to the issue description: fixes make check on --enable-optimize --enable-debug builds (and thus this, or something like it, is a prerequisite for adding a buildbot instance that covers |
|
@pnkfelix here's a testcase that should trigger the same problem: |
…ion. This is trickier than it sounds (because the DST code was written assuming that one could divide the sized and unsized portions of a type strictly into a sized prefix and unsized suffix, when it reality it is more like a sized prefix and sized suffix that surround the single unsized field). I chose to put in a pretty hack-ish approach to this because drop-flags are scheduled to go away anyway, so its not really worth the effort to to make an infrastructure that sounds as general as the previous paragraph indicates. Also, I have written notes of other fixes that need to be made to really finish fixing rust-lang#27023, namely more work needs to be done to account for alignment when computing the size of a value.
|
@pnkfelix as long as llvm assertions are enabled, it shouldn't matter. I get the error with the regular nightly, which AFAIK isn't even built with |
|
@dotdash ah, I might have had LLVM assertions disabled on that build, let me check. Confimed: LLVM assertions were disabled on that configuration that I tested. :( |
|
I am of the opinion that no RFC is required for this, this is a bug fix. |
pnkfelix
force-pushed
the
dst-size-and-align-issue-27023
branch
from
9ee57e2
to
26f4ebe
Compare
|
(updated description so that it no longer asks if this requires an RFC, and instead asserts that it does not.) |
|
I'd like to get this landed (but feel free to reassign.) The main thing I'm currently not satisfied with is the way I'm emulating the formula:
I'm fairly sure there's gotta be a better bit-trickery way to do this than my emitting the expression:
but I was on a plane when I authored this and thus did not attempt to search the internet for appropriate bit tricks. |
|
@pnkfelix I believe the "standard" formula is |
|
@eddyb ah yes that looks familiar, thanks! (writing the next commit that switches to that now...) |
Hat-tip to eddyb for the appropriate bit-trickery here.
brson
added
relnotes
|
@bors r+ |
|
📌 Commit 21be094 has been approved by |
|
⌛ Testing commit 21be094 with merge efdbc0e... |
…omatsakis Change the behavior of the glue code emitted for `size_and_align_of_dst`. This thus changes the behavior of `std::mem::size_of_val` and `std::mem::align_of_val`. It tries to move us towards a world where the following property holds: Given type `T` implements `Trait` and a value `b: Box<T>`, where `std::mem::size_of::<T>()` returns `k`, then: * `std::mem::size_of_val(b)` returns `k` * `std::mem::size_of_val(b as Box<Trait>)` returns `k` Note that one might legitimately question whether the above property *should* hold. The property certainly does not hold today, as illustrated by #27023. (A follow-up task is to make various tests that check that the above property holds for a wide variety of types ... I chose not to invest effort in writing such a test before we actually determine that the above property is desirable.) nmatsakis and pnkfelix agree that this PR does not require an RFC. cc @rust-lang/lang (since others may disagree). (It also *might* break code, though it is hard for me to imagine that it could break code that wasn't already going to assert-fail when run in e.g. debug builds...) Fix issue #27023 Also, this (or something like it) is a prerequisite for *fixing`make check` on `--enable-optimize --enable-debug` builds*
Change the behavior of the glue code emitted for
size_and_align_of_dst.This thus changes the behavior of
std::mem::size_of_valandstd::mem::align_of_val. It tries to move us towards a world where the following property holds:Given type
TimplementsTraitand a valueb: Box<T>, wherestd::mem::size_of::<T>()returnsk, then:std::mem::size_of_val(b)returnskstd::mem::size_of_val(b as Box<Trait>)returnskNote that one might legitimately question whether the above property should hold. The property certainly does not hold today, as illustrated by #27023.
(A follow-up task is to make various tests that check that the above property holds for a wide variety of types ... I chose not to invest effort in writing such a test before we actually determine that the above property is desirable.)
nmatsakis and pnkfelix agree that this PR does not require an RFC. cc @rust-lang/lang (since others may disagree).
(It also might break code, though it is hard for me to imagine that it could break code that wasn't already going to assert-fail when run in e.g. debug builds...)
Fix issue #27023
Also, this (or something like it) is a prerequisite for fixing
make checkon--enable-optimize --enable-debugbuilds