Skip to content
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

Enable a panic-free Jefe for the first time #1938

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Nov 26, 2024

The supervisor task in Hubris is not permitted to panic, since it's responsible for handling panics.

Jefe has historically contained a bunch of (static) panics, many of which aren't actually possible at runtime. I've been gradually grinding away at these in my other PRs.

As of #1937, it's now possible to build a minimal Jefe (like we use on the G0) that contains no panics. So I've enabled that on donglet, and turned on the userlib/no-panic feature that will statically ensure it remains true.

Turning on dump support in Jefe causes a bunch of panics to reappear, because humpty is panic-heavy. That's a task for another day.

@cbiffle cbiffle requested review from mkeeter and hawkw November 26, 2024 19:28
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

image

Comment on lines +322 to +323
// TODO: it might be nice to fold this into a utility function
// in kipc or something
Copy link
Member

Choose a reason for hiding this comment

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

Hm, what are you imagining here? A newtype representing a validated task index that's in range? IIUC that would also mean having a newtype around a task states array in kipc that has a safe unchecked index operation taking that type, right? And at that point it feels like stuffing a lot of Jefe into the kipc crate...

Or, am I misunderstanding the idea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking maybe a wrapper for find_faulted_task that also takes an array array and indexes it for you, based on this validity assumption. So it'd be like

fn find_faulted_task_plusplus<T>(start: usize, array: &mut [T; NUM_TASKS]) -> Option<(NonZeroUsize, &mut T)>

And at that point it feels like stuffing a lot of Jefe into the kipc crate...

You're not wrong about that, but since kipc is mostly intended for the use of the supervisor... if we find a genuinely repeating pattern, moving it into the crate isn't necessarily bad.

This one might be too special purpose though, which is why I left a TODO instead of attempting to design it.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking maybe a wrapper for find_faulted_task that also takes an array array and indexes it for you, based on this validity assumption.

Oh, that's much simpler than the API I had envisioned, that seems like a good idea!

And at that point it feels like stuffing a lot of Jefe into the kipc crate...

You're not wrong about that, but since kipc is mostly intended for the use of the supervisor... if we find a genuinely repeating pattern, moving it into the crate isn't necessarily bad.

Yeah, I think it's entirely reasonable for kipc to have APIs that might be reasonably used by any supervisor implementation, which I think a find_faulted_task_in_array (or whatever you want to call it) would be. I guess it does kind of make the assumption that a supervisor implementation will want to store per-task states in an array, but I don't know why you wouldn't want to do that, and you can just not use the API if you're doing something weird...

@@ -31,6 +31,7 @@ build-util = { path = "../../build/util" }
[features]
dump = []
nano = [ "ringbuf/disabled" ]
no-panic = [ "userlib/no-panic" ]
Copy link
Member

Choose a reason for hiding this comment

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

take it or leave it: perhaps we ought to add something like:

#[cfg(all(feature = "dump", feature = "no-panic"))]
compile_error!(
      "the \"dump\" feature cannot currently be enabled when \
      \"no-panic\" is enabled,
");

at the top of main.rs?

i realize that enabling them both will get a linker error anyway when humpty tries to link with the panic impl, but it might be nicer to actually tell the person who enabled both features the specific reason Jefe is trying to panic, rather than making them go and figure it out? not a huge deal though.

The supervisor task in Hubris is not permitted to panic, since it's
responsible for handling panics.

Jefe has historically contained a bunch of (static) panics, many of
which aren't actually possible at runtime. I've been gradually grinding
away at these in my other PRs.

As of #1937, it's now possible to build a _minimal_ Jefe (like we use on
the G0) that contains no panics. So I've enabled that on donglet, and
turned on the userlib/no-panic feature that will statically ensure it
remains true.

Turning on dump support in Jefe causes a bunch of panics to reappear,
because humpty is panic-heavy. That's a task for another day.

In addition to eliminating all panic sites, this also eliminates the
last indirect function calls from the generated binaries -- meaning, the
static stack size estimate is very likely exact. So I have taken the
opportunity to shrink the stack by a bit, reclaiming 256 bytes of RAM on
this RAM-starved part.
@cbiffle cbiffle force-pushed the cbiffle/first-nopanic-jefe branch from 4568f58 to 3650dd9 Compare December 10, 2024 19:51
@cbiffle cbiffle enabled auto-merge (rebase) December 10, 2024 19:51
@cbiffle cbiffle merged commit 8437fbd into master Dec 10, 2024
125 checks passed
@cbiffle cbiffle deleted the cbiffle/first-nopanic-jefe branch December 10, 2024 19:59
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