Disallow reader.readStruct for packed structs

[zxubian]

Zig Version

0.9.1 (windows, chocolatey),0.10.0-dev.4166+cae76d829

Steps to Reproduce

1. create file repro.zig

/// repro.zig
const std = @import("std");
const expect = std.testing.expect;

const PackedStruct = packed struct {
    a: u48,
    b: u48,
    c: u16,
};

test "reading a packed struct" {
    const file = try std.fs.cwd().openFile("repro.zig", .{});
    const reader = file.reader();
    _ = try reader.readStruct(PackedStruct);
    const pos_from_reading_struct = try reader.context.getPos();
    try reader.context.seekTo(0);
    _ = try reader.readBytesNoEof(@bitSizeOf(PackedStruct) / 8);
    const pos_from_reading_bytes = try reader.context.getPos();
    std.log.warn("{}, {}", .{ pos_from_reading_struct, pos_from_reading_bytes });
    try expect(pos_from_reading_struct == pos_from_reading_bytes);
}

2. zig test repro.zig

Expected Behavior

Test passes.
Log output should be 14. 14

Actual Behavior

Test fails. Log output is 16, 14.
This is because @sizeOf(PackedStruct) is 16.
However, using sizeOf in readStruct is undesirable for reading consecutive packed structs, because now the seeker position is wrong, affecting future reads downstream. To correct this manually, the developer has to check if packed struct requires padding, and manually wind the seeker back using seekBy.

[zxubian]

Possibly related:
#12948
#10958

[layneson]

Just ran into this - seems like a significant footgun.

I'm not sure why a packed struct would need padding at the end if that struct has a bit length divisible by 8.

For now, I'm using the following function to get the desired behavior:

fn readPackedStruct(reader: anytype, comptime T: type) !T {
    comptime {
        if (@typeInfo(T).Struct.layout != .Packed) @compileError("readPackedStruct: " ++ @typeName(T) ++ " is not a packed struct!");
        if (@bitSizeOf(T) % 8 != 0) @compileError("readPackedStruct: " ++ @typeName(T) ++ " has a non-byte-aligned length!");
    }

    var buffer: [@bitSizeOf(T) / 8]u8 = undefined;
    try reader.readNoEof(&buffer);
    return @ptrCast(*align(1) T, &buffer).*;
}

[zxubian]

I'm using this as a workaround:

        const bytes = try reader.readBytesNoEof(@divExact(@bitSizeOf(T), 8));
        var result: T= undefined;
        @memcpy(std.mem.asBytes(&result), bytes[0..], @divExact(@bitSizeOf(T), 8));

[nektro]

imo Reader.readStruct shouldn't be in the stdlib. there's too many variables wrt to padding and endianness that it should be left to the user

[User65-87-11]

e a significant footgun.

I have the same issue

test "Hello @Sizeof" {
    const s1 = packed struct {
        a: u32,
        b: u16,
    };

    const s2 = packed struct {
        a: u8,
        b: u16,
    };
    print("sizeof : s1:{d}, s2:{d}\n", .{ @sizeOf(s1), @sizeOf(s2) });
    
       try std.testing.expect(@sizeOf(s1) == @sizeOf(u32) + @sizeOf(u16));
    try std.testing.expect(@sizeOf(s2) == @sizeOf(u8) + @sizeOf(u16));
}
// @sizeof... sizeof : s1:8, s2:4

Documentation says on packed structs "There is no padding between fields."

[Vexu]

Reader.readStruct has not been updated since before packed structs became integer backed, the assertion should be changed to explicitly check for .Extern layout.

[kj4tmp]

i recently hit this footgun pretty hard (ethernet headers), are we still interested in making this change? I am happy to submit a PR.

I assume this change would be considered "breaking"?