access to packed struct fields returns incorrect value

[likern]

Zig Version

0.10.0-dev.358+36f13f591

Steps to Reproduce

1. Run zig test on this file https://gist.github.com/likern/760631d8c363c31330ef0076145baf87

const std = @import("std");
const Allocator = std.mem.Allocator;
const expect = std.testing.expect;

pub const DatabaseVersion = packed struct {
    major: u16,
    minor: u16,
    patch: u16,
};

pub const FileLayoutVersion = packed struct {
    major: u16,
    minor: u16,
    patch: u16,
};

pub const DatabaseSettings = packed struct {
    const Self = @This();

    name: [8]u8,
    version: DatabaseVersion,
    layout: FileLayoutVersion,
};

pub const database_settings = DatabaseSettings{
    .name = [8]u8{ 0x7a, 0x65, 0x6e, 0x64, 0x79, 0x20, 0x20, 0x20 },
    .version = .{ .major = 0, .minor = 0, .patch = 1 },
    .layout = .{ .major = 0, .minor = 0, .patch = 1 },
};

pub const MetaBlock = packed struct {
    const Self = @This();

    info: DatabaseSettings,

    pub fn createNoHash() Self {
        return Self{
            .info = database_settings,
        };
    }
};

test "create default metadata block" {
    try expect(database_settings.version.major == 0);

    var source_meta_block = MetaBlock.createNoHash();
    std.debug.print("major: {}\n", .{source_meta_block.info.version.major});
    try expect(source_meta_block.info.version.major == 0);
}

Expected Behavior

Test should pass and source_meta_block.info.version.major return 0

Actual Behavior

Returns 25978

[mateuszradomski]

For some reason the offset is not taken into account, the returned value 25978 is 0x657a, so the first two bytes of name in DatabaseSettings when using LE arch. I ran some tests and it seems that some places do understand that the offset should be 8-bytes, but others do not.

pub const database_settings = DatabaseSettings{
    .name = [8]u8{ 0x7a, 0x65, 0x6e, 0x64, 0x79, 0x20, 0x20, 0x20 },
    .version = .{ .major = 0xaaaa, .minor = 0xbbbb, .patch = 0xcccc },
};
// ...
var s = MetaBlock.createNoHash();
std.debug.print("offsetOf info:    {}\n", .{@offsetOf(@TypeOf(s), "info")});
std.debug.print("offsetOf version: {}\n", .{@offsetOf(@TypeOf(s.info), "version")});

const memory = @ptrCast([*]u8, &s.info)[0..@sizeOf(@TypeOf(s.info))];
std.debug.print("hex  [0x{}]\n", .{std.fmt.fmtSliceHexLower(memory)});
std.debug.print("ptr1 {p}\n", .{@ptrCast(*u8, &s.info)});
std.debug.print("ptr2 {p}\n", .{@ptrCast(*u8, &s.info.version)});

Gives:

offsetOf info:    0
offsetOf version: 8
hex  [0x7a656e6479202020aaaabbbbcccc0000]
ptr1 u8@7ffd202aa4a8
ptr2 u8@7ffd202aa4a8

@offsetOf understands that it should be 8 bytes, but taking the pointers returns the same address as the base address.

Also, doing this at comptime:

test "comptime the same thing" {
    comptime {
        try expect(database_settings.version.major == 0xaaaa);

        var s = MetaBlock.createNoHash();
        try expect(s.info.version.major == 0xaaaa);
    }
}

Passes the test.

[likern]

It doesn't work even without .name field.

const std = @import("std");
const Allocator = std.mem.Allocator;
const expect = std.testing.expect;

pub const DatabaseVersion = packed struct {
    major: u16,
    minor: u16,
    patch: u16,
};

pub const FileLayoutVersion = packed struct {
    major: u16,
    minor: u16,
    patch: u16,
};

pub const DatabaseSettings = packed struct {
    const Self = @This();

    //name: [8]u8,
    version: DatabaseVersion,
    layout: FileLayoutVersion,
};

pub const database_settings = DatabaseSettings{
    //.name = [8]u8{ 0x7a, 0x65, 0x6e, 0x64, 0x79, 0x20, 0x20, 0x20 },
    .version = .{ .major = 0, .minor = 0, .patch = 1 },
    .layout = .{ .major = 0, .minor = 0, .patch = 1 },
};

pub const MetaBlock = packed struct {
    const Self = @This();

    info: DatabaseSettings,

    pub fn createNoHash() Self {
        return Self{
            .info = database_settings,
        };
    }
};

test "create default metadata block" {
    try expect(database_settings.version.major == 0);

    var s = MetaBlock.createNoHash();
    std.debug.print("major: {}\n", .{s.info.version.major});

    std.debug.print("offsetOf info:    {}\n", .{@offsetOf(@TypeOf(s), "info")});
    std.debug.print("offsetOf version: {}\n", .{@offsetOf(@TypeOf(s.info), "version")});
    std.debug.print("offsetOf version.patch: {}\n", .{@offsetOf(@TypeOf(s.info.version), "patch")});

    std.debug.print("version.patch: {}\n", .{s.info.version.patch});

    try expect(s.info.version.major == 0);
    try expect(s.info.version.minor == 0);
    try expect(s.info.version.patch == 1);
}

