Skip to content

std.os.uefi.protocol: ziggify function signatures #23214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Apr 1, 2025

Conversation

dotcarmen
Copy link
Contributor

@dotcarmen dotcarmen commented Mar 12, 2025

A lot of the function signatures in std.os.uefi.protocol aren't very ziggy. This PR modifies a significant number of the namespace's functions to align more with verbose Zig code.

  • Some parameters changed from * to *const and vice-versa as appropriate
  • Some pairs of parameters that represent slices were consolidated into a single slice parameter
  • Status errors associated with each function according to the UEFI Specification are handled specifically, resulting in narrow error unions being returned from each function
    • unexpected status codes result in a new call to uefi.unexpectedError, which is akin to posix.unexpectedError
  • As appropriate, return values from various functions are returned directly when the underlying function's return value is .success, rather than requiring an output pointer parameter

note: most of std.os.uefi is untested, and that's not really changing here. I'm mostly focused on making sure there are no compiler errors. Any further problems will require a future PR to address.

@dotcarmen
Copy link
Contributor Author

one problem I'm struggling to address in this PR is the potential for the underlying API to return status codes - most of std.os.uefi already assumes non-.success return values are errors, but I wonder if warnings (high bit clear) should be stored somewhere?

@dotcarmen dotcarmen force-pushed the uefi-fn-signatures branch from c5ad247 to 3c4cfa8 Compare March 12, 2025 15:39
@dotcarmen dotcarmen force-pushed the uefi-fn-signatures branch from 3c4cfa8 to 29c91b7 Compare March 13, 2025 15:42
pub fn delete(self: *File) bool {
switch (self._delete(self)) {
.success => return true,
.warn_delete_failure => return false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a good solution. Once we find an API that can return both a value and a warning it'll get awkward though.

@dotcarmen
Copy link
Contributor Author

for warnings, i'm considering a global pub var warning: Status = .success; in uefi.zig, which the user can check for a value. then, uefi.unexpectedStatus can return UnexpectedError!void and callers can return .success value.

alternatively, I can make a pub fn MaybeWarned(T: type) type which has an optional warning: ?Status field in addition to result: T. thoughts?

@dotcarmen
Copy link
Contributor Author

as mentioned in the last commit message, i've finished the initial refactorings :D I don't have time for a self-review right now, I'll get to it later today (I'm traveling in Japan for another couple days)

@dotcarmen dotcarmen marked this pull request as ready for review March 17, 2025 01:59
pub const Udp6 = @import("protocol/udp6.zig").Udp6;

pub const HiiDatabase = @import("protocol/hii_database.zig").HiiDatabase;
pub const HiiPopup = @import("protocol/hii_popup.zig").HiiPopup;

pub fn ServiceBinding(service_guid: uefi.Guid) type {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's more ServiceBinding types, and none of them provide more specific info about errors, nor do I see any of them have more functionality...

Comment on lines 290 to 294
pub fn getStatus(
self: *SimpleNetwork,
interrupt_status: ?*InterruptStatus,
tx_buf: ?*?[*]u8,
) GetStatusError!void {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol there's 4 different ways to return values here, so i just left it as is...

Comment on lines +60 to +63
pub fn outputString(self: *SimpleTextOutput, msg: [*:0]const u16) OutputStringError!bool {
switch (self._output_string(self, msg)) {
.success => return true,
.warn_unknown_glyph => return false,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another instance of a defined warning being possible, luckily this one is also easy...

getEdid should return all values via return value

nitty mcpicky

more enum Flag types

oops didnt update the signature

IpAddress union
Copy link
Collaborator

@linusg linusg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, I really like where this is going 🙂

Please ping me when you consider this done and want a full review.

@dotcarmen
Copy link
Contributor Author

ah dang, i'm getting some compiler errors as i'm integrating this into my code 😅 sorry if i push while you're reviewing, i'll try to push only once (after i get regular zig build and the zig test artifact built with refAllDeclsRecursive(std.os.uefi))

@dotcarmen
Copy link
Contributor Author

ok fixed the compiler errors :) done pushing for the (japanese) night!

@dotcarmen
Copy link
Contributor Author

addressed all feedback, ready for 🤞🏻 final review @linusg

the only changes i made that were beyond the previous reviews' comments were
dc2df7c and 478bae7 - if these are too weird, i can revert.

@dotcarmen
Copy link
Contributor Author

added 5205b19 because in my work on my next PR, i realized that the next methods should return null when the next node is the termination entry in the list - similar to how iterating over a null-terminated slice doesn't yield the null terminator in the final iteration.

@linusg
Copy link
Collaborator

linusg commented Mar 24, 2025

Apologies for the delay on this, I haven't forgotten about it yet 🙂

@dotcarmen
Copy link
Contributor Author

No worries! Thanks for letting me know

Copy link
Collaborator

@linusg linusg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got around to testing this with my own project - std.debug needs an update:

--- a/lib/std/debug.zig
+++ b/lib/std/debug.zig
@@ -629,9 +629,9 @@ pub fn defaultPanic(
             // isn't visible on actual hardware if directly booted into
             inline for ([_]?*uefi.protocol.SimpleTextOutput{ uefi.system_table.std_err, uefi.system_table.con_out }) |o| {
                 if (o) |out| {
-                    _ = out.setAttribute(uefi.protocol.SimpleTextOutput.red);
-                    _ = out.outputString(exit_msg);
-                    _ = out.setAttribute(uefi.protocol.SimpleTextOutput.white);
+                    out.setAttribute(.{ .foreground = .red }) catch {};
+                    _ = out.outputString(exit_msg) catch {};
+                    out.setAttribute(.{ .foreground = .white }) catch {};
                 }
             }

Please do a quick grep around std to see if any other UEFI-specific code paths need updating.


Once this is merged I think we should look into supporting returning std.os.uefi.Status.Error from main(), having to convert the errors back into a status every time is a bit tedious :)

@dotcarmen
Copy link
Contributor Author

dotcarmen commented Mar 31, 2025

I grepped and none of the other uefi symbols in lib/std are using these APIs 😅 they're attempting to use boot_services or runtime_services (which this PR doesn't affect) or checking the current os tag for other usage (path separaters, string selection).

none of the rest of the codebase uses anything from std.os.uefi :)

good idea! i'll open a PR in a bit :)

@dotcarmen dotcarmen requested a review from linusg March 31, 2025 21:42
@linusg linusg enabled auto-merge (squash) March 31, 2025 21:54
@linusg linusg merged commit fa86e09 into ziglang:master Apr 1, 2025
17 of 18 checks passed
@dotcarmen dotcarmen deleted the uefi-fn-signatures branch April 1, 2025 12:02
jedisct1 added a commit to jedisct1/zig that referenced this pull request Apr 2, 2025
* master:
  Sema: increment extra index even if return type is generic
  std.start: allow return uefi error union in main (ziglang#23425)
  std.os.uefi.protocol: ziggify function signatures (ziglang#23214)
  zon: normalize negative zeroes
  Elf: fix incrementally reallocating the last atom in a section
  Sema: allow `@ptrCast` slice of zero-bit type to slice of non-zero-bit type
  translate-c: fix referencing extern locals from nested blocks
  Add quota for comptime sort, add test
  std.compress.zstd: ensure window size fits into usize
  std.compress.zstd: fix OOB access in literal decode
  ci: Build stage4 and run behavior tests with it on aarch64-linux-debug.
  ci: Don't do the update-zig1 test steps on aarch64-linux.
  ci: Don't build the compiler for arm-linux-musleabihf on aarch64-linux.
  ci: Set execute bit on aarch64-linux scripts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants