Introduce the `PanicWriter` trait, a standard mechanism for implementing synchronous writers for panic messages by bradjc · Pull Request #4684 · tock/tock

bradjc · 2025-12-05T19:00:40Z

Pull Request Overview

For a long time, Tock's kernel-provided panic utilities required some type that implements IoWrite to display panic messages. How a board created such a type was up to each board, and the io.rs files we have in boards have used a variety of methods to do this. This has had the effect of creating three issues:

These types (that implement IoWrite) are often instantiated as static muts, which we are trying to get rid of.
These types often make assumptions about the state of the underlying hardware when the panic happens, and assume the kernel initialized the hardware correctly and that the object used by the normal kernel operation can still be used during a panic.
Implementations of the types are duplicated across many boards, sometimes identically and sometimes with minor changes.

To try to fix these issues, this PR (from the static mut task force) proposes a new trait PanicWriter that chips would implement to provide a synchronous writer suitable for panic messages. Boards then only need to select the implementation of PanicWriter they want to use for panics, and the chip-provided implementation is used by debug.rs to correctly display the panic message.

The trait looks like this:

pub trait PanicWriter {
    /// The configuration data the mechanism needs to configure the writer for panic output.
    type Config;

    /// Create a new synchronous writer capable of sending panic messages.
    unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite;
}

The config type permits the board to provide whatever settings are necessary for the implementation to correctly configure the hardware for panic output.

This addresses the three concerns by:

The create_panic_writer() function must instantiate the object on the stack, so it is not declared as a static mut.
The chip-provided implementations must create a new hardware object instantiation, eliminating sharing between the main kernel and the panic handler. Also, the config provides a method for communicating exactly the settings needed by the writer.
The implementations are provided by chips and not each board, eliminating redundancy.

Testing Strategy

todo

TODO or Help Wanted

Thoughts?

Need to port every chip.

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

lschuermann

In general I agree with this approach. Given the implementation of PanicWriter will be chip-specific, and should carefully reason about whether an implementation of PanicWriter is actually safe for each UART driver, I think it might make sense to split these up over multiple PRs.

lschuermann · 2025-12-05T22:00:01Z

+    /// Create a new synchronous writer capable of sending panic messages.
+    ///
+    /// The constructed writer must be created on the stack. Because panic
+    /// will never return this is effectively a static allocation.
+    ///
+    /// The writer must implement [`IoWrite`].
+    unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite;


We can't really enforce that the return value will be stored on the stack, and I don't think it actually matters.

What does matter are the other guarantees we ought to require callers of this unsafe function to uphold:

the function must only be called once: that ensures that we will never have two instances of the resulting impl IoWrite around that may be trampling over each other, and

this function must only be called in the panic handler. In particular, after this function is called, no other code will access the hardware backing this writer. In a sense, this function will forcefully "take over" the underlying hardware, whichever state it will be in. A full system reset is required before any other code is allowed to access the underlying hardware again.

These promises by the caller are, I think, required to build reliable and sound panic writer implementations.

The other two guarantees make sense and I will add.

Re: where the writer is stored, aren't the three options statically, on the heap, or on the stack? We don't have a heap. Is there a way for the implementation to declare it statically that we would find acceptable?

@bradjc I think you're right, but I'm not even sure what the reader is supposed to do with this:

/// The constructed writer must be created on the stack. Because panic /// will never return this is effectively a static allocation.

I don't necessarily even think it's the case that it must have be static. If this drops, isn't it probably fine?

I think it's to help people like me, who are used to tock drivers being statically allocated, and being confused about how to to do that within this interface. Also, I'm worried about implementors wanting to use static_init in the implementation.

I think what bugs me about this comment specifically is the word "must", which implies that I have to do something special to make this work (which is not the case). When rephrasing this into more of an explanation, as opposed to a thing that users of this function "must" do, it is less confusing. Something like:

Because the panic handler will never return, and the returned value is only used for synchronously writing a panic message once, it is fine for the return value of this function to be stored on the panic handler's stack. No static_init! or similar is necessary.

I still would like to see this addressed.

Updated. Just removed the comment since we have examples.

alexandruradovici · 2025-12-15T12:32:33Z