So, in general, I consider (at least nested) packed struct as broken.

[mateuszradomski]

Lowering into LLVM IR is wrong, everything is safe-and-sound in AIR stage. Consider the following:

pub fn accessStructField(meta: MetaBlock) u16 {
    return meta.info.version.major + 1;
}

While using struct we get:

define internal fastcc i16 @accessStructField(%MetaBlock* nonnull readonly align 2 %0) unnamed_addr #1 !dbg !19091 {
Entry:
  %result = alloca i16, align 2
  call void @llvm.dbg.declare(metadata %MetaBlock* %0, metadata !19095, metadata !DIExpression()), !dbg !19096
=>%1 = getelementptr inbounds %MetaBlock, %MetaBlock* %0, i32 0, i32 0, !dbg !19097
=>%2 = getelementptr inbounds %DatabaseSettings, %DatabaseSettings* %1, i32 0, i32 1, !dbg !19099
=>%3 = getelementptr inbounds %DatabaseVersion, %DatabaseVersion* %2, i32 0, i32 0, !dbg !19100
  %4 = load i16, i16* %3, align 2, !dbg !19100
  %5 = call { i16, i1 } @llvm.uadd.with.overflow.i16(i16 %4, i16 1), !dbg !19101
  %6 = extractvalue { i16, i1 } %5, 0, !dbg !19101
  %7 = extractvalue { i16, i1 } %5, 1, !dbg !19101
  br i1 %7, label %OverflowFail, label %OverflowOk, !dbg !19101

And for packed struct we get:

define internal fastcc i16 @accessStructField(%MetaBlock* nonnull readonly align 1 %0) unnamed_addr #1 !dbg !19081 {
Entry:
  %result = alloca i16, align 2
  call void @llvm.dbg.declare(metadata %MetaBlock* %0, metadata !19085, metadata !DIExpression()), !dbg !19086
=>%1 = getelementptr inbounds %MetaBlock, %MetaBlock* %0, i32 0, i32 0, !dbg !19087
  %2 = bitcast [16 x i8]* %1 to i128*, !dbg !19087
  %3 = load i128, i128* %2, align 1, !dbg !19089
  %4 = lshr i128 %3, 0, !dbg !19089
  %5 = trunc i128 %4 to i16, !dbg !19089
  %6 = call { i16, i1 } @llvm.uadd.with.overflow.i16(i16 %5, i16 1), !dbg !19090
  %7 = extractvalue { i16, i1 } %6, 0, !dbg !19090
  %8 = extractvalue { i16, i1 } %6, 1, !dbg !19090
  br i1 %8, label %OverflowFail, label %OverflowOk, !dbg !19090

In short, only one GEP is emited, while we expect three of them, like in the non-packed case. I suspect that the logic in here might be off, because we return the struct pointer instead of generating the GEP. There is some logic below the GEP, maybe the load is premature, or the lshr is by zero and should be by something different.

[HotCRC]

std.debug.print("hex  [0x{}]\n", .{std.fmt.fmtSliceHexLower(memory)});

[iacore]

Is this the same as #11514 ?

[ghost]

I have encountered a similar issue (on 0.10.0-dev.2624+d506275a0), with the following snippet:

const Root = packed struct {
    foo: Foo = .{},
    bar: Bar = .{},

    pub fn init() Root {
        return .{};
    }
};

const Foo = packed struct {
    x: u8 = 0xAA,
    y: u16 = 0xBBCC,
};

const Bar = packed struct {
    z: u24 = 0xDDEEFF
};

const expectEqual = @import("std").testing.expectEqual;

test "comptime Root.init()" {
    const root = comptime Root.init();

    try expectEqual(@as(u8, 0xAA), root.foo.x); 
    try expectEqual(@as(u16, 0xBBCC), root.foo.y);
    try expectEqual(@as(u24, 0xDDEEFF), root.bar.z);
}

test "Root.init().foo.x" {
    try expectEqual(@as(u8, 0xAA), Root.init().foo.x);
}

test "Root.init().foo.y" {
    try expectEqual(@as(u16, 0xBBCC), Root.init().foo.y);
}

test "Root.init().bar.z" {
    try expectEqual(@as(u24, 0xDDEEFF), Root.init().bar.z);
}

The 3 last tests fail:

• foo.x: expected 170 (0xAA), found 255 (0xFF),
• foo.y: expected 48076 (0xBBCC), found 56814 (0xDDEE),
• bar.z: expected 14544639 (0xDDEEFF), found 8388168 (0x7F????) - this one looks partly unitiliazed and random each run.

Root looks like 0xFF 0xEE 0xDD 0x?? 0x?? 0x7F in memory (little-endian), where the first question mark byte is garbage, and the second one has only 6 last bits set (0b111111??) for some reason.

[Vexu]

Arrays are no longer allowed in packed structs.