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

Incorrectly generated code for struct alignments #23431

Closed
AerialX opened this issue Mar 17, 2015 · 1 comment · Fixed by #24472
Closed

Incorrectly generated code for struct alignments #23431

AerialX opened this issue Mar 17, 2015 · 1 comment · Fixed by #24472
Assignees
Labels
A-codegen Area: Code generation

Comments

@AerialX
Copy link

AerialX commented Mar 17, 2015

The attached source produces code that misbehaves on some platforms that are picky about memory access alignment. The code should exit with code 2, but on both Emscripten-generated JavaScript and ARMv5, it finishes with exit code 1 to indicate that the en field is incorrectly tested as equal to Enum::A. Changing the val field to a u32 type results in proper execution on these platforms. All optimization levels seem to cause it.

Libraries that make use of structs with u8 fields followed by enums seem particularly prone to this problem. It all works fine on platforms that allow unaligned memory access like x86, ARMv6, etc.

Expected output: (x86, ARMv6)

test: Test { val: 1, en: B }
testvalue: B

Actual output: (ARMv5, Emscripten)

test: Test { val: 0, en: A }
testvalue: A

Note that the same ARM binary runs on both ARMv5 and ARMv6 platforms, producing the different results.

See also: emscripten-core/emscripten#3261

Generated LLVM IR (println!() and derive(Debug) removed)

#![feature(start)]

#[inline(never)]
fn testvalue(e: Enum) -> isize {
    println!("testvalue: {:?}", e);
    let mut ch = 0;
    match &e {
        &Enum::A => ch = 1,
        &Enum::B => ch = 2,
        &Enum::C(_) => (),
    };
    ch
}

#[inline(never)]
fn clonetest(k: Test) -> Test {
    k.clone()
}

#[inline(never)]
#[start]
fn main(_: isize, _: *const *const u8) -> isize {
    let test = Test {
        val: 1,
        en: Enum::B,
    };

    let test = clonetest(test.clone());
    println!("test: {:?}", test);
    testvalue(test.en)
}

#[derive(Debug, Copy, Clone)]
pub enum Enum {
    A,
    B,
    C(u16)
}

#[derive(Debug, Copy, Clone)]
pub struct Test {
    pub val: u8,
    pub en: Enum,
}
@sfackler sfackler added A-codegen Area: Code generation I-wrong labels Mar 17, 2015
@dotdash
Copy link
Contributor

dotdash commented Apr 14, 2015

To quote myself:

22:42:34 <doener_> Aaron: ah, the load from test.en is indeed qualified with
                   the wrong alignment
22:44:11 <doener_> Aaron: the Test struct has an aligment on 8, and the enum
                   has an offset of 2 from that. So it's aligned on 2. The load
                   from the i32* was inferred to load from an address that has
                   an alignment of 4, because rustc doesn't specify an
                   alignment for loads/store at all, llvm used the "natural"
                   alignment for that type

@dotdash dotdash self-assigned this Apr 15, 2015
dotdash added a commit to dotdash/rust that referenced this issue Apr 15, 2015
Loading from and storing to small aggregates happens by casting the
aggregate pointer to an appropriately sized integer pointer to avoid
the usage of first class aggregates which would lead to less optimized
code.

But this means that, for example, a tuple of type (i16, i16) will be
loading through an i32 pointer and because we currently don't provide
alignment information LLVM assumes that the load should use the ABI
alignment for i32 which would usually be 4 byte alignment. But the
alignment requirement for the (i16, i16) tuple will usually be just 2
bytes, so we're overestimating alignment, which invokes undefined
behaviour.

Therefore we must emit appropriate alignment information for
stores/loads through such casted pointers.

Fixes rust-lang#23431
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 17, 2015
 Loading from and storing to small aggregates happens by casting the
aggregate pointer to an appropriately sized integer pointer to avoid
the usage of first class aggregates which would lead to less optimized
code.

But this means that, for example, a tuple of type (i16, i16) will be
loading through an i32 pointer and because we currently don't provide
alignment information LLVM assumes that the load should use the ABI
alignment for i32 which would usually be 4 byte alignment. But the
alignment requirement for the (i16, i16) tuple will usually be just 2
bytes, so we're overestimating alignment, which invokes undefined
behaviour.

Therefore we must emit appropriate alignment information for
stores/loads through such casted pointers.

Fixes rust-lang#23431
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 17, 2015
 Loading from and storing to small aggregates happens by casting the
aggregate pointer to an appropriately sized integer pointer to avoid
the usage of first class aggregates which would lead to less optimized
code.

But this means that, for example, a tuple of type (i16, i16) will be
loading through an i32 pointer and because we currently don't provide
alignment information LLVM assumes that the load should use the ABI
alignment for i32 which would usually be 4 byte alignment. But the
alignment requirement for the (i16, i16) tuple will usually be just 2
bytes, so we're overestimating alignment, which invokes undefined
behaviour.

Therefore we must emit appropriate alignment information for
stores/loads through such casted pointers.

Fixes rust-lang#23431
dotdash added a commit to dotdash/rust that referenced this issue Apr 18, 2015
Loading from and storing to small aggregates happens by casting the
aggregate pointer to an appropriately sized integer pointer to avoid
the usage of first class aggregates which would lead to less optimized
code.

But this means that, for example, a tuple of type (i16, i16) will be
loading through an i32 pointer and because we currently don't provide
alignment information LLVM assumes that the load should use the ABI
alignment for i32 which would usually be 4 byte alignment. But the
alignment requirement for the (i16, i16) tuple will usually be just 2
bytes, so we're overestimating alignment, which invokes undefined
behaviour.

Therefore we must emit appropriate alignment information for
stores/loads through such casted pointers.

Fixes rust-lang#23431
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 18, 2015
Loading from and storing to small aggregates happens by casting the
aggregate pointer to an appropriately sized integer pointer to avoid
the usage of first class aggregates which would lead to less optimized
code.

But this means that, for example, a tuple of type (i16, i16) will be
loading through an i32 pointer and because we currently don't provide
alignment information LLVM assumes that the load should use the ABI
alignment for i32 which would usually be 4 byte alignment. But the
alignment requirement for the (i16, i16) tuple will usually be just 2
bytes, so we're overestimating alignment, which invokes undefined
behaviour.

Therefore we must emit appropriate alignment information for
stores/loads through such casted pointers.

Fixes rust-lang#23431
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants