Prevent panics statically

[joshlf]

Issues like #200 demonstrate that the compiler is sometimes not smart enough to optimize out panics that we can prove statically won't happen. It would be good if we could:

• Refactor to only use operations which don't include panicking in their call graph
• Validate this in CI

See also #325, #1125

Mentoring instructions

• Easy steps
  • Create a new step in this CI job that runs Red Pen on zerocopy, only running the step under if: matrix.crate == 'zerocopy' && matrix.toolchain == 'nightly'
  • Annotate as many functions as possible with #[redpen::dont_panic]
• Medium to hard (depending on the code): Refactor as much code as possible to remove all panics

[jettr]

Here is a discussion on another project about having a no panic policy with some good information: https://github.com/orgs/tpm-rs/discussions/5

I think it could help the zerocopy library if it were interested in some automated CI checks to ensure no panics are present in "tier 1" tool chains.

[joshlf]

Awesome, thanks for that breadcrumb! I'd like to go even further than that, and ensure that panics aren't just optimized out, but never emitted in the first place. Do you know if that technique would work in conjunction with opt-level = 0, and would that suffice to guarantee that we're never emitting panics? I want to make sure we don't get into the situation in #200 in which panics are optimized out in a toy example but not in a larger application.

[jettr]

I like the idea of using opt-level=0 for the "tier 1" tool chains that CI checks against to ensure there are no panics. I don't know of a way to ensure panics are never emitted with 100% certainty though. I think your idea is about as close as you can get with currently tooling.

[jrvanwhy]

I'm starting to work on this. I suspect I'll run out of time to send a PR this week but I hope to send one next week.

[joshlf]

Awesome, thanks! I'll assign it to you. Feel free to comment here if you have any questions.

[jrvanwhy]

It turns out that redpen does not catch every possible panic path. For example, it does not error on this program:

pub trait MaybePanic { fn maybe_panic(&self); }

pub struct WillPanic;
impl MaybePanic for WillPanic {
    fn maybe_panic(&self) { panic!(); }
}

#[redpen::dont_panic]
pub fn will_this_panic<M: MaybePanic>(m: M) {
    m.maybe_panic();
}

#[redpen::dont_panic]
pub fn main() {
    will_this_panic(WillPanic);
}

So we have a few tools at our disposal, all of which are incomplete:

1. Linking-based checks: Build code that uses zerocopy in a way where we can look at the emitted symbols/links to determine whether a panic could have happened. no_panic is an example of this approach, but I think it could also be done using e.g. a custom #[panic_handler].
2. Red Pen: use the redpen tool to find panics, marking everything that we don’t want to panic with #[redpen::dont_panic].
3. Clippy lints: there are more clippy lints we can enable to find sources of accidental panics.

These all have their advantages and disadvantages:

Linking-based checks

Pro: Errors exactly when rustc emits a panic in generated code, which is the cost we care about.
Con: Pretty much all of zerocopy is #[inline] or generic, so we can’t just check the zerocopy rlib. We’ll have to manually maintain an additional binary/library that invokes much of zerocopy’s functionality, which adds a lot of maintenance effort. I suspect this crate will be forgotten about frequently.
Con: Sensitive to exact compiler versions, optimization levels. As previously mentioned, this could pass in zerocopy's CI when the same functionality will emit a panic in user code. Likely to break during nightly toolchain updates.
Con: Ugly error messages, which may make it difficult to identify where the panic is.

Red Pen

Note: At the moment, redpen does not change its status code to indicate whether it found any issues. If we'd like to use redpen, I'll send a PR upstream to make it report its failures, and then we'll probably want to wait for a new release of redpen before we start using it.

Pro: Catches most panics.
Pro: Clearly documents what can panic in the code (if it has #[redpen::dont_panic] then it can't panic... theoretically).
Con: Relatively new and unproven. It requires a shim crate to be added to zerocopy's [dependencies]. That shim crate is simple, but may be concerning for some of zerocopy's users. Also, if it disappears we'll have to tear it out of zerocopy's codebase, and would need another approach to avoid panics anyway.
Con: Awkward integration into the CI's toolchain test matrix, as it is pinned to its own toolchain.
Con: It's one more tool for zerocopy contributors to install and run.

Clippy lints

With Rust 1.69 (what I happen to have on my system), the following Clippy lints point to code in zerocopy that can emit panic branches:

• expect_used
• integer_arithmetic
• integer_division
• unreachable
• unwrap_in_result

I also think there is merit in enabling missing_panics_doc, although at the moment it does not point out anything in zerocopy.

Pro: Well-supported tool that zerocopy already uses which emits well-formatted error messages.
Con: Won't catch all panic causes. E.g. no clippy lint catches uses of Vec::swap_remove, which can panic.

Looking for feedback

We can choose to use any combination of linking checks, Red Pen, and Clippy to minimize panics. I would like input from zerocopy's maintainers on which approaches you would prefer. I can pursue any of them.

Here's my take:

1. Linking-based checks are more toil than they are worth.
2. I'm ambivalent on Red Pen. It is design to solve exactly the problem we have but has significant drawbacks.
3. I think we should investigate the additional Clippy lints. I can send PRs that enable and address each lint individually, and we can see if each one is beneficial in practice.

Maybe I should enable and address the Clippy lints first, then give Red Pen a try and see how many panics it catches that Clippy doesn't? Then we can evaluate whether Red Pen is worth the effort.

[joshlf]

Holy cow, thank you for such a detailed writeup!!

Maybe I should enable and address the Clippy lints first, then give Red Pen a try and see how many panics it catches that Clippy doesn't? Then we can evaluate whether Red Pen is worth the effort.

This sounds like a good plan to me!

Linking-based checks

...
Con: Pretty much all of zerocopy is #[inline] or generic, so we can’t just check the zerocopy rlib. We’ll have to manually maintain an additional binary/library that invokes much of zerocopy’s functionality, which adds a lot of maintenance effort. I suspect this crate will be forgotten about frequently.

Do you think it'd be possible to do this in zerocopy itself? A few thoughts:

• If we mark a generic function as #[no_panic] and then instantiate it with a concrete type (e.g. by doing let _ = foo::<T>;), will that exercise the check?
• Maybe we could just emit code in zerocopy itself (e.g. when cfg(test)) which takes the place of an external crate, and so requires less overhead
• Is there any way we could write an automatic check that determines whether each function has been called in this checking code?

Con: Sensitive to exact compiler versions, optimization levels. As previously mentioned, this could pass in zerocopy's CI when the same functionality will emit a panic in user code. Likely to break during nightly toolchain updates.

My hope is that we can get rid of panics without relying on the optimizer, and so we can run this check with opt-level = 0. Hopefully that will also make exact compiler versions a non-issue.

Red Pen

Note: At the moment, redpen does not change its status code to indicate whether it found any issues. If we'd like to use redpen, I'll send a PR upstream to make it report its failures, and then we'll probably want to wait for a new release of redpen before we start using it.

+1

Con: Relatively new and unproven. It requires a shim crate to be added to zerocopy's [dependencies]. That shim crate is simple, but may be concerning for some of zerocopy's users. Also, if it disappears we'll have to tear it out of zerocopy's codebase, and would need another approach to avoid panics anyway.

I was thinking we'd just have this be a dev-dependency and only check it during cargo test in CI. Would that be a viable approach?

[jrvanwhy]

Maybe I should enable and address the Clippy lints first, then give Red Pen a try and see how many panics it catches that Clippy doesn't? Then we can evaluate whether Red Pen is worth the effort.

This sounds like a good plan to me!

Alright, I'll start working on Clippy lint PRs. We can continue discussing the other options in parallel.

Linking-based checks

...
Con: Pretty much all of zerocopy is #[inline] or generic, so we can’t just check the zerocopy rlib. We’ll have to manually maintain an additional binary/library that invokes much of zerocopy’s functionality, which adds a lot of maintenance effort. I suspect this crate will be forgotten about frequently.

Do you think it'd be possible to do this in zerocopy itself? A few thoughts:

• If we mark a generic function as #[no_panic] and then instantiate it with a concrete type (e.g. by doing let _ = foo::<T>;), will that exercise the check?

With the no_panic crate, the check only happens when the object files are linked into a binary. We can use the test binary, so the panic check happens during cargo test. In that case, you can invoke a particular monomorphization of the function, which will check that monomorphization.

• Maybe we could just emit code in zerocopy itself (e.g. when cfg(test)) which takes the place of an external crate, and so requires less overhead

Yes, using the test binary works (I don't know why I didn't think of that initially).

• Is there any way we could write an automatic check that determines whether each function has been called in this checking code?

We could use a code coverage tool. If we tag a function as #[no_panic], and the coverage tool indicates that function was executed by cargo test, then I think we can be confident the no-panic check ran on that function.

Con: Sensitive to exact compiler versions, optimization levels. As previously mentioned, this could pass in zerocopy's CI when the same functionality will emit a panic in user code. Likely to break during nightly toolchain updates.

My hope is that we can get rid of panics without relying on the optimizer, and so we can run this check with opt-level = 0. Hopefully that will also make exact compiler versions a non-issue.

Sadly, with the latest stable compiler (1.73.0), even an empty function requires opt-level = 1 to pass:

// Only compiles with optimization
#[no_panic::no_panic]
#[test]
fn foo() {
}

Red Pen

Note: At the moment, redpen does not change its status code to indicate whether it found any issues. If we'd like to use redpen, I'll send a PR upstream to make it report its failures, and then we'll probably want to wait for a new release of redpen before we start using it.

+1

Con: Relatively new and unproven. It requires a shim crate to be added to zerocopy's [dependencies]. That shim crate is simple, but may be concerning for some of zerocopy's users. Also, if it disappears we'll have to tear it out of zerocopy's codebase, and would need another approach to avoid panics anyway.

I was thinking we'd just have this be a dev-dependency and only check it during cargo test in CI. Would that be a viable approach?

I suspect yes, but it's not obvious to me. redpen is its own binary, so it's unclear how to enable Rust's test harness when using it. It may or may not pay attention to command line flags and environment variables, so that may require some more work and an upstream contribution to enable.

[kupiakos]

IIUC estebank/redpen@887ea4f should fix the specific example provided in #202 (comment)

[joshlf]

Sorry, I definitely phrased my review comment badly.

A few points:

• I agree with @kupiakos that semantic panic-freedom is ideal
• I also agree that it often requires unsafe code in practice
• In cases where unsafe is required, I haven't thought through how I want to handle that. We try to avoid unsafe at all costs, so introducing new unsafe just to take our code from "this panic gets optimized out in practice" to "this semantically doesn't panic" is a tough pill to swallow. I could be convinced, though, and depending on the use cases, I could also see some more general-purpose unsafe utilities that let us do this in a way that's reusable; it'd just take engineering work that we haven't had the cycles for so far
• At an absolute minimum, a tool like redpen is useful to annotate all functions that don't currently panic so we can prevent regressions
• It would also be good to use a "hack" like link-time assertions for the cases where we can't yet prevent panics semantically, although of course I consider this a last resort
• The thing that worried me in my review comment was introducing a branch. On further reflection, a panic also introduces a branch, so I guess ? is strictly better? That said, maybe a panic coaxes the optimizer to try harder to prove that it's unreachable? I'd need to do more thinking/godbolt-ing.

I hope that provides some clarity. Let me know if you have any other questions or thoughts on how best to proceed.

[briansmith]

That said, maybe a panic coaxes the optimizer to try harder to prove that it's unreachable?

I spent some time looking into this and AFAICT the compiler doesn't try to optimize more to avoid panics. The cases I looked at were division by a non-constant (contains check-for-zero and branch to panic) and slice indexing.

For slice indexing, I basically had to remove all uses of it in favor of higher-level operations (iterators, split_at, etc.), or else get and get_mut. The optimizer does seem to consider the panic code paths "cold" pretty reliably.

[joshlf]

Ah okay cool that's good to know.

[kupiakos]

We got bit by another panic path today, again from split_at. This diff, an unambiguous ergonomic improvement, introduced a new panic path detected via the linking method:

-    let Some(mut counts) = buf
-        .get_mut(..size_of::<ResetCounts>())
-        .and_then(Ref::<_, ResetCounts>::new)
-    else {
+    let Some(mut counts) = ResetCounts::mut_from_prefix(buf) else {

I don't have stats, but I've noticed split_at, split_at_mut, and copy_from_slice are usually the panic-causing culprits.

[joshlf]

We got bit by another panic path today, again from split_at. This diff, an unambiguous ergonomic improvement, introduced a new panic path detected via the linking method:

-    let Some(mut counts) = buf
-        .get_mut(..size_of::<ResetCounts>())
-        .and_then(Ref::<_, ResetCounts>::new)
-    else {
+    let Some(mut counts) = ResetCounts::mut_from_prefix(buf) else {

I don't have stats, but I've noticed split_at, split_at_mut, and copy_from_slice are usually the panic-causing culprits.

How are you detecting these panics? Do you have static tooling that you're using? We're considering landing #1071 to help reduce panic paths, and it'd be good to have a way to test to confirm that it's actually having the desired effect.

[kupiakos]

How are you detecting these panics

@joshlf This was detected via the linking method, as mentioned. This is how we do it specifically:

• Ensure that the #[panic_handler] function calls a #[no_mangle] function. Give that function a well-defined name like panic_is_possible. Hint extremely strongly to LLVM to never optimize or inline it with #[cold], #[inline(never)], and a body containing core::hint::black_box(()). I believe you can also make the #[panic_handler] itself directly the symbol being detected.
• Build without stripping symbols - defer this to a later build step that invokes objcopy.
• After build, check if the well-defined name is present in the non-stripped output binary. We currently use objdump on the object file and grep for the symbol.

This forbids panics in a single output binary. It does not function as designed for anything that isn't fully linked.

We also could've had the panic handler produce a linking failure by referencing an undefined symbol, but we determined that the developer experience was much worse than a tool explicitly saying "a panic was introduced in this binary, but it forbids them". This tooling is not public and is highly specific to our bespoke build system.

Do you have static tooling that you're using

I'm not entirely sure what you mean by "static tooling" here. It doesn't run the code to do the detection, so it's static in that way.

[jswrenn]

Do you have a trick for working backwards from the #[no_mangle] function to the panic location?

[kupiakos]

Do you have a trick for working backwards from the #[no_mangle] function to the panic location?

@jswrenn objdump -CSd <binary> | less, then search for the panic handler symbol - you'll find a jump somewhere. There are other binary analysis tools, but this is the quick and dirty way. If you've managed to eliminate the other panics from the binary and this is a new trigger, it's usually easy to find the specific function that does it. We don't currently have automatic tooling that points to a source location or anything like that.

It's highly sensitive to compiler and build changes; it's definitely non-ideal but better than re-introducing panic paths after we managed to remove all of them in a binary. It's not uncommon that the panic is in code you didn't directly touch due to the additional optimizer pressure introduced by the change.

[joshlf]

Related: #1125

[jrvanwhy]

Redpen

estebank/redpen#7 is still open, and I haven't seen much movement in the redpen repository. The last commit message on the branch is:

[WIP]

I'll continue waiting to see if development picks up. If it becomes more actively-maintained, then I'll look at using it in zerocopy, but at the moment it seems like it's likely to be more maintenance work than it is worth.

Clippy lints

As an update of my previous summary, I've enabled every Clippy lint and dug through the results to find every not-yet-enabled lint that points to a panic in zerocopy:

1. expect_used (previously rejected in Replace a panic with an error branch #544 due to a high false positive rate)
2. missing_panics_doc
3. panic_in_result_fn
4. unreachable

The following sections include every location at which the above lints trip:

False positives

1. src/layout.rs:99 (unreachable) Only executes in a const context, guaranteed to succeed.
2. src/layout.rs:110 (unreachable) Only executes in a const context, guaranteed to succeed.
3. src/layout.rs:122 (unreachable) Only executes in a const context. This expression will panic at compile time on platforms where usize is 16 bits, but otherwise will suceed.
4. src/util/macro_util.rs:62 (unreachable) AlignOf is never constructed and cannot be constructed outside this module.

Technically panics but not actionable

1. src/layout.rs:165 (unreachable) Technically could be emitted, but it is trivial for the optimizer to remove. Removing the panic requires unsafe.
2. src/layout.rs:188 (unreachable) Technically could be emitted, but it is trivial for the optimizer to remove. Removing the panic requires unsafe.
3. src/util/macro_util.rs:77 (unreachable) Technically user code could invoke this and trigger a panic. However, MaxAlignsOf is not intended to be used by user code, it's only exposed for macros, and there's no reason to execute MaxAlignsOf::new at all (it does nothing but panic).

Panics that are part of a public API

1. src/layout.rs:455 (panic_in_result_fn)
2. src/lib.rs:3153 (panic_in_result_fn)

In both cases, these functions return Err(...) for some errors but are documented to panic for other errors. On first glance, the lint seems like an obvious improvement: why were they not made to report those errors to their caller? However, the documentation really makes it look like this API design was intentional and thought through.

Even though we can't fix these two instances without makin breaking changes, it may be worthwhile to enable panic_in_result_fn to discourage introducing similar unnecessarily-panicing functions in the future.

Panics that "shouldn't" happen and are hard to remove

Code comments indicate these panics should never happen, but it isn't obvious that the optimizer will be able to remove them.

1. src/ref.rs:624 (missing_panics_doc and expect_used)
2. src/ref.rs:660 (missing_panics_doc and expect_used)
3. src/ref.rs:772 (expect_used)
4. src/ref.rs:801 (expect_used)

Unlike in #544, there isn't a way for these functions to return if try_cast_into_no_leftover errors, because they would have to fabricate valid references out of thin air. abort is not a stable intrinsic, so the only alternative I can think of is loop {}. That is likely to be cheaper than a panic (IIRC on the embedded platforms I've worked with it's a single instruction), but it just feels excessively ugly so I'm not sure it's the right solution.

Conclusion

It probably makes sense to enable missing_panics_doc and panic_in_result_fn. I believe that adding them will discourage adding new unnecessary panics in the future.

We could also consider replacing the four expect calls with loop {}, though I think that change should be associated with benchmarks showing a decrease in code size.