+///
+/// See also the tracking issue:
+/// <https://github.com/rust-lang/rfcs/issues/2262>.
+pub trait IoWrite: core::fmt::Write {


Is there any reason why we restrict implementations to impl core::fmt::Write types?

Saves some copying and pasting. Reverted in c481254

bradjc · 2025-12-24T01:12:42Z

Ok based on last week's meeting I've:

Removed the apollo3 changes we can't easily test.
Added back the old panic routines as panic_old().
Fixed import errors so CI passes.

bradjc · 2026-01-09T18:08:11Z

I updated the nrf52840dk, including adding an implementation for segger rtt. Please take a look and see if you think it is reasonable. It doesn't try to create a brand new SeggerRttMemory, but instead uses what is already setup.

brghena

Alright, I think this is an improvement on the current state of the world and we should merge it.

bradjc · 2026-03-11T20:23:31Z

I just added support for the esp32 and tested it.

Thoughts on moving forward with this?

lschuermann

This seems close. I'd like to see the open comments addressed.

Also, have you actually gone through the implementations of the various UARTs and reasoned about whether it is safe to construct the PanicWriter at an arbitrary time? For some of the implementations, I can see that some code was changed to ensure that prior, asynchronous operations are stopped and the hardware is returned to a well-defined state. For others, I think we've just been moving code from io.rs to the respective chip crate. That's fine, but we should discuss that we haven't yet carefully reasoned about the safety of these particular PanicWriter implementations.

lschuermann · 2026-04-10T15:48:45Z

+    /// Create a new synchronous writer capable of sending panic messages.
+    ///
+    /// The constructed writer must be created on the stack. Because panic
+    /// will never return this is effectively a static allocation.
+    ///
+    /// The writer must implement [`IoWrite`].
+    unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite;


I still would like to see this addressed.

lschuermann · 2026-04-16T15:16:50Z

-    unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite;
+    /// The writer must implement [`IoWrite`] (which is just `std:io::Write`
+    /// implemented for no_std).
+    unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite + core::fmt::Write;


I think I'm still a little confused, why does the returned object need to implement core::fmt::Write? Couldn't we implement that with a zero-sized wrapper over the impl IoWrite that's being returned?

I don't know. These are interleaved throughout the kernel and I don't know what it is "supposed" to be.

Also, maybe what you are describing is what this PR was? Should I remove c481254?

Yeah, I don't think we need to block this PR on that. I still don't understand why we have both IoWrite and core::io::Write, as these seem effectively redundant (esp. the write function. Perhaps core::io::Write could be a supertrait of IoWrite, and the latter be renamed to be called RingBufferWrite? Either way, not something to hold this up on.

alexandruradovici · 2026-04-16T15:41:18Z

+    ///
+    /// The writer must implement [`IoWrite`] (which is just `std:io::Write`
+    /// implemented for no_std).
+    unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite + core::fmt::Write;


Suggested change

unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite + core::fmt::Write;

unsafe fn with_writer(config: Self::Config, f: F) -> !

where F: FnOnce(|&writer: impl IoWrite + core::fmt::Write| -> !;

If we want to make sure that this is called from and only from the panic, can we make it divergent and make it call a divergent closure?

@amit? @lschuermann? We met and discussed this 5 months ago and arrived at the current design and I can't remember everything else we considered.

I'm not sure. I think unsafe does the trick here.

bradjc · 2026-04-16T15:46:52Z

Also, have you actually gone through the implementations of the various UARTs and reasoned about whether it is safe to construct the PanicWriter at an arbitrary time? For some of the implementations, I can see that some code was changed to ensure that prior, asynchronous operations are stopped and the hardware is returned to a well-defined state. For others, I think we've just been moving code from io.rs to the respective chip crate. That's fine, but we should discuss that we haven't yet carefully reasoned about the safety of these particular PanicWriter implementations.

We're 5 months in...no one seems to want to take up this PR. I think this is a step in the right direction. Because this is happening when the entire board is crashing, I personally don't care that much about what happens.

alexandruradovici · 2026-04-16T16:04:03Z

Also, have you actually gone through the implementations of the various UARTs and reasoned about whether it is safe to construct the PanicWriter at an arbitrary time? For some of the implementations, I can see that some code was changed to ensure that prior, asynchronous operations are stopped and the hardware is returned to a well-defined state. For others, I think we've just been moving code from io.rs to the respective chip crate. That's fine, but we should discuss that we haven't yet carefully reasoned about the safety of these particular PanicWriter implementations.

We're 5 months in...no one seems to want to take up this PR. I think this is a step in the right direction. Because this is happening when the entire board is crashing, I personally don't care that much about what happens.

I tend to agree with @bradjc here, the board panics anyway, something went wrong and I don't think it matters to much.

Panics should never happen in safery critical use cases, these need to make sure at link time the panic is never called. La

lschuermann · 2026-04-20T13:32:23Z

I think I made a bad typo here:

we should discuss that we haven't yet carefully reasoned about the safety of these particular PanicWriter implementations

I meant to say: we should document that we haven't reasoned about this.

I'm in full agreement that this PR improves over the current state. That being said, we'd ultimately want to check each implementation at some point in the future, to check that they're actually sound. That should in no way hold up this PR though.

bradjc · 2026-04-20T16:05:04Z

I think I made a bad typo here:

we should discuss that we haven't yet carefully reasoned about the safety of these particular PanicWriter implementations

I meant to say: we should document that we haven't reasoned about this.

I'm in full agreement that this PR improves over the current state. That being said, we'd ultimately want to check each implementation at some point in the future, to check that they're actually sound. That should in no way hold up this PR though.

Like this? 1564b17

The `PanicWriter` trait allows creation of a synchronous, reinitialized Uart-like peripheral for use in panic handlers.

This supports passing in a Writer

tested with the panic command in the process console

clippy warning

Need to reason about whether they are sound to create during a panic.

lschuermann · 2026-04-22T02:22:23Z

Like this? 1564b17

Yes, apologies if I missed this before.

github-actions Bot added kernel nrf labels Dec 5, 2025

lschuermann reviewed Dec 5, 2025

View reviewed changes

github-actions Bot assigned alexandruradovici Dec 6, 2025

Base automatically changed from dev/panic-handler-stv-transition to master December 11, 2025 00:00

alexandruradovici reviewed Dec 15, 2025

View reviewed changes

bradjc force-pushed the dev/panic-writer2 branch from 06405bd to 45be5c6 Compare December 23, 2025 20:53

github-actions Bot added the WG-OpenTitan In the purview of the OpenTitan working group. label Dec 23, 2025

alevy self-assigned this Jan 14, 2026

brghena previously approved these changes Jan 21, 2026

View reviewed changes

bradjc dismissed brghena’s stale review via 6aa8e76 March 11, 2026 20:22

bradjc force-pushed the dev/panic-writer2 branch from 6aa8e76 to e204f1a Compare April 9, 2026 18:01

lschuermann reviewed Apr 10, 2026

View reviewed changes

lschuermann reviewed Apr 16, 2026

View reviewed changes

alexandruradovici reviewed Apr 16, 2026

View reviewed changes

alevy and others added 8 commits April 20, 2026 15:27

Prototype specialized PanicWriter for nrf52dk

9b97dd4

The `PanicWriter` trait allows creation of a synchronous, reinitialized Uart-like peripheral for use in panic handlers.

kernel: debug: panic writer: add comments

0e59c7b

kernel: move PanicWriter to platform/chip

c1d7ef6

kernel: debug: use PanicWriter in panic_print

04579f3

boards: nrf52dk: update panic call

d5abd7b

chips: nrf52: comment uart panic writer

701cb24

chips: sifive: uart: implement PanicWriter

a94566e

boards: arty: new PanicWriter

ecec8da

bradjc added 16 commits April 20, 2026 15:27

boards: wm1110dev: use PanicWriter

024ab55

IoWrite: update use paths

7898e01

boards: io.rs: use panic_old

dc9abe9

This supports passing in a Writer

chips: qemu rv32: implement panic writer

575f181

boards: qemu rv32: use panic writer

05fca97

tested with the panic command in the process console

chips: segger: add panic writer for rtt

ef33c8a

boards: nrf52840dk: update panic to panicwriter

0596431

chips: segger: rtt: fix lifetime

492f1ed

clippy warning

esp32: uart: implement PanicWriter

04787d5

boards: esp32-c3: use PanicWriter for io.rs

d47d232

boards: lpc55s69-evk on old panic

e113729

boards: esp32: fix test panic handler

e8f49a1

kernel: PanicWriter: improve comments

247b06e

iowrite: do not depend on core::fmt::Write

978e606

chips: add todo about validating the sync writers

13dfb40

Need to reason about whether they are sound to create during a panic.

boards: lora_e5_mini: update to panic_old

01a31f1

bradjc force-pushed the dev/panic-writer2 branch from 1564b17 to 01a31f1 Compare April 20, 2026 19:33

lschuermann approved these changes Apr 22, 2026

View reviewed changes

alevy approved these changes Apr 22, 2026

View reviewed changes

alevy added this pull request to the merge queue Apr 22, 2026

Merged via the queue into master with commit b037ee4 Apr 22, 2026
23 checks passed

alevy deleted the dev/panic-writer2 branch April 22, 2026 16:35

	unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite + core::fmt::Write;
	unsafe fn with_writer(config: Self::Config, f: F) -> !
	where F: FnOnce(\|&writer: impl IoWrite + core::fmt::Write\| -> !;

Uh oh!

Conversation

bradjc commented Dec 5, 2025

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

Uh oh!

lschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lschuermann Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradjc commented Dec 24, 2025

Uh oh!

bradjc commented Jan 9, 2026

Uh oh!

brghena left a comment

Choose a reason for hiding this comment

Uh oh!

bradjc commented Mar 11, 2026

Uh oh!

lschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexandruradovici Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradjc commented Apr 16, 2026

Uh oh!

alexandruradovici commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lschuermann commented Apr 20, 2026

Uh oh!

bradjc commented Apr 20, 2026

Uh oh!

lschuermann commented Apr 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

lschuermann Dec 12, 2025 •

edited

Loading

alexandruradovici Apr 16, 2026 •

edited

Loading

alexandruradovici commented Apr 16, 2026 •

edited

Loading