Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DST size_of_val and align_of_val computations #27351

Merged
merged 5 commits into from
Aug 5, 2015

Conversation

pnkfelix
Copy link
Member

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 fixingmake check on --enable-optimize --enable-debug builds

@rust-highfive
Copy link
Collaborator

r? @Aatch

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

@pnkfelix
Copy link
Member Author

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 --enable-optimize --enable-debug)

@dotdash
Copy link
Contributor

dotdash commented Jul 28, 2015

@pnkfelix Are you sure about that? I don't see how that would fix the debuginfo ODR violation from #26447.

@pnkfelix
Copy link
Member Author

@dotdash hmm, is #26447 linux-only? I have run make check successfully on my Mac... still, I can weaken my claim above.

@dotdash
Copy link
Contributor

dotdash commented Jul 28, 2015

@pnkfelix here's a testcase that should trigger the same problem:
https://gist.github.com/dotdash/1f45130c7580087dc56e

@pnkfelix
Copy link
Member Author

@dotdash also, this detail may or may not be relevant, but my claim was about --enable-optimize --enable-debug, which is not the same as --enable-debug as used in #26447...

…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.
@dotdash
Copy link
Contributor

dotdash commented Jul 28, 2015

@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 --enable-debug.

@pnkfelix
Copy link
Member Author

@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. :(

@nikomatsakis
Copy link
Contributor

I am of the opinion that no RFC is required for this, this is a bug fix.

@pnkfelix pnkfelix force-pushed the dst-size-and-align-issue-27023 branch from 9ee57e2 to 26f4ebe Compare July 29, 2015 17:44
@pnkfelix
Copy link
Member Author

(updated description so that it no longer asks if this requires an RFC, and instead asserts that it does not.)

@pnkfelix
Copy link
Member Author

I'd like to get this landed

r? @nikomatsakis

(but feel free to reassign.)


The main thing I'm currently not satisfied with is the way I'm emulating the formula:

size + ((size & (align-1)) ? align : 0)

I'm fairly sure there's gotta be a better bit-trickery way to do this than my emitting the expression:

size + ((size & (align-1)) * align-(size & (align-1)))

but I was on a plane when I authored this and thus did not attempt to search the internet for appropriate bit tricks.

@rust-highfive rust-highfive assigned nikomatsakis and unassigned Aatch Jul 29, 2015
@eddyb
Copy link
Member

eddyb commented Jul 29, 2015

@pnkfelix I believe the "standard" formula is (size + (align - 1)) & !align, assuming align is a power of 2.

@pnkfelix
Copy link
Member Author

@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 brson added relnotes Marks issues that should be documented in the release notes of the next release. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jul 29, 2015
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2015

📌 Commit 21be094 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 4, 2015

⌛ Testing commit 21be094 with merge efdbc0e...

bors added a commit that referenced this pull request Aug 4, 2015
…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*
@bors bors merged commit 21be094 into rust-lang:master Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants