Tracking / RFC: Multiple Client traits in HILs

[lschuermann]

This is a tracking issue, going back to the discussion on the core call of Sep 15th: https://github.com/tock/tock/blob/master/doc/wg/core/notes/core-notes-2023-09-15.md#updates

tl;dr: our current HIL / client traits in these interfaces make it (a) ambiguous which set_client method a given implementation expects to be used (every supertrait still exposes the base trait's methods), and (b) impossible to compose implementations, despite them implementing all required operations.

Issue

Some of our HILs (e.g., digest) split up parts of a subsystem's exposed functionality into multiple HILs (such as DigestData, DigestHash and DigestVerify). This makes sense, as it allows underlying implementations to implement the exact set of traits matching its available operations.

Consequently, each of these HILs also takes an individual Client type (such as ClientHash), alongside an appropriate method to set this as part of the primary traits (e.g., set_hash_client). Normally, this would mean that an implementation exposing all of DigestData, DigestHash and DigestVerify would need to store one each of ClientData, ClientHash and ClientVerify. Furthermore, a user of this interface would need to invoke all of set_{data,hash,verify}_client, to set all of these client fields.

The HIL further defines "supertrait markers", which indicate that an implementation provides a given composition of traits, such as DigestDataHash (indicating that the implementation provides both DigestData and DigestHash). These "marker" traits further define their own Client traits, such as ClientDataHash, alongside their respective setters: DigestDataHash::set_client.

This trait structure poses a problem: an implementation accepting a Client cannot decompose this reference into a ClientData, ClientVerify, and ClientHash respectively. Thus, it'll have to store a dedicated Client reference. Now, imagine that a user of this interface requires an implementation that can compute hashes, but not verify them. It will accept a DigestDataHash instead of a Digest. On this type, it has the options to set a ClientData and ClientHash, or a ClientDataHash. However, the underlying T: Digest due to the aforementioned restrictions will expect a Client instead.

This creates issues with composability, as consumers of these interfaces don't know which of these methods to invoke for a given underlying type, and sometimes don't even implement the required (but less specific) traits.

Potential Options

We have two options right now:

• Always use the base set_$thing_client methods (e.g. set_data_client). Advantage: clients will only need to implement the client callbacks they actually support. Drawbacks: it takes multiple such set_client calls to wire up callbacks, and we require n times the amount of RAM to store those client references (incl. vtables).
• Use a single, unified Client trait, which has callback methods for all of the various HIL traits. Drawbacks: clients would need to provide a bunch of empty blanked trait method implementations (and let's not use default trait fn impls for this!).

At some point, we could have a hybrid approach, where set_client in supertraits would just call out to individual set_$thing_client trait methods. However, trait upcasting coercion is experimental: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=488b15d822918312ed22bbd6342c4d3b

[bradjc]

I have a prototype for option 2 for digest: https://github.com/tock/tock/compare/hmac-one-client?expand=1

I think the single client approach is a good balance of the tradeoff between compile time checks and overhead. Yes, capsules would implement a client for functions that should never be called. But, the trait matches are checked on the downcall side, so mismatches would still be caught by the compiler. The potential error that could arise (i.e. that a callback function for a trait that the capsule is not using is called) is not ideal, but I argue not that impactful. It's clearly a bug, so this should be fixed in the lower layer implementation. But, even in the case of split client traits, this bug could still exist, it's just that the client field would be None, so no client would actually be called. The difference between no function being called an empty function being called feels minimal to me.

[lschuermann]

FWIW, trait upcasting has been merged for stabilization. It's usable without feature-flags on the latest nightly compilers, and will be landing in stable sometime early next year: rust-lang/rust#118133

This will make it possible for us to have these aggregate set_client methods, while maintaining individual client fields for the more specific traits (e.g. DigestHash and ClientHash). While this is arguably more ergonomic, it'll be interesting to quantify the memory / code-size overhead incurred by that approach.