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

Constant packed struct instantiations can generate incorrect unaligned loads on armv4t/armv5 #2767

Closed
cartr opened this issue Jun 27, 2019 · 2 comments · Fixed by #5570
Closed
Labels
arch-arm 32-bit ARM bug Observed behavior contradicts documented or intended behavior stage1 The process of building from source via WebAssembly and the C backend.
Milestone

Comments

@cartr
Copy link
Contributor

cartr commented Jun 27, 2019

Prior to ARMv6, using LDR or LDRH with an unaligned address wouldn't load the data like you'd expect. (Depending on the situation, sometimes the result was "unpredictable", and sometimes it would mask off the lower bits of the address and use them to byte-rotate the result of the read.) But Zig can sometimes generate instructions that attempt to read unaligned constant addresses when compiling for ARMv4T and ARMv5. Here's a minimal example:

const Color = packed struct {
    r: u5,
};

const LcdControlValue = packed struct {
    mode: u3, _padding: u7 = 0, display_bg2: bool
};

export fn main() void {
    var color = Color {.r = 31 };
    var control = LcdControlValue{ .mode = 3, .display_bg2 = true };
}

According to https://godbolt.org/z/Jrmak7 , this generates the following assembly when compiled for armv5-freestanding in Debug mode:

Assembly
main:
        sub     sp, sp, #8
        ldr     r0, .LCPI1_0
        ldrb    r0, [r0]
        strb    r0, [sp, #4]
        ldr     r0, .LCPI1_1
        ldrh    r0, [r0]
        strh    r0, [sp]
        add     sp, sp, #8
        bx      lr
.LCPI1_0:
        .long   __unnamed_1
.LCPI1_1:
        .long   __unnamed_2

__unnamed_1:
        .byte   31

__unnamed_2:
        .short  1027

The ldrh instruction attempts to read a constant from __unnamed_2, which is not halfword-aligned, so incorrect data is loaded.

Although Zig's documentation does say that "[p]acked structs have 1-byte alignment", it seems kind of bad to generate code that silently does the wrong thing. It should maybe do something slower but safer, like using two LDRB instructions to read the data, when it isn't sure the pointer is aligned. Or maybe this would be solved by #1512?

@andrewrk andrewrk added arch-arm 32-bit ARM bug Observed behavior contradicts documented or intended behavior stage1 The process of building from source via WebAssembly and the C backend. labels Jun 27, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Jun 27, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Aug 30, 2019
@tucibi
Copy link

tucibi commented Feb 5, 2020

I'll get spun up on this. If users wants to concern themselves with explicitly aligning their packed structs per #1512 then they may, but I think it's a fair prerequisite for the compiler to handle this case correctly with LDRB by default.

@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Mar 12, 2020
@cartr
Copy link
Contributor Author

cartr commented Jun 8, 2020

(Now that sub-architectures have been removed, you have to compile with -target arm-freestanding -mcpu=arm7tdmi to reproduce the issue. Here's an updated Compiler Explorer link: https://godbolt.org/z/c8Ku6e)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-arm 32-bit ARM bug Observed behavior contradicts documented or intended behavior stage1 The process of building from source via WebAssembly and the C backend.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants