Spot the bug

Challenge
1

Bonus points if you can propose an alternative code structure that would prevent this type of bug in the future.

/// Convert little endian packed bytes from EtherCAT to host representation.
///
/// Supports enums, packed structs, and most primitive types. All most have
/// bitlength divisible by 8.
pub fn packFromECat(comptime T: type, ecat_bytes: [@divExact(@bitSizeOf(T), 8)]u8) T {
    comptime assert(isECatPackable(T));
    switch (native_endian) {
        .little => {
            if (@typeInfo(T) == .@"enum") {
                return @enumFromInt(@as(@typeInfo(T).@"enum".tag_type, @bitCast(ecat_bytes)));
            }
            return @bitCast(ecat_bytes);
        },
        .big => {
            var bytes_copy = ecat_bytes;
            std.mem.reverse(u8, &bytes_copy);
            if (@typeInfo(T) == .@"enum") {
                return @enumFromInt(@as(@typeInfo(T).@"enum".tag_type, @bitCast(ecat_bytes)));
            }
            return @bitCast(bytes_copy);
        },
    }
    unreachable;
}
1 Like
4

I see it!

Avoid unnecessary use of var. When you have to use var, limit its scope with a labeled block. Here you did not need to use var at all, which would have caught the bug.

Here’s an improved implementation:

/// Convert little endian packed bytes from EtherCAT to host representation.
///
/// Supports enums, packed structs, and most primitive types. All must have
/// bit size divisible by 8.
pub fn packFromEcat(comptime T: type, ecat_bytes: *const [@divExact(@bitSizeOf(T), 8)]u8) T {
    comptime assert(isEcatPackable(T));
    return switch (@typeInfo(T)) {
        .@"enum" => |info| @enumFromInt(std.mem.readInt(info.tag_type, ecat_bytes, .little)),
        else => @bitCast(std.mem.readInt(T, ecat_bytes, .little)),
    };
}

I didn’t compile it or test it though.

7 Likes
2

in the .big branch, you dont use the reversed bytes to create the enum

3 Likes
3

This doesnt prevent the problem, but might make it less likely

/// Convert little endian packed bytes from EtherCAT to host representation.
///
/// Supports enums, packed structs, and most primitive types. All most have
/// bitlength divisible by 8.
pub fn packFromECat(comptime T: type, ecat_bytes: [@divExact(@bitSizeOf(T), 8)]u8) T {
    comptime assert(isECatPackable(T));
    var bytes = ecat_bytes;
    if (native_endian == .big) std.mem.reverse(u8, &bytes);
    if (@typeInfo(T) == .@"enum") {
                return @enumFromInt(@as(@typeInfo(T).@"enum".tag_type, @bitCast(bytes)));
            }
    return @bitCast(bytes);
    unreachable;
}
5

my final implementation, which supports packed structs, with tests


// TODO: check recursively for struct fields that are enums?
pub fn isECatPackable(comptime T: type) bool {
    if (@bitSizeOf(T) % 8 != 0) return false;
    return switch (@typeInfo(T)) {
        .@"struct" => |_struct| blk: {
            // must be a packed struct
            break :blk (_struct.layout == .@"packed");
        },
        .int, .float => true,
        .@"union" => |_union| blk: {
            // must be a packed union
            break :blk (_union.layout == .@"packed");
        },
        .@"enum" => |_enum| blk: {
            // TODO: check enum cannot contain invalid values after bitcast
            // i.e. must be non-exhaustive or represent all values of
            // backing integer.
            break :blk !_enum.is_exhaustive;
        },
        else => false,
    };
}

/// Convert little endian packed bytes from EtherCAT to host representation.
///
/// Supports enums, packed structs, and most primitive types. All must have
/// bitlength divisible by 8.
pub fn packFromECat(comptime T: type, ecat_bytes: *const [@divExact(@bitSizeOf(T), 8)]u8) T {
    comptime assert(isECatPackable(T));
    return switch (@typeInfo(T)) {
        .@"enum" => |info| @enumFromInt(std.mem.readInt(
            info.tag_type,
            ecat_bytes,
            .little,
        )),
        else => @bitCast(std.mem.readInt(
            @Int(.unsigned, @bitSizeOf(T)),
            ecat_bytes,
            .little,
        )),
    };
}

