zig icon indicating copy to clipboard operation
zig copied to clipboard

change uefi packed structs to new integer backed syntax

Open truemedian opened this issue 3 years ago • 11 comments
trafficstars

  • device_path_protocol now uses extern structs with align(1) fields because the transition to integer backed packed struct broke alignment

  • added comptime asserts that device_path_protocol structs do not violate alignment and size specifications

truemedian avatar Oct 15 '22 18:10 truemedian

Thanks for the help reviewing, @ominitay.

The CI failure will be fixed by running zig fmt:

Looking for non-conforming code formatting...
../lib/std/os/uefi/protocols/device_path_protocol.zig

andrewrk avatar Oct 17 '22 11:10 andrewrk

Whoops, looks like I forgot the newline at the end of the file :sweat:

truemedian avatar Oct 17 '22 14:10 truemedian

Might be worth adding std.testing.refAllDeclsRecursive(@This()) to uefi.zig to ensure that all the packed struct stuff gets checked and stays fixed fwiw

fifty-six avatar Oct 17 '22 18:10 fifty-six

The issue with running tests on them is that there is no testing infrastructure for uefi, there is simply no way to run them without doing something weird with the test runner. And you can't safely run the tests on another platform because of potential differences in the abi

truemedian avatar Oct 17 '22 18:10 truemedian

Ah ok. I assume that would change extern struct stuff then? Because refAllDecls would just be to ensure it stays compiling, not much else afaik.

fifty-six avatar Oct 17 '22 18:10 fifty-six

It would appear that the change making packed structs backed by integers also made them no longer align(1). So I guess I also need to go through and align(1) all the fields manually.

truemedian avatar Oct 17 '22 18:10 truemedian

Tested this with my bootloader and it works again after changing those 2 alignCasts! :tada:

fifty-six avatar Oct 18 '22 03:10 fifty-six

I've cleaned up the commit history, if people can verify that it also works for them it seems like this can be merged.

truemedian avatar Oct 18 '22 03:10 truemedian

Can confirm it still works, lgtm

fifty-six avatar Oct 18 '22 03:10 fifty-six

I finally found the important words for DevicePathProtocol.

Therefore all nodes are byte-packed data structures that may appear on any byte boundary. All code references to device path notes must assume all fields are unaligned.

Therefore, I've gone ahead and made all strings in device_path_protocol align(1) and removed all alignCasts. I also added a note so that this doesn't get confusing in the future.

truemedian avatar Oct 19 '22 16:10 truemedian

Is anything else required to merge this PR?

davidgmbb avatar Oct 23 '22 16:10 davidgmbb

I think everything that broke has been fixed, lgtm

truemedian avatar Oct 24 '22 18:10 truemedian

Maybe rebase against master to pass the CI?

davidgmbb avatar Oct 28 '22 17:10 davidgmbb

As far as I can tell, the compiler is failing on the stdlib tests. From what I can tell this is because std.os.uefi.Status is a enum(usize) and most of its fields have the high bit set.

thread 93367 panic: integer cast truncated bits
/workspace/src/value.zig:1181:58: 0x66eda97 in toSignedInt (zig)
            .int_u64 => return @intCast(i64, self.castTag(.int_u64).?.data),
                                                         ^
codegen/llvm.zig:1508:64: 0x6706bdf in lowerDebugTypeImpl (zig)
codegen/llvm.zig:1444:36: 0x656715e in lowerDebugType (zig)

The code in lowerDebugTypeImpl points to https://github.com/ziglang/zig/issues/645.

It would seem that zig cannot generate debug info for these types, however I think the blind intCast to i64 is the wrong idea here, since it makes this enum impossible.

I believe that this error has never come up before since std.os.uefi has never been included in the test suite.

truemedian avatar Oct 29 '22 01:10 truemedian

Note that you can easily check locally whether the CI will fail by running this command:

stage3/bin/zig test ../lib/std/std.zig -target x86_64-linux

andrewrk avatar Oct 29 '22 19:10 andrewrk