Specify exception handlers via a trait. - #33
Conversation
d3688be to
357fecd
Compare
This is cleaner and safer than relying on unmangled names.
| /// Functions to handle aarch64 exceptions. | ||
| /// | ||
| /// The default implementations of each method will panic. | ||
| pub trait ExceptionHandlers { |
There was a problem hiding this comment.
If we want to make this even more ergonomic, we could make this trait private (say ExceptionsHandlerImpl) and instead expose a separate one, like so:
pub trait ExceptionHandlers {
// no extern "C" and raw pointers
fn sync_lower(register_state: &mut RegisterState) {
// ...
}
// more methods here
}
impl<T: ExceptionHandlers> ExceptionHandlersImpl for T {
extern "C" fn sync_lower(register_state: NonNull<RegisterState>) {
unsafe { T::sync_lower(register_state.as_mut()); }
}
// same thing for other method impls
}(do we need NonNull here instead of a reference, by the way?)
I would guess that this will be inlined anyways with the optimizations turned on, so we shouldn't even sacrifice performance or binary size with this.
There was a problem hiding this comment.
I'm inclined to keep the current trait, as it's a bit simpler than adding another layer, and writing extern "C" isn't too much hassle.
The reason I went with NonNull<RegisterState> rather than &mut RegisterState is that modifying the register state could cause undefined behaviour, so it should require an unsafe block to do so. For example changing the ELR could cause the exception handler to return to an arbitrary invalid address, or changing some general-purpose register value which the code wasn't expecting to change could likewise cause undefined behaviour.
Reading the RegisterState should always be safe though, so I could provide some kind of custom smart pointer type which has a safe method to return an &RegisterState but an unsafe method to get an &mut RegisterState. What do you think of that approach?
There was a problem hiding this comment.
I'm inclined to keep the current trait, as it's a bit simpler than adding another layer, and writing extern "C" isn't too much hassle.
Fair!
I could provide some kind of custom smart pointer type which has a safe method to return an &RegisterState but an unsafe method to get an &mut RegisterState. What do you think of that approach?
Having this kind of a smart pointer certainly sounds better than providing NonNull<..>. An alternative is to keep all fields in RegisterState private and create safe getters and unsafe setters for them. This may be a solution that gives more API stability in the long run, but is perhaps arguably unnecessarily convoluted.
There was a problem hiding this comment.
I've gone with the smart pointer approach. I don't think having private fields and unsafe setters on RegisterState would be enough, as the entire RegisterState could still be overwritten with a value from another exception.
There was a problem hiding this comment.
the entire RegisterState could still be overwritten with a value from another exception.
Ah fair, I didn't think of this. Perhaps we could work around this by returning Pin<&mut RegisterState> instead and define the setter methods with self: Pin<..>, but this seems like a more convoluted option anyways.
This is cleaner and safer than relying on unmangled names.
I've also added a pointer to the saved register state to the lower exception handlers.