test packFromECat {
    const Command = packed struct(u8) {
        flag: bool = true,
        reserved: u7 = 0,
    };
    try std.testing.expectEqual(
        Command{},
        packFromECat(Command, &[_]u8{1}),
    );

    const Command2 = packed struct(u16) {
        flag: bool = true,
        reserved: u7 = 0,
        num: u8 = 7,
    };
    try std.testing.expectEqual(
        Command2{},
        packFromECat(Command2, &[_]u8{ 1, 7 }),
    );

    const Command3 = packed struct(u24) {
        flag: bool = true,
        reserved: u7 = 0,
        num: u16 = 0x1122,
    };
    try std.testing.expectEqual(
        Command3{},
        packFromECat(Command3, &[_]u8{ 1, 0x22, 0x11 }),
    );

    const Command4 = packed struct(u32) {
        flag: bool = true,
        reserved: u7 = 0,
        num: u16 = 0x1122,
        num2: u5 = 0x03,
        num3: u3 = 0,
    };
    try std.testing.expectEqual(
        Command4{},
        packFromECat(Command4, &[_]u8{ 1, 0x22, 0x11, 0x03 }),
    );
    const Command5 = packed struct(u40) {
        flag: bool = true,
        reserved: u7 = 0,
        num: u16 = 0x1122,
        num2: u5 = 0x03,
        num3: u3 = 0,
        num4: u8 = 0xAB,
    };
    try std.testing.expectEqual(
        Command5{},
        packFromECat(Command5, &[_]u8{ 1, 0x22, 0x11, 0x03, 0xAB }),
    );

    const EnumUnsigned = enum(u24) {
        one = 1,
        _,
    };

    try std.testing.expectEqual(
        EnumUnsigned.one,
        packFromECat(EnumUnsigned, &[_]u8{ 1, 0, 0 }),
    );

    const EnumSigned = enum(i8) {
        minus_one = -1,
        _,
    };

    try std.testing.expectEqual(
        EnumSigned.minus_one,
        packFromECat(EnumSigned, &[_]u8{0xFF}),
    );
}
6
  1. Naming issues. From the comments, I realized that the input content of EtherCAT is “packed bytes”, while the output content is native. I’m not quite sure why it’s named “packFromECat”. It feels like the direction has been reversed. If I understand correctly, the meaning of this code is “unpackFromECat” or “fromPackedECat”
  2. I think an “intermediate type” can be constructed to represent the type after byte order is packed by ECAT. This is how I usually use it:
pub fn BackingAsUnsigned(comptime T: type) type {
    return std.meta.Int(.unsigned, @bitSizeOf(T));
}
pub fn toBacking(comptime T: type, v: T) BackingAsUnsigned(T) {
    return switch (@typeInfo(T)) {
        .@"enum" => @bitCast(@intFromEnum(v)),
        else => @bitCast(v),
    };
}
pub fn fromBacking(comptime T: type, bv: BackingAsUnsigned(T)) T {
    return switch (@typeInfo(T)) {
        .@"enum" => |enum_struct| @enumFromInt(@as(enum_struct.tag_type, @bitCast(bv))),
        else => @bitCast(bv),
    };
}

pub fn Seq(comptime Native: type, comptime e: std.builtin.Endian) type {
    const Backing: type = BackingAsUnsigned(Native);
    return enum(Backing) {
        _,
        pub const endian = e;
        pub fn fromNative(n: Native) @This() {
            return @enumFromInt(std.mem.nativeTo(Backing, toBacking(Native, n), e));
        }
        pub fn toNative(self: @This()) Native {
            return fromBacking(Native, std.mem.toNative(Backing, @intFromEnum(self), e));
        }
    };
}

pub fn fromPackedECat(comptime T: type, ecat_bytes: [@divExact(@bitSizeOf(T), 8)]u8) T {
    const ecatseq = std.mem.bytesAsValue(Seq(T, .big), &ecat_bytes);
    return ecatseq.toNative();
}

test fromPackedECat {
    try std.testing.expectEqual(@as(f32, 1.5), fromPackedECat(f32, [_]u8{ 0x3F, 0xC0, 0x00, 0x00 }));
    try std.testing.expectEqual(@as(f32, -2.25), fromPackedECat(f32, [_]u8{ 0xC0, 0x10, 0x00, 0x00 }));
    try std.testing.expectEqual(@as(f64, 123.456), fromPackedECat(f64, [_]u8{ 0x40, 0x5E, 0xDD, 0x2F, 0x1A, 0x9F, 0xBE, 0x77 }));
    try std.testing.expectEqual(@as(u32, 0x12345678), fromPackedECat(u32, [_]u8{ 0x12, 0x34, 0x56, 0x78 }));
    try std.testing.expectEqual(@as(i64, -2), fromPackedECat(i64, [_]u8{ 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE }));
    const Color = enum(u16) {
        red = 1,
        green = 300,
        blue = 65535,
    };
    try std.testing.expectEqual(Color.green, fromPackedECat(Color, [_]u8{ 0x01, 0x2C }));
    const Header = packed struct {
        a: u8,
        b: u16,
        c: u8,
    };
    const h = Header{
        .a = 0x12,
        .b = 0x3456,
        .c = 0xFF,
    };
    try std.testing.expectEqual(h, fromPackedECat(Header, [_]u8{ 0xFF, 0x34, 0x56, 0x12 }));
}