`riscv`: Consider strategy for exception safe code

[rmsyn]

From the Rustinomicon section on exception safety:

In unsafe code, we must be exception safe to the point of not violating memory safety. 

We'll call this minimal exception safety.

There are a number of locations in the code that use explicit panics (e.g. unimplemented!, assert.*! macros). Sometime, these panicing calls are made within safe functions, which users may call inside an unsafe context.

riscv should convert all explicit and implicit panicing code into fallible code that returns an Option or Result.

For example, Mcounteren::hpm could be changed from:

    /// Supervisor "hpm\[x\]" Enable (bits 3-31)
    #[inline]
    pub fn hpm(&self, index: usize) -> bool {
        assert!((3..32).contains(&index));
        self.bits & (1 << index) != 0
    }

To the fallible:

    /// Supervisor "hpm\[x\]" Enable (bits 3-31)
    #[inline]
    pub fn hpm(&self, index: usize) -> Option<bool> {
        if (3..32).contains(&index) {
            Some(self.bits & (1 << index) != 0)
        } else {
            None
        }
    }

Any strategy will almost definitely require breaking changes, and a version bump.

[romancardenas]

Good point! I would like to discuss this with @rust-embedded/cortex-m and get to a similar design to make the developing experience more homogeneous.

[rmsyn]

I would like to discuss this with @rust-embedded/cortex-m and get to a similar design to make the developing experience more homogeneous.

Should we add this as a talking point for the next rust-embedded WG meeting?

[romancardenas]

Sure! I didn't add it to this week's meeting because I'm currently in a conference, but we can give it a try next week.

[rmsyn]

In the last WG meeting, we discussed some trade-offs of having explicit panics in the code.

Some advantages of explicit panics are:

• indicating conditions where we convey "this should never happen".
• panics also ensure a "fail-fast" condition for exceptional code where the user can't really do anything to handle the error.
• panic handlers can also use the passed in context to print out info, call subsequent specialized handlers, etc.

Some advantages of fallible functions:

• can take advantage of Rust facilities to bubble-up errors
• error handling looks more like the rest of Rust
• defers the decision to panic to the user (less opinionated)

Also, since we are discussing breaking changes, we should also consider using Rust's version of static_assert, e.g.:

impl Mcounteren {
// ...
    pub fn set_hpm<const N: usize>(&mut self, hpm: bool) {
           const { assert!(3 <= N && N < 32) };
           self.bits = bf_insert(self.bits, N, 1, hpm as usize);
    }
// ...
}

This would then change the usage to:

let mcounteren = Mcounteren::from_bits(0);
mcounteren.set_hpm::<3>(true);

The API is much less flexible.

Given some tinkering, we might come up with ergonomic helper macros to make this usable in dynamic contexts (like loops).

It may not be worth the usability tradeoffs, and/or the engineering effort. 🤷

[rmsyn]

Another possibillity is to use a ranged type wrapping the usize primitive:

master...rmsyn:riscv:riscv/ranged-index

This avoids the const_assert-style static assert, and the run-time assert currently being used.

It uses a "clamping" style of infallible conversion function, and a TryFrom implementation for a fallible conversion.

use crate::error::Error;

/// Represents an index type restricted by a given valid range.
#[repr(C)]
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub struct RangedIndex<const MIN: usize, const MAX: usize>(usize);

impl<const MIN: usize, const MAX: usize> RangedIndex<MIN, MAX> {
    /// Creates a new [RangedIndex].
    pub const fn new() -> Self {
        Self(MIN)
    }

    /// Infallible function that creates a [RangedIndex] from its inner representation.
    ///
    /// Clamps the provided value to the restricted range.
    ///
    /// If the value falls outside the valid range, it will be "clamped" to its nearest in-range
    /// value.
    pub const fn from_inner(val: usize) -> Self {
        match val {
            v if v <= MIN => Self(MIN),
            v if  v >= MAX => Self(MAX),
            v => Self(v),
        }
    }

    /// Converts the [RangedIndex] into its inner representation.
    pub const fn into_inner(self) -> usize {
        self.0
    }
}

impl<const MIN: usize, const MAX: usize> TryFrom<usize> for RangedIndex<MIN, MAX> {
    type Error = Error;

    fn try_from(val: usize) -> Result<Self, Self::Error> {
        match val {
            v if (MIN..=MAX).contains(&v) => Ok(Self(v)),
            _ => Err(Error::OutOfBounds),
        }
    }
}

[romancardenas]

Sorry, I took a few days off and couldn't step through this before.

From my experience, we should avoid using const generics, as as you mentioned it makes it more difficult to use loops and so on. Maybe it is something we can leave for future exploration.

I like the idea of moving from panics to Results, as I think it is the Rustacean way. In general, I would add a riscv::result module where we can define a common interface for errors. Then, we could progressively update the crate to stop using assertions and using results.

[rmsyn]

Sorry, I took a few days off and couldn't step through this before.

No worries, thanks for reviewing it :)

In general, I would add a riscv::result module where we can define a common interface for errors.

This would contain the crate's Error and Result types, right? Should we re-export the modules types to make them available as top-level imports, or just make the result module public?

We discussed this issue more in the meeting today, and I think the general consensus was to provide both an infallible function that panics on failure, and a fallible try_ version that returns a Result.

Using that approach, we could also avoid a minor version bump, since we would be adding the fallible function without removing anything from the current API.

[romancardenas]

I like the try_* thing, it is very Rustacean (and the Rust for Rustaceans book mentions this as a good practice in APIs). Regarding re-exporting it, I hacen't seen that on crates such as syn or tokio. They usually encapsulate error messages in a public crate::result module.