rustc_codegen_llvm: don't assume offsets are always aligned. #53998

eddyb · 2018-09-06T15:48:09Z

Fixes #53728 by taking into account not just overall type alignment and the field's alignment when determining whether a field is aligned or not ("packed"), but also the field's offset within the type.

Previously, rustc assumed that the offset was always at least as aligned as min(struct.align, field.align). However, there's no real reason to have that assumption, and it obviously can't always be true after we implement #[repr(align(N), pack(K))]. There's also a case today where that assumption is not true, involving niche discriminants in enums:

Suppose that we have the code in #53728:

#[repr(u16)]
enum DeviceKind {
    Nil = 0,
}

#[repr(packed)]
struct DeviceInfo {
    endianness: u8,
    device_kind: DeviceKind,
}

struct Wrapper {
    device_info: DeviceInfo,
    data: u32
}

Observe the layout of Option<Wrapper>. It has an alignment of 4 because of the u32. device_info.device_kind is a good niche field to use, which means the enum ends up with this layout:

size = 8
align = 4
fields = [
    { offset=1, type=u16 } // discriminant, .<Some>.device_info.device_kind
]

And here we have an discriminant with alignment 2 (u16) but offset 1.

rust-highfive · 2018-09-06T15:48:21Z

r? @oli-obk

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

oli-obk · 2018-09-06T15:56:49Z

src/librustc_codegen_llvm/mir/place.rs

