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

Make `align_of` behave like `min_align_of`. #25646

Merged
merged 1 commit into from Jun 26, 2015
Merged

Make `align_of` behave like `min_align_of`. #25646

merged 1 commit into from Jun 26, 2015

Conversation

@huonw
Copy link
Member

huonw commented May 20, 2015

This removes a footgun, since it is a reasonable assumption to make that
pointers to T will be aligned to align_of::<T>(). This also matches
the behaviour of C/C++. min_align_of is now deprecated.

Closes #21611.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 20, 2015

r? @brson

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

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 21, 2015

These functions have changed so many times. How can we know this time is right? It even seems to conflict with the deleted comment 'We use the preferred alignment as the default alignment for a type. This appears to be what clang migrated towards as well'.

This is a stable breaking change and should be marked as such.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 22, 2015

cc @aturon re breakage

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 26, 2015

I think that the "breakage" here in this sense is fine (through deprecation), but I agree with @brson that this may want some more research into what clang is using and why (e.g. do we still match them?)

@alexcrichton alexcrichton added the T-libs label May 26, 2015
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 27, 2015

☔️ The latest upstream changes (presumably #25790) made this pull request unmergeable. Please resolve the merge conflicts.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jun 1, 2015

It's not breaking because of the deprecation but because it changes the values reported by the non-deprecated stable align_of function.

@brson brson added the I-nominated label Jun 1, 2015
@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jun 1, 2015

Nominating this because of the breakiness. If this is important we should do it fast.

@huonw

This comment has been minimized.

Copy link
Member Author

huonw commented Jun 2, 2015

I have a strong suspicion this won't cause any significant actual breakage. In fact, I suspect this change will unbreak more code than it breaks (code that was assuming that T pointers are guaranteed to have align_of::<T>() alignment, which is far more true after this patch).

I looked through all the search results for "mem align_of" and didn't see anything where this would cause breakage. Essentially all uses of align_of::<T>() are fed into allocate/reallocate/deallocate calls, and these are all fine as long as (a) the alignment is valid for type T (which is true), and (b) each of those functions gets called with the same alignment value (also true).

(I did notice some places in the search results which fall prey to making invalid assumptions about align_of... including in my own code. whoops.)


Unfortunately, I just realised this change still doesn't guarantee that every &T pointer will be aligned to align_of::<T>(), due to packed fields, e.g. this prints 1, but "should" print 0:

use std::mem;

#[repr(packed)]
struct Foo {
    _x: u8,
    y: u32,
}
fn main() {
    let x = Foo { _x: 0, y: 0 };

    println!("{}", &x.y as *const _ as usize % mem::align_of::<u32>())
}

However, I think this is still an improvement: the example in #21611 seems like a footgun (there's nothing special about the struct in that example).

We use the preferred alignment as the default alignment for a type. This appears to be what clang migrated towards as well

It seems we're currently using a different definition of preferred, e.g. clang's behaviour is what I'd expect Rust to do.

@Gankra

This comment has been minimized.

Copy link
Contributor

Gankra commented Jun 3, 2015

I'm in favour of this change; basically no one on on stable could have been using this (it's only really useful for the heap::allocate API AFAICT). Everyone on unstable was wrong if they were using align_of. If they were correctly using min_align_of then they get a correct error.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jun 4, 2015

I, too, would be in favor of this to bring us in line with clang. I share @brson's concerns in that very low-level code like this often has very large ramifications later on down the road, but I'm fairly confident that those kinds of libraries have yet to be written (or are still underway), so now's definitely the time for a change like this.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jun 10, 2015

This PR is now entering its week-long final comment period.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jun 11, 2015

I feel very strongly that anything called "align-of" (with no qualifier) ought to be doing the same thing that you would get with C/C++'s alignof operator. I don't have the C or C++ spec handy, but some quick web searching suggests to me that "min align of" is indeed that value. (For all I know, the precise semantics of the alignof operator are only loosely defined in the spec -- but in either case being compatible with gcc/clang is very likely to meet expectations.)

Therefore: 👍

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jun 16, 2015

The consensus of the library subteam was to merge this, and @huonw has taken a blood oath that this is the correct implementation, assuaging @brson's fears.

r=me with a rebase, thanks @huonw!

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jun 24, 2015

Nominating for backport to 1.2. Needs to be added to relnotes as a change in behavior.

This removes a footgun, since it is a reasonable assumption to make that
pointers to `T` will be aligned to `align_of::<T>()`. This also matches
the behaviour of C/C++. `min_align_of` is now deprecated.

Closes #21611.
@huonw huonw force-pushed the huonw:align branch from c168494 to 225b116 Jun 26, 2015
@huonw

This comment has been minimized.

Copy link
Member Author

huonw commented Jun 26, 2015

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 26, 2015

📌 Commit 225b116 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 26, 2015

⌛️ Testing commit 225b116 with merge 378a370...

bors added a commit that referenced this pull request Jun 26, 2015
This removes a footgun, since it is a reasonable assumption to make that
pointers to `T` will be aligned to `align_of::<T>()`. This also matches
the behaviour of C/C++. `min_align_of` is now deprecated.

Closes #21611.
@bors bors merged commit 225b116 into rust-lang:master Jun 26, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jun 30, 2015

triage: beta-accepted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.