Skip to content

Unpredictable behaviour regarding packed struct #15886

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

Closed
MichaelByrneAU opened this issue May 28, 2023 · 5 comments
Closed

Unpredictable behaviour regarding packed struct #15886

MichaelByrneAU opened this issue May 28, 2023 · 5 comments
Labels
bug Observed behavior contradicts documented or intended behavior

Comments

@MichaelByrneAU
Copy link
Contributor

MichaelByrneAU commented May 28, 2023

Zig Version

0.11.0-dev.3218+b873ce1e0

Steps to Reproduce and Observed Behavior

I have observed some strange behaviour regarding packed structs which disappear completely when the packed annotation is removed. Consider the following code, which I've attempted to make relatively minimal in order to demonstrate this behaviour:

const std = @import("std");

pub fn main() !void {
    const output_width:  u32 = 1_280;
    const output_height: u32 = 720;
    _ = BitmapHeader.new(output_width, output_height);
}

// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
// Bitmap
// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

const BitmapHeader = packed struct {
    const Self = @This();

    file_header        : FileHeader,
    information_header : InformationHeader,

    // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

    pub fn new(width: u32, height: u32) Self {
        const output_pixels_size = width * height * 4;
        const signed_width:  i32 = @intCast(i32, width);
        const signed_height: i32 = @intCast(i32, height);
        
        std.debug.print("{} {}\n", .{signed_width, signed_height});
        
        const bitmap_header = BitmapHeader{
            .file_header = FileHeader{
                .file_size = header_total_size + output_pixels_size,
            },
            .information_header = InformationHeader{
                .width  = signed_width, 
                .height = signed_height, 
            },
        };
        
        std.debug.print("{} {} {} {}\n", .{
            bitmap_header.file_header.file_size,
            bitmap_header.file_header.offset,
            bitmap_header.information_header.width, 
            bitmap_header.information_header.height
        });
        
        return bitmap_header;
    }

    // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

    const FileHeader = packed struct {
        file_type  : u16 = 0x4D42, 
        file_size  : u32, 
        reserved_1 : u16 = 0,
        reserved_2 : u16 = 0, 
        offset     : u32 = size(FileHeader) + size(InformationHeader),
    };

    const InformationHeader = packed struct {
        size              : u32 = size(@This()), 
        width             : i32, 
        height            : i32,
        planes            : u16 = 1, 
        bits_per_pixel    : u16 = 32,
        compression       : u32 = 0, 
        size_of_bitmap    : u32 = 0, 
        horz_resolution   : i32 = 0,
        vert_resolution   : i32 = 0, 
        colours_used      : u32 = 0,
        colours_important : u32 = 0,
    };

    /// Return size of T in bytes without padding.
    fn size(comptime T: type) comptime_int {
        const size_in_bits = @bitSizeOf(T);
        if ((size_in_bits % 8) != 0) @compileError("Type size must be expressable in bytes");
        return @bitSizeOf(T) / 8;
    }

    /// Total size of the header structure.
    const header_total_size = size(FileHeader) + size(InformationHeader);
};

When creating the BitmapHeader packed struct through the new method as above, field values will turn into gibberish. For example, I have run the above code on aarch64 macOS and:

  • The file_header.file_size and file_header.offsert fields are seemingly random values, like some bits in memory have been misinterpreted as their actual values.
  • The information_header.width and information_header.height fields are 0, not 1280 and 720 respectively as expected.

These conditions are pretty fragile. If I directly place the literals 1280 and 720 into the width and height fields, the example works correctly. If I lift out all of the logic from the new method into the main function, the example works correctly. If I remove an arbitrary number of fields from the InformationHeader packed struct (such as the last two, as was the case when I was trying to minimise this example), funnily enough the example works correctly. If the packed struct annotation is removed from all structs in the example, it works correctly.

Apologies if I'm butting against a part of the language I just don't understand, perhaps there are footguns with the packed struct that I need to learn. However, this seemed unintuitive to the point where I felt it was a bug.

Expected Behavior

That packed structs behave the same way as normal structs, at least in the contexts described above.

The above happens in debug, release fast and release safe modes.

@MichaelByrneAU MichaelByrneAU added the bug Observed behavior contradicts documented or intended behavior label May 28, 2023
@nektro
Copy link
Contributor

nektro commented May 28, 2023

packed structs have the same ABI as ints of the same size in this case a u432. if you're not using exotic sizes you're almost certainly looking for extern struct instead.

https://ziglang.org/documentation/master/#packed-struct

@MichaelByrneAU
Copy link
Contributor Author

MichaelByrneAU commented May 28, 2023

Should this behaviour happen regardless of whether the struct is annotated as extern or packed? You are correct, I did end up resolving this issue through use of extern instead, but it still seemed like an odd corner to find myself in.

Further the language reference does state under extern that "This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct or normal struct." This initially pushed me towards using packed, but perhaps this was not the way to go.

@nektro
Copy link
Contributor

nektro commented May 28, 2023

the bug is still likely valid yeah. it was likely extended to be a u512 and found some miscompilation around the field access since such large ints arent as well tested

@MichaelByrneAU
Copy link
Contributor Author

My guess as well, I was perhaps a bit ambitious with my mega integer haha

@Vexu
Copy link
Member

Vexu commented May 28, 2023

Duplicate of #13480

@Vexu Vexu marked this as a duplicate of #13480 May 28, 2023
@Vexu Vexu closed this as completed May 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

No branches or pull requests

3 participants