@@ -174,7 +174,12 @@ impl PlaceRef<'ll, 'tcx> {
        let cx = bx.cx;
        let field = self.layout.field(cx, ix);
        let offset = self.layout.fields.offset(ix);
-        let align = self.align.min(self.layout.align).min(field.align);
+        let mut real_field_align = self.align.min(self.layout.align).min(field.align);
+        if offset.abi_align(real_field_align) > offset {


Since this code is already duplicated twice in rustc and will get a third duplication in miri, I think this warrants a method on Align taking a field alignment and a field offset and producing the real alignment.

That's not enough, sadly, unless you mean starting from self.align.min(self.layout.align) and then using the method on that. I think I like that, and I can fix the FIXME too.

Yes that's what I meant.

eddyb · 2018-09-06T17:57:09Z

@oli-obk Does this new approach work for you?

oli-obk · 2018-09-06T19:32:03Z

@bors r+ the math checks out, yes

bors · 2018-09-06T19:32:04Z

📌 Commit b9e7574 has been approved by oli-obk

arielb1 · 2018-09-08T10:10:41Z

src/librustc_codegen_llvm/mir/place.rs

@@ -174,7 +174,10 @@ impl PlaceRef<'ll, 'tcx> {
        let cx = bx.cx;
        let field = self.layout.field(cx, ix);
        let offset = self.layout.fields.offset(ix);
-        let align = self.align.min(self.layout.align).min(field.align);
+        let effective_field_align = self.align


Why isn't field.align set up properly here?

eddyb · 2018-09-08T13:11:46Z

src/librustc_codegen_llvm/mir/place.rs

-        let align = self.align.min(self.layout.align).min(field.align);
+        let effective_field_align = self.align
+            .min(self.layout.align)
+            .min(field.align)


It might help #54028 to remove this line, and rely strictly on the alignment of the type, combined with the best-case alignment for the offset. That way, we can hint LLVM about alignments that are higher than it can figure out by itself. Do we want to do that and do we expect a performance improvement?

rustc_codegen_llvm: don't assume offsets are always aligned. Fixes rust-lang#53728 by taking into account not just overall type alignment and the field's alignment when determining whether a field is aligned or not ("packed"), but also the field's offset within the type. Previously, rustc assumed that the offset was always at least as aligned as `min(struct.align, field.align)`. However, there's no real reason to have that assumption, and it obviously can't always be true after we implement `#[repr(align(N), pack(K))]`. There's also a case today where that assumption is not true, involving niche discriminants in enums: Suppose that we have the code in rust-lang#53728: ```Rust #[repr(u16)] enum DeviceKind { Nil = 0, } #[repr(packed)] struct DeviceInfo { endianness: u8, device_kind: DeviceKind, } struct Wrapper { device_info: DeviceInfo, data: u32 } ``` Observe the layout of `Option<Wrapper>`. It has an alignment of 4 because of the `u32`. `device_info.device_kind` is a good niche field to use, which means the enum ends up with this layout: ``` size = 8 align = 4 fields = [ { offset=1, type=u16 } // discriminant, .<Some>.device_info.device_kind ] ``` And here we have an discriminant with alignment 2 (`u16`) but offset 1.

bors · 2018-09-09T08:54:36Z

⌛ Testing commit b9e7574 with merge df6ba0c...

rustc_codegen_llvm: don't assume offsets are always aligned. Fixes #53728 by taking into account not just overall type alignment and the field's alignment when determining whether a field is aligned or not ("packed"), but also the field's offset within the type. Previously, rustc assumed that the offset was always at least as aligned as `min(struct.align, field.align)`. However, there's no real reason to have that assumption, and it obviously can't always be true after we implement `#[repr(align(N), pack(K))]`. There's also a case today where that assumption is not true, involving niche discriminants in enums: Suppose that we have the code in #53728: ```Rust #[repr(u16)] enum DeviceKind { Nil = 0, } #[repr(packed)] struct DeviceInfo { endianness: u8, device_kind: DeviceKind, } struct Wrapper { device_info: DeviceInfo, data: u32 } ``` Observe the layout of `Option<Wrapper>`. It has an alignment of 4 because of the `u32`. `device_info.device_kind` is a good niche field to use, which means the enum ends up with this layout: ``` size = 8 align = 4 fields = [ { offset=1, type=u16 } // discriminant, .<Some>.device_info.device_kind ] ``` And here we have an discriminant with alignment 2 (`u16`) but offset 1.

bors · 2018-09-09T11:19:39Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing df6ba0c to master...

RalfJung · 2018-09-17T14:35:48Z

@eddyb so you mention on Discord that miri might also need a fix here. Do you have a testcase to check? I assume we'd currently throw an "unaligned access" error where we should not.

@eddyb

miri: correctly compute expected alignment for field This is the miri version of rust-lang#53998. A test is added by rust-lang/miri#457. r? @eddyb

add some interesting tests for alignment corner cases `strange_enum_discriminant_offset` example found in rust-lang/rust#53998.

add some interesting tests for alignment corner cases `strange_enum_discriminant_offset` example found in rust-lang#53998.

rust-highfive assigned oli-obk Sep 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2018

oli-obk reviewed Sep 6, 2018

View reviewed changes

rustc_codegen_llvm: don't assume offsets are always aligned.

b9e7574

eddyb force-pushed the issue-53728 branch from 7803f51 to b9e7574 Compare September 6, 2018 17:56

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2018

arielb1 reviewed Sep 8, 2018

View reviewed changes

eddyb commented Sep 8, 2018

View reviewed changes

eddyb mentioned this pull request Sep 8, 2018

Rust generates unaligned loads when unpacking an aligned struct #54028

Closed

Mark-Simulacrum mentioned this pull request Sep 8, 2018

Rollup of 11 pull requests #54068

Closed

bors merged commit b9e7574 into rust-lang:master Sep 9, 2018

bors mentioned this pull request Sep 9, 2018

rustc_target: separate out an individual alignment quantity type from Align. #54071

Merged

eddyb deleted the issue-53728 branch September 9, 2018 14:55

RalfJung mentioned this pull request Sep 17, 2018

miri: correctly compute expected alignment for field #54298

Merged

smmalis37 mentioned this pull request Apr 15, 2021

ICE: #[rustc_variance] on trait: asked to compute variance for wrong kind of item #79699

Closed

RalfJung mentioned this pull request Jul 31, 2023

add some interesting tests for alignment corner cases rust-lang/miri#3001

Merged

bors added a commit to rust-lang/miri that referenced this pull request Jul 31, 2023

Auto merge of #3001 - RalfJung:align, r=RalfJung

5bae909

add some interesting tests for alignment corner cases `strange_enum_discriminant_offset` example found in rust-lang/rust#53998.

bors added a commit to rust-lang/miri that referenced this pull request Jul 31, 2023

Auto merge of #3001 - RalfJung:align, r=RalfJung

51488ae

add some interesting tests for alignment corner cases `strange_enum_discriminant_offset` example found in rust-lang/rust#53998.

RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 6, 2023

Auto merge of rust-lang#3001 - RalfJung:align, r=RalfJung

10d77b9

add some interesting tests for alignment corner cases `strange_enum_discriminant_offset` example found in rust-lang#53998.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_codegen_llvm: don't assume offsets are always aligned. #53998

rustc_codegen_llvm: don't assume offsets are always aligned. #53998

eddyb commented Sep 6, 2018 •

edited by arielb1

Loading

rust-highfive commented Sep 6, 2018

oli-obk Sep 6, 2018

eddyb Sep 6, 2018

oli-obk Sep 6, 2018

eddyb commented Sep 6, 2018

oli-obk commented Sep 6, 2018

bors commented Sep 6, 2018

arielb1 Sep 8, 2018

arielb1 Sep 8, 2018

eddyb Sep 8, 2018

bors commented Sep 9, 2018

bors commented Sep 9, 2018

RalfJung commented Sep 17, 2018

rustc_codegen_llvm: don't assume offsets are always aligned. #53998

rustc_codegen_llvm: don't assume offsets are always aligned. #53998

Conversation

eddyb commented Sep 6, 2018 • edited by arielb1 Loading

rust-highfive commented Sep 6, 2018

oli-obk Sep 6, 2018

Choose a reason for hiding this comment

eddyb Sep 6, 2018

Choose a reason for hiding this comment

oli-obk Sep 6, 2018

Choose a reason for hiding this comment

eddyb commented Sep 6, 2018

oli-obk commented Sep 6, 2018

bors commented Sep 6, 2018

arielb1 Sep 8, 2018

Choose a reason for hiding this comment

arielb1 Sep 8, 2018

Choose a reason for hiding this comment

eddyb Sep 8, 2018

Choose a reason for hiding this comment

bors commented Sep 9, 2018

bors commented Sep 9, 2018

RalfJung commented Sep 17, 2018

eddyb commented Sep 6, 2018 •

edited by arielb1

Loading