uefi-raw: unified boolean type #1307

phip1611 · 2024-08-11T10:58:57Z

This streamlines the existing usages of u8 and bool. Note that BootPolicy from #1297 is seamlessly integrated into u8 and bool.

Steps to Undraft

close LoadFileProtocol and LoadFile2Protocol #1297 and rebase onto
reevaluate the discussion from uefi: add BootPolicy type #1326 (comment)

Checklist

Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
Update the changelog (if necessary)

nicholasbishop · 2024-08-12T21:55:44Z

bool is an interesting case in terms of how far to go with worrying about undefined behavior. Creating a bool from any value other than 0 or 1 is UB. (Miri correctly catches it: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8777a1510372038e487aea18979f78ac.) The UEFI spec says pretty much the same thing: 0 and 1 are valid, other values are undefined. However, in C code it's not actually that hard to accidentally end up with a boolean-ish value that isn't 0 or 1. For example: bitwise operators like flags & MY_FLAG -- in Rust you have to explictly convert that to a bool with (flags & MY_FLAG) != 0, but not so in C.

So this ends up being similar to enums, where we carefully construct wrappers with newtype_enum! -- they can hold any bit pattern, unlike normal enums. If we want to guard against UB, then the Boolean type should be a repr(transparent) wrapper around u8, rather than a Rust bool. I think it's definitely questionable whether it's worth worrying about, but that's the explanation for why I used u8 rather than bool in most places in uefi-raw.

phip1611 · 2024-10-01T11:28:31Z

I've come up with a new design. What do you think?

Open Question:

We could loosen the requirements. 0bxxxxxx1 can be a a valid bit pattern mapping to true. currently, this is a failure. What do you think?
Regarding check-raw and the failing CI: I'm not sure if cargo xtask check-raw brings us more benefits than breaks development 🤔 Can you please elaborate what's the idea here, @nicholasbishop ?

nicholasbishop · 2024-10-01T14:41:47Z

I think this design looks good.

We could loosen the requirements. 0bxxxxxx1 can be a a valid bit pattern mapping to true. currently, this is a failure. What do you think?

Yes, I think loosening the requirements so that any non-zero value is treated as true sounds good. This matches C behavior and is also what r-efi does: https://docs.rs/r-efi/latest/src/r_efi/base.rs.html#488.

Regarding check-raw and the failing CI: I'm not sure if cargo xtask check-raw brings us more benefits than breaks development

I feel that it does. For example, with the field pub check that this PR is failing on, almost all structs in uefi-raw should have all of their fields pub, because otherwise the struct cannot really be constructed outside the crate. It's very easy to forget while writing code, and easy to miss in code review.

In this particular case, I think the easiest thing to do is to change the definition to pub struct Boolean(pub u8). There's no harm in making the field public in uefi-raw, and the Boolean type isn't used in the public API of uefi.

This way we can be ABI-compatible and guarantee lack of UB, while being more precise in interfaces.

This aligns the behaviour with r_efi [0]. [0] https://docs.rs/r-efi/5.1.0/src/r_efi/base.rs.html#488

nicholasbishop · 2024-10-06T14:07:55Z

uefi-raw/Cargo.toml

@@ -23,5 +23,9 @@ bitflags.workspace = true
 ptr_meta.workspace = true
 uguid.workspace = true

+[features]


There's no error type in uefi-raw now, so I think the features section can be dropped

nicholasbishop · 2024-10-06T14:08:21Z

uefi-raw/CHANGELOG.md


 # uefi-raw - 0.8.0 (2024-09-09)

 ## Added

 - Added `PAGE_SIZE` constant.
-
+- Added a new `unstable` feature


No longer needed

nicholasbishop · 2024-10-06T14:14:03Z

uefi-raw/src/lib.rs

+        assert_eq!(Boolean::FALSE.0, 0);
+        assert_eq!(bool::from(Boolean(0b0)), false);
+        assert_eq!(bool::from(Boolean(0b1)), true);
+        // We do it a C: Every bit pattern not 0 is equal to true


Suggested change

// We do it a C: Every bit pattern not 0 is equal to true

// We do it as in C: Every bit pattern not 0 is equal to true

nicholasbishop · 2024-10-06T14:16:37Z

uefi/CHANGELOG.md

 - Fixed `boot::open_protocol` to properly handle a null interface pointer.

-


tiny nit: per your changes in 4d1906e, two blank lines between each version entry

nicholasbishop · 2024-10-06T14:17:54Z

uefi/src/proto/console/text/output.rs

