Re: [PATCH v8 02/14] rust: hrtimer: introduce hrtimer support

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 3:20 AM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>
> "Tamir Duberstein" <tamird@xxxxxxxxx> writes:
>
> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>
> [...]
>
> >> >> +pub unsafe trait HrTimerHandle {
> >> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> >> >> +    /// till the handler has finished.
> >> >> +    fn cancel(&mut self) -> bool;
> >> >> +}
> >> >> +
> >> >> +/// Implemented by structs that contain timer nodes.
> >> >> +///
> >> >> +/// Clients of the timer API would usually safely implement this trait by using
> >> >> +/// the [`crate::impl_has_hr_timer`] macro.
> >> >> +///
> >> >> +/// # Safety
> >> >> +///
> >> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> >> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
> >> >> +/// implemented according to their documentation.
> >> >> +///
> >> >> +/// [`impl_has_timer`]: crate::impl_has_timer
> >> >> +pub unsafe trait HasHrTimer<T> {
> >> >> +    /// Offset of the [`HrTimer`] field within `Self`
> >> >> +    const OFFSET: usize;
> >> >
> >> > Does this need to be part of the trait? As an alternative the provided
> >> > methods could be generated in the macro below and reduce the
> >> > opportunity to implement this trait incorrectly.
> >>
> >> There is no risk of implementing the trait wrong, because it is usually
> >> derived by a macro.
> >
> > There's no risk when it's implemented by the macro, but you used the
> > word usually,  which means there is a risk.
> >
> >> We need at least one of the methods to be able to have the type system
> >> verify that the type for which we implement `HasHrTImer` actually has a
> >> field with the name we specify, and that this field has the right type.
> >> And to have that, we need the OFFSET.
> >
> > I don't follow this logic. OFFSET is calculated in the body of the
> > macro. I'm suggesting that the macro generate the method
> > implementations (which would no longer be provided). In effect I'm
> > saying: keep OFFSET private.
> >
> > I'm also noticing now that the macro generates an implementation of
> > raw_get_timer *in addition to* the provided implementation. Why are
> > both needed?
>
> HasHrTimer is unsafe, because it would be unsound to implement, if the
> type it is implemented on does not have a `Timer` at the specified
> offset.
>
> To be able to implement it safely with a macro, the macro must verify
> that the type we implement the trait on satisfies the safety
> requirement. That is, we have to have the macro verify that the type
> indeed has a field of type `Timer` with the given name. If that is the
> case, the macro can calculate OFFSET.
>
> The way we achieve this is we re-implement on of the trait methods in
> such a way that it only compiles if the type we reimplement trait
> on actually have the field of the right type.
>
> I want to generate as little code as possible in the macro, and I would
> rather rely on the default implementations given in the trait, than have
> the macro generate implementations for all the methods. Generated code
> are more difficult to reason about.

Again, I don't follow. The provided implementation of raw_get_timer is
either not used (in the presence of the macro) or it relies on the
implementer correctly setting OFFSET, which the compiler cannot check
and which can break at a distance.

Wouldn't it be simpler to just generate both functions that rely on
OFFSET? They're both one-liners that delegate to other existing
macros.