Implementation of #[repr(packed(n))] RFC 1399.
#48528
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @eddyb |
|
|
|
You found the one in print-size-types :) Do you think I should add more though? It's not very thoroughly tested. |
|
It's always better to err on the side of too many tests. |
|
@bitshifter I think it would be a good idea for the tests to do their best to try to break the FWIW I'd like |
Currently |
|
You can put a |
src/librustc/ty/layout.rs
Outdated
| @@ -974,7 +979,8 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> { | |||
| let mut inverse_memory_index: Vec<u32> = (0..fields.len() as u32).collect(); | |||
|
|
|||
| // Anything with repr(C) or repr(packed) doesn't optimize. | |||
| let mut optimize = (repr.flags & ReprFlags::IS_UNOPTIMISABLE).is_empty(); | |||
| let mut optimize = (repr.flags & ReprFlags::IS_UNOPTIMISABLE).is_empty() | |||
There was a problem hiding this comment.
Now that this is a bit more complex, I think I'll pull this out to a method on ReprOptions, something like inhibit_field_reordering_opt.
bitshifter
force-pushed
the
repr_packed
branch
3 times, most recently
from
bf94f97 to
c5af68a
Compare
| // rustc::ty::layout::Align restricts align to <= 2147483647 | ||
| if align <= 2147483647 { | ||
| acc.push(ReprAlign(align as u32)); | ||
| if *literal <= 2147483647 { |
There was a problem hiding this comment.
According to LLVM docs the maximum alignment on alloca, load, store etc is 1 << 29 (e.g. https://llvm.org/docs/LangRef.html#id185). Perhaps we should enforce that restriction here in case anyone tries to use alignments that LLVM doesn't support.
There was a problem hiding this comment.
@eddyb do think it would be a good idea to change this max value to 1 << 29? I'll do it as a separate PR if so - don't want to jeopardise my position in the homu queue!
|
@gnzlbg I've added a bunch more tests combining repr C and packed. Let me know if there's anything specific you think needs better test coverage. |
|
☔ The latest upstream changes (presumably #48653) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
src/libsyntax/attr.rs
Outdated
| let mut align_error = None; | ||
| if let ast::LitKind::Int(align, ast::LitIntType::Unsuffixed) = value.node { | ||
| if align.is_power_of_two() { | ||
| let parse_int_literal = |node: &ast::LitKind| -> Result<u32, &'static str> { |
There was a problem hiding this comment.
This should be parse_alignment or similar.
|
@bitshifter thanks! the new tests cover the cases that are relevant for libc and mach :) |
|
☔ The latest upstream changes (presumably #48860) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #49051) made this pull request unmergeable. Please resolve the merge conflicts. |
kennytm
added
S-waiting-on-author
|
I couldn't repro it building my branch via msys2 and msvc, will try again after merging latest master. |
|
Still didn't get this failure after merging latest master. I have no idea why this test has failed. It appears to have just exited with a non-zero code (due to a panic) but nothing in stdout or stderr. I'll follow up on IRC when I have time, probably over the long weekend here. |
|
Ping from triage, @bitshifter ! Will you have time to work on this soon? |
|
Current status is I'm stuck on this. I haven't had the time and energy to proactively ask for help on IRC and a lot of people were at the all hands or in transit last weekend anyway. I have not been able to reproduce this test locally after rebuilding about half a dozen times. I am also unable to see any reason why my changes would break mir-opt. My gut feeling is this is a spurious test fail unrelated to this PR and I'd like to see if I second run through Homu gets the same failure. In the process of chasing this I've been failing to get my PR or even master to complete a full test pass under msys2/msvc. I had a few fails due to path length and after moving my check out even master always fails on run-make-fulldeps/target-specs. So perhaps there is some difference in my msys2 environment and what is being used on homu to cause this and the other fail that I can't reproduce? My msys2 is basically up to date. My MSVC is the latest 2017, running Windows 10 home edition. Anyway I still intend to chase this up on IRC, hopefully sometime over the next week. If someone wanted to try running it through homu again that would be great. |
|
@bitshifter which arch is exactly failing here? Maybe we could involve someone interested / experienced with this architecture to help fix this one last issue? cc @alexcrichton ? |
|
@gnzlbg the failing arch is x86_64-pc-windows-msvc. I'm going to download the mingw that's being used by the rust appveyor.yml and see if that makes a difference. edit: it's using mingw-w64 instead of msys2 so perhaps that will make a difference. |
|
I checked out this branch locally and it passed on x86_64-pc-windows-msvc (the mir-opt tests), so I'm not sure :( |
|
I tried building in a mingw-w64 environment like what is being used on appveyor but the test still passed without issue, so still no idea what's going on here. |
|
@eddyb could you +r this again please to see if it will pass the mir-opt/issue-38669.rs test. |
|
@bors r+ |
|
📌 Commit 15d1c4d has been approved by |
bors
added
S-waiting-on-bors
Implementation of `#[repr(packed(n))]` RFC 1399. Tracking issue #33158.
|
☀️ Test successful - status-appveyor, status-travis |
don't inhibit random field reordering on repr(packed(1)) `inhibit_struct_field_reordering_opt` being false means we exclude this type from random field shuffling. However, `packed(1)` types can still be shuffled! The logic was added in rust-lang#48528 since it's pointless to reorder fields in packed(1) types (there's no padding that could be saved) -- but that shouldn't inhibit `-Zrandomize-layout` (which did not exist at the time). We could add an optimization elsewhere to not bother sorting the fields for `repr(packed)` types, but I don't think that's worth the effort. This *does* change the behavior in that we may now reorder fields of `packed(1)` structs (e.g. if there are niches, we'll try to move them to the start/end, according to `NicheBias`). We were always allowed to do that but so far we didn't. Quoting the [reference](https://doc.rust-lang.org/reference/type-layout.html): > On their own, align and packed do not provide guarantees about the order of fields in the layout of a struct or the layout of an enum variant, although they may be combined with representations (such as C) which do provide such guarantees.
don't inhibit random field reordering on repr(packed(1)) `inhibit_struct_field_reordering_opt` being false means we exclude this type from random field shuffling. However, `packed(1)` types can still be shuffled! The logic was added in rust-lang/rust#48528 since it's pointless to reorder fields in packed(1) types (there's no padding that could be saved) -- but that shouldn't inhibit `-Zrandomize-layout` (which did not exist at the time). We could add an optimization elsewhere to not bother sorting the fields for `repr(packed)` types, but I don't think that's worth the effort. This *does* change the behavior in that we may now reorder fields of `packed(1)` structs (e.g. if there are niches, we'll try to move them to the start/end, according to `NicheBias`). We were always allowed to do that but so far we didn't. Quoting the [reference](https://doc.rust-lang.org/reference/type-layout.html): > On their own, align and packed do not provide guarantees about the order of fields in the layout of a struct or the layout of an enum variant, although they may be combined with representations (such as C) which do provide such guarantees.
don't inhibit random field reordering on repr(packed(1)) `inhibit_struct_field_reordering_opt` being false means we exclude this type from random field shuffling. However, `packed(1)` types can still be shuffled! The logic was added in rust-lang/rust#48528 since it's pointless to reorder fields in packed(1) types (there's no padding that could be saved) -- but that shouldn't inhibit `-Zrandomize-layout` (which did not exist at the time). We could add an optimization elsewhere to not bother sorting the fields for `repr(packed)` types, but I don't think that's worth the effort. This *does* change the behavior in that we may now reorder fields of `packed(1)` structs (e.g. if there are niches, we'll try to move them to the start/end, according to `NicheBias`). We were always allowed to do that but so far we didn't. Quoting the [reference](https://doc.rust-lang.org/reference/type-layout.html): > On their own, align and packed do not provide guarantees about the order of fields in the layout of a struct or the layout of an enum variant, although they may be combined with representations (such as C) which do provide such guarantees.
don't inhibit random field reordering on repr(packed(1)) `inhibit_struct_field_reordering_opt` being false means we exclude this type from random field shuffling. However, `packed(1)` types can still be shuffled! The logic was added in rust-lang/rust#48528 since it's pointless to reorder fields in packed(1) types (there's no padding that could be saved) -- but that shouldn't inhibit `-Zrandomize-layout` (which did not exist at the time). We could add an optimization elsewhere to not bother sorting the fields for `repr(packed)` types, but I don't think that's worth the effort. This *does* change the behavior in that we may now reorder fields of `packed(1)` structs (e.g. if there are niches, we'll try to move them to the start/end, according to `NicheBias`). We were always allowed to do that but so far we didn't. Quoting the [reference](https://doc.rust-lang.org/reference/type-layout.html): > On their own, align and packed do not provide guarantees about the order of fields in the layout of a struct or the layout of an enum variant, although they may be combined with representations (such as C) which do provide such guarantees.
don't inhibit random field reordering on repr(packed(1)) `inhibit_struct_field_reordering_opt` being false means we exclude this type from random field shuffling. However, `packed(1)` types can still be shuffled! The logic was added in rust-lang/rust#48528 since it's pointless to reorder fields in packed(1) types (there's no padding that could be saved) -- but that shouldn't inhibit `-Zrandomize-layout` (which did not exist at the time). We could add an optimization elsewhere to not bother sorting the fields for `repr(packed)` types, but I don't think that's worth the effort. This *does* change the behavior in that we may now reorder fields of `packed(1)` structs (e.g. if there are niches, we'll try to move them to the start/end, according to `NicheBias`). We were always allowed to do that but so far we didn't. Quoting the [reference](https://doc.rust-lang.org/reference/type-layout.html): > On their own, align and packed do not provide guarantees about the order of fields in the layout of a struct or the layout of an enum variant, although they may be combined with representations (such as C) which do provide such guarantees.
Tracking issue #33158.