-    pub const fn cursor_visible(&self) -> bool {
-        self.data().cursor_visible
+    pub fn cursor_visible(&self) -> bool {
+        // Panic: Misbehaving UEFI impls are so unlikely; just fail


Can drop the panic comment now that conversion cannot fail

nicholasbishop · 2024-10-06T14:18:33Z

uefi/src/proto/media/block.rs

-    pub const fn is_removable_media(&self) -> bool {
-        self.0.removable_media
+    pub fn is_removable_media(&self) -> bool {
+        // Panic: Misbehaving UEFI impls are so unlikely; just fail


Can drop all the panic comments in this file too

nicholasbishop · 2024-10-06T14:20:04Z

xtask/src/check_raw.rs

@@ -31,6 +31,7 @@ enum ErrorKind {
    ForbiddenType,
    MalformedAttrs,
    MissingPub,
+    MissingRepr,


The check_raw changes can all be dropped since there's no error type anymore.

nicholasbishop · 2024-10-06T14:21:52Z

uefi/CHANGELOG.md

@@ -15,9 +15,9 @@ details of the deprecated items that were removed in this release.
 - **Breaking:** `FileSystem` no longer has a lifetime parameter, and the
  deprecated conversion from `uefi::table::boot::ScopedProtocol` has been
  removed.
+- **Breaking:** Removed `BootPolicyError` as `BootPolicy`


Suggested change

- **Breaking:** Removed `BootPolicyError` as `BootPolicy`

- **Breaking:** Removed `BootPolicyError` as `BootPolicy` construction is no longer fallible.

phip1611 marked this pull request as draft August 11, 2024 11:00

phip1611 changed the title ~~uefi-raw: unified boolean type~~ [BLOCKED] uefi-raw: unified boolean type Aug 11, 2024

phip1611 mentioned this pull request Aug 13, 2024

uefi: add BootPolicy type #1326

Merged

2 tasks

phip1611 force-pushed the bool branch from f75772e to b8b13af Compare October 1, 2024 11:21

phip1611 marked this pull request as ready for review October 1, 2024 11:23

phip1611 changed the title ~~[BLOCKED] uefi-raw: unified boolean type~~ uefi-raw: unified boolean type Oct 1, 2024

phip1611 requested a review from nicholasbishop October 1, 2024 11:25

phip1611 force-pushed the bool branch 3 times, most recently from ab54a33 to 483dee0 Compare October 6, 2024 08:53

phip1611 added 2 commits October 6, 2024 10:53

uefi-raw: add opaque Boolean type

4bc2997

This way we can be ABI-compatible and guarantee lack of UB, while being more precise in interfaces.

uefi: streamline BootPolicy and Boolean types

6f9d1b0

phip1611 force-pushed the bool branch 2 times, most recently from fed3cee to 95e4c16 Compare October 6, 2024 09:04

phip1611 added 2 commits October 6, 2024 11:05

xtask/uefi-raw: fix/improve check-raw

89e8359

uefi-raw: make Boolean less restrictive, allow any bit pattern

492f002

This aligns the behaviour with r_efi [0]. [0] https://docs.rs/r-efi/5.1.0/src/r_efi/base.rs.html#488

phip1611 force-pushed the bool branch from 95e4c16 to 492f002 Compare October 6, 2024 09:05

nicholasbishop reviewed Oct 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uefi-raw: unified boolean type #1307

uefi-raw: unified boolean type #1307

phip1611 commented Aug 11, 2024 •

edited

Loading

nicholasbishop commented Aug 12, 2024 •

edited

Loading

phip1611 commented Oct 1, 2024 •

edited

Loading

nicholasbishop commented Oct 1, 2024

nicholasbishop Oct 6, 2024

nicholasbishop Oct 6, 2024

nicholasbishop Oct 6, 2024

nicholasbishop Oct 6, 2024

nicholasbishop Oct 6, 2024

nicholasbishop Oct 6, 2024

nicholasbishop Oct 6, 2024

nicholasbishop Oct 6, 2024

	// We do it a C: Every bit pattern not 0 is equal to true
	// We do it as in C: Every bit pattern not 0 is equal to true

		- Fixed `boot::open_protocol` to properly handle a null interface pointer.

	- Breaking: Removed `BootPolicyError` as `BootPolicy`
	- Breaking: Removed `BootPolicyError` as `BootPolicy` construction is no longer fallible.

uefi-raw: unified boolean type #1307

Are you sure you want to change the base?

uefi-raw: unified boolean type #1307

Conversation

phip1611 commented Aug 11, 2024 • edited Loading

Steps to Undraft

Checklist

nicholasbishop commented Aug 12, 2024 • edited Loading

phip1611 commented Oct 1, 2024 • edited Loading

nicholasbishop commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phip1611 commented Aug 11, 2024 •

edited

Loading

nicholasbishop commented Aug 12, 2024 •

edited

Loading

phip1611 commented Oct 1, 2024 •

edited

Loading