disallow readStruct for packed structs

[kj4tmp]

Intended to close #12960

I did a quick grep of this entire repo and every usage of readStruct is using an extern struct.

Warning: I think this is considered a "breaking" change?

[rohlem]

Here's an alternative that should handle (ignore) byte-sized padding as well, for discussion:

pub fn readStruct(self: Self, comptime T: type) anyerror!T {
  const size_bytes = @divExact(@bitSizeOf(T), 8);
  //comptime assert(@sizeOf(T) == size_bytes); //this assert would reject types with byte-sized alignment padding, imo we could just as well support them
  var res: [size_bytes]u8 = undefined;
  try self.readNoEof(&res);
  return @bitCast(res); //already results in a compile error for auto-layout structs, an explicit check isn't needed
}

[nektro]

Suggested change

	// Packed structs may have padding due to alignment of the backing integer.
	// Therefore, only extern structs are allowed.
	comptime assert(@typeInfo(T).@"struct".layout == .@"extern");
	comptime assert(@typeInfo(T) == .@"struct");
	comptime assert(std.meta.hasUniqueRepresentation(T));

[tauoverpi]

Regular structs do not have a defined field order and the order may be changed by the compiler. This is not a sound suggestion as it will break when such happens.

[kj4tmp]

If we want to support packed structs, two questions need to be answered:

1. What to do about packed structs which have padding due to alignment? One option is to use the @bitSizeOf to ignore the padding bytes as rohelm pointed out.
2. What to do about readStructEndian? (this would be affected if packed structs are later modified to allow arrays Packed structs can't contain arrays #12547) as certain implementations of that would disallow a single bytes reversal to convert between little endian and big endian representations of packed structs. As it stands right now, a single bytes reversal is all that is needed.

I am happy to implement changes to support packed structs, or simply remove support here with this PR and open a separate issue on how to handle packed structs.

[ikskuh]

1. What to do about packed structs which have padding due to alignment? One option is to use the @bitSizeOf to ignore the padding bytes as rohelm pointed out.

packed structs don't have any padding bits by definition. i guess it would be an option to alias them to readInt() instead of reading them field by field (which doesn't work really, anyways)

[kj4tmp]

1. What to do about packed structs which have padding due to alignment? One option is to use the @bitSizeOf to ignore the padding bytes as rohelm pointed out.

packed structs don't have any padding bits by definition. i guess it would be an option to alias them to readInt() instead of reading them field by field (which doesn't work really, anyways)

Packed structs have the alignment of the backing integer. This means a packed struct with backing integer u24 will occupy 4 bytes of memory and the most significant 8 bits will be padding.

[rohlem]

1. What to do about packed structs which have padding due to alignment? One option is to use the @bitSizeOf to ignore the padding bytes as rohelm pointed out.

packed structs don't have any padding bits by definition. i guess it would be an option to alias them to readInt() instead of reading them field by field (which doesn't work really, anyways)

The source of this issue is that the current definition uses @sizeOf which may be larger than @bitSizeOf. (This is f.e. common for u24/packed struct(u24).)
Hence my suggested code above doesn't rely on that as source of truth.

[ikskuh]

1. What to do about packed structs which have padding due to alignment? One option is to use the @bitSizeOf to ignore the padding bytes as rohelm pointed out.

packed structs don't have any padding bits by definition. i guess it would be an option to alias them to readInt() instead of reading them field by field (which doesn't work really, anyways)

Packed structs have the alignment of the backing integer. This means a packed struct with backing integer u24 will occupy 4 bytes of memory and the most significant 8 bits will be padding.

Ah, this! As said, we should use std.mem.readInt anyways which afaik does it right:

zig/lib/std/io/Reader.zig

Lines 279 to 282 in 0345775

	pub inline fn readInt(self: Self, comptime T: type, endian: std.builtin.Endian) anyerror!T {
	const bytes = try self.readBytesNoEof(@divExact(@typeInfo(T).int.bits, 8));
	return mem.readInt(T, &bytes, endian);
	}

[kj4tmp]

Sounds like we want to support packed structs. I will open a separate PR for this soon (couple days). Closing.