-
Notifications
You must be signed in to change notification settings - Fork 214
Add custom-fallback
Backend
#684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add custom-fallback
Backend
#684
Conversation
I've added a CI test to ensure the fallback works as intended, including asserting that we should see a compiler error when the fallback is double-defined/not provided. You can view the results here. Looking at the double-definition linking error (below), I think this is an acceptable message to receive. It clearly indicates the issue is within setting a backend, so appropriate documentation on the macro should make it straightforward for users to diagnose and resolve the issue. Compiling fallback_test v0.1.0 (/home/runner/work/getrandom/getrandom/fallback_test)
error: symbol `__getrandom_v03_fallback_fill_uninit` is already defined
--> src/main.rs:47:5
|
47 | getrandom::set_backend!(ConstantBackend);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this error originates in the macro `getrandom::set_backend` (in Nightly builds, run with -Z macro-backtrace for more info)
error: could not compile `fallback_test` (bin "fallback_test") due to 1 previous error Note that I'm using an embedded target for this test, not |
As I've wrote previously in #672, I don't think we need to introduce a trait. It's just a lot of needless complexity. Free-standing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is superior to #672 in that it avoids the issue of a custom WASM-web fallback provided by some other library accidentally being incompatible with a custom backend.
Alongside this change we should provide a crate which depends on getrandom
with the custom-backend
feature and provides the JS/web backend.
# Optional backend: custom-fallback | ||
# This flag allows a custom fallback backend. | ||
# This will be used as a last-resort instead of raising a compiler error when | ||
# no other backend is available. | ||
custom-fallback = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the documentation to say that this feature should only be enabled by a crate which provides a custom fallback.
macro_rules! use_fallback_or { | ||
($($tt: tt)*) => { | ||
cfg_if! { | ||
if #[cfg(feature = "custom-fallback")] { | ||
mod fallback; | ||
pub use fallback::Implementation; | ||
} else { | ||
$($tt)* | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This design (in addition to documentation on the custom_fallback
feature flag) mostly solves one of my concerns: linker errors due to usage of an undefined function.
/// # Safety | ||
/// | ||
/// Implementors must uphold the contracts of all methods provided by this trait. | ||
pub unsafe trait Backend { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to a trait based backend adds a lot of noise to this PR. I don't have a strong preference etiher for or against, but it should be merged in a separate PR.
I think @newpavlov would prefer not using a trait at all. @bushrat011899 do you have motivation for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rationale was that a trait can have default method implementations, but extern functions cannot. This allows the u32' and
u64` methods to be potentially defined by a backend without adding boilerplate for backends which would just use the default implementation anyway.
Personally, I prefer this structure, and it lends itself well to extension in the future (e.g., maybe the backend trait could include the customised error messages instead of putting all the backend error types in a single file, etc.)
I do think if this PR is acceptable as a whole, the trait refactor should be split out and done first and then a simpler fallback PR can follow.
Concern (minor): introducing support for new targets will become a behavioural change. E.g. it is conceivable that introducing support for WASI-P3 would change the code paths used, and also possible that this would cause run-time failure. Concern (minor): the error messages (especially for Question: should we deprecate the |
That is the case, but I think the design intent here is clear; the backend you provide as a fallback is only used if there's no other choice. If the backend you're providing is preferrable to one
Documentation around this approach will definitely need to be improved to make up for the reduced utility of the error messages. If we do create a
In my opinion, yes. I would even go as far as to suggest moving most of the optional backends out of |
Objective
wasm_js
feature? #675custom
to act as a Fallback Backend #672Solution
As an alternative to #672, and based on feedback from @newpavlov (here) and @briansmith (here), this PR adds a new last-resort optional backend,
custom-fallback
. Unlike #672, this defines new external symbols specifically for the fallback backend, rather than re-using the existing symbol forcustom
. This avoids confusing linking errors when a user intends to provide a custom backend, but a fallback has already been provided.As a precaution, the
custom-fallback
backend is behind a new feature of the same name. If the feature is not enabled, then the fallback symbols are not defined and it will not be used. As a further precaution, aRUSTFLAGS
flaggetrandom_no_custom_fallback
can be set to make the usage of thecustom-fallback
a compiler error. Since this will only raise a compiler error when the fallback is used, users can add this flag liberally without needing to understand what targetsgetrandom
officially supports.To make defining fallback backends as ergonomic as possible, this PR adds an
unsafe
trait,Backend
, and a macroset_backend
. All backends now use thisBackend
trait rather than exporting functions directly. This is done for consistency and to encourage internal API decisions to be made with the external backends in mind. The trait is markedunsafe
to highlight the implementer must provide CSPRNG-appropriate values. The methods themselves and marked safe, asgetrandom
has no safety contract it should uphold in order to call these methods.Examples
Now, users and libraries can define a backend appropriate for use as a fallback without the use of
RUSTFLAGS
. First, theBackend
trait must be implemented for some marker type:Now, a user can choose to activate the
custom-fallback
feature fromgetrandom
and use the backend with theset_backend
macro:Libraries may call this macro themselves, but since it will only be used when no other backend is available, and the way pruning at link-time works, it is highly encouraged that end-users set the backend themselves.
By using a trait as the interface for defining fallback backends, default methods for
u32
andu64
can be provided bygetrandom
, but overridden by the fallback where appropriate. Since the external functions are not part of the public API, new methods can be added toBackend
without a major release.Notes
macro_rules!
macro is used to set the backend. However, an attribute proc-macro could be used to provide an experience more similar to#[global_allocator]
or#[panic_handler]
:proc_macro
crate so I defer that to a future PR (if at all)getrandom-js
), but this would be a breaking change.custom
works as a backend to ensure this PR is not a breaking change. However, a future breaking change may want to replicate howcustom-fallback
works forcustom
, as it's a better user experience and allows overriding theu32
andu64
methods.