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

Packed structs don't work correctly when using inline @ptrCasts #14261

Closed
Dudejoe870 opened this issue Jan 10, 2023 · 5 comments
Closed

Packed structs don't work correctly when using inline @ptrCasts #14261

Dudejoe870 opened this issue Jan 10, 2023 · 5 comments
Labels
backend-llvm The LLVM backend outputs an LLVM IR Module. bug Observed behavior contradicts documented or intended behavior
Milestone

Comments

@Dudejoe870
Copy link

Zig Version

0.11.0-dev.1253+fcee1bf99

Steps to Reproduce and Observed Behavior

Download this code, compile it using zig build-exe issue.zig and run it.
You will observe this output:

info:
0: 0
1: 300000

Which is incorrect and by looking at the code, it's not obvious why.

Expected Behavior

The correct output would be this:

info:
0: 30000000
1: 0

which you can get by removing "inline" from the function definition.

@Dudejoe870 Dudejoe870 added the bug Observed behavior contradicts documented or intended behavior label Jan 10, 2023
@AssortedFantasy
Copy link

Why make a small zip file? Just comment the code inline.

const std = @import("std");

var arr = [_]u32{0} ** 2;

const PackedStructBitField = packed struct(u32) {
    test_bit: bool,
    someother_data: u12,
    other_test_bit: bool,
    someother_more_different_data: u12,
    other_bits: packed struct(u6) {
        enable_1: bool,
        enable_2: bool,
        enable_3: bool,
        enable_4: bool,
        enable_5: bool,
        enable_6: bool
    }
};

// Remove "inline" to get the correct result.
pub inline fn getPackedStructFlags() *PackedStructBitField {
    return @ptrCast(*PackedStructBitField, &arr[0]);
}

pub fn main() void {
    getPackedStructFlags().other_bits.enable_3 = true;
    getPackedStructFlags().other_bits.enable_4 = true;
    std.log.info("\n0: {x}\n1: {x}", .{ arr[0], arr[1] });
}

So you seem to be setting bits 28 and 29. Assuming packed structs work little endian bit style like I assume they do. However 2^28+2^29 is 805306368 which isn't even what you say is the correct output.

Plus 30000000 in hex is 0x01_C9_C3_80 and that can't possibly be correct if you are only setting two bits.

@Dudejoe870
Copy link
Author

The output is already in hex. The zip file is just there cause that's how I ended up doing it, you are correct that I could've made it inline, I just didn't.

@Dudejoe870
Copy link
Author

To me personally, it seems when the code is inline, the compiler is somehow calculating the memory offset incorrectly.

@AssortedFantasy
Copy link

AssortedFantasy commented Jan 10, 2023

I somehow missed the hex part. I plugged it into compiler explorer and indeed the result is all wrong. https://godbolt.org/z/rvevG5exs

Here is the assembly it generates for main.

example.main: ; broken code
        push    rbp
        mov     rbp, rsp
        sub     rsp, 16
        mov     eax, dword ptr [example.arr+3] ; Should not be doing +3
        and     eax, -268435457
        or      eax, 268435456
        mov     dword ptr [example.arr+3], eax
        mov     eax, dword ptr [example.arr+3]
        and     eax, -536870913
        or      eax, 536870912
        mov     dword ptr [example.arr+3], eax
        mov     ecx, dword ptr [example.arr]
        mov     eax, dword ptr [example.arr+4]
        mov     dword ptr [rbp - 8], ecx
        mov     dword ptr [rbp - 4], eax
        lea     rdi, [rbp - 8]
        call    "log.scoped(.default).info__anon_3466"
        add     rsp, 16
        pop     rbp
        ret

When you remove the inline the assembly changes to this.

example.main:
        push    rbp
        mov     rbp, rsp
        sub     rsp, 32
        call    example.getPackedStructFlags
        mov     qword ptr [rbp - 24], rax
        mov     rax, qword ptr [rbp - 24]
        mov     ecx, dword ptr [rax]   ; in broken code this is rax+3
        and     ecx, -268435457
        or      ecx, 268435456
        mov     dword ptr [rax], ecx
        call    example.getPackedStructFlags
        mov     qword ptr [rbp - 16], rax
        mov     rax, qword ptr [rbp - 16]
        mov     ecx, dword ptr [rax] ; in broken code this is rax+3.
        and     ecx, -536870913
        or      ecx, 536870912
        mov     dword ptr [rax], ecx
        mov     ecx, dword ptr [example.arr]
        mov     eax, dword ptr [example.arr+4]
        mov     dword ptr [rbp - 8], ecx
        mov     dword ptr [rbp - 4], eax
        lea     rdi, [rbp - 8]
        call    "log.scoped(.default).info__anon_3464"
        add     rsp, 32
        pop     rbp
        ret

Weirdly enough what it is doing is that it is doing a misaligned load from byte 3 when you inline.

@Dudejoe870
Copy link
Author

Personally what it looks like to me then, that the compiler should be either
a. Not adding an offset at all
or b. doing a byte "and" and "or" at byte 3 (assuming little endian, this would indeed be the correct byte) with the value 0x30
It seems to me a logic error in the compiler has ended up with a weird amalgamation between these two approaches when inlining the code.

@Vexu Vexu added this to the 0.11.0 milestone Jan 13, 2023
@Vexu Vexu added the backend-llvm The LLVM backend outputs an LLVM IR Module. label Jan 13, 2023
Vexu added a commit to Vexu/zig that referenced this issue Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend-llvm The LLVM backend outputs an LLVM IR Module. bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

No branches or pull requests

3 participants