Support more compliation targets (including WebAssembly, no_std, and miri)
#11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `special` crate current has to be linked with the system `libm`
C library, which is not available on all targets (e.g., when compiling
to WebAssembly). This commit resolves the issue in a backward compatible
way by allowing crates that depend on `special` to *optionally* replace
`special`'s dependency on the system `libm` C library with a pure rust
reimplementation provided in the `libm` rust crate.
This commit adds a cargo feature "system_math" that is activated by
default. As long as a crate that depends on `special` doesn't explicitly
deactivate the "system_math" feature, no behavior changes, and `special`
will still be linked against the system `libm` C library. But if a crate
that uses `special` deactivates the "system_math" feature (see blow),
then `special` will use the `libm` rust crate instead.
In addition to enabling support for WebAssembly, removing this FFI
dependency also makes it possible to now use miri to test for undefined
behavior in `special` and in crates that depend on `special`:
`cargo +nightly miri test --no-default-features`
Unfortunately, benchmarks indicate that the `libm` rust crate is
slightly slower than the `libm` C library, which is why this commit
still uses the system `libm` C library as long as the "std" feature is
activated (which is the default).
**How users can deactivate the "system_math" feature:**
Application authors can deactivate the "system_math" feature by adding
the following to their `Cargo.toml`:
```
[dependencies]
special = {version = "x.y.z", default-features = false}
```
Library authors might rather want to pass this decision on to their
users. To do so, add a new default feature called "system_math" to your
library which, when activated, activates `special`'s "system_math"
feature:
```
[features]
default = ["system_math"]
system_math = ["special/system_math"]
[dependencies]
special = {version = "x.y.z", default-features = false}
```
**Rationale for activating the "system_math" feature by default:**
Making the "system_math" feature active by default makes this change
backward compatible, i.e., existing users of the `special` crate won't
have to change anything in their `Cargo.toml`. Alternatively to
introducing a default feature that turns the system dependency *on*, we
could instead introduce a non-default feature that turns the system
dependency *off*. However, this might not be forward-compatible since
cargo features have to be *additive* (i.e., adding a cargo feature must
never remove functionality), and a future version of `special` might
provide additional features that are only available if a system `libm`
C library is available. See the following part in the cargo reference
(in particular the discussion of `no_std` vs `std`, which is related):
<https://doc.rust-lang.org/cargo/reference/features.html#feature-unification>
Adds a cargo features "std" that is active by default and that, when deactivated by a downstream crate, ensures that `special` can be compiled to a `no_std` target (e.g., a microcontroller). This is the recommended way to optionally support `no_std`, see cargo reference: <https://doc.rust-lang.org/cargo/reference/features.html#feature-unification> **Changes for users:** This change is backward compatible. Existing application authors don't have to change anything. Existing library authors aren't strictly forced to change anything either, but they should consider passing on passing the optional `no_std` compatibility on to their users. If you can make your library or at least parts of it `no_std` compatible, then: * Add the line `#![cfg_attr(not(feature = "std"), no_std)]` to your library's `lib.rs`; * Avoid using any functionality from the `std` part of the rust standard library; use equivalent definitions from `core` or `alloc` instead where available. * If some features or tests of your library really require the `std` part of the standard library, then annotate them with the compiler directive `#[cfg(not(std))]` to exclude them from builds for `no_std` systems. * Add the following (or similar) to your `Cargo.toml`: ``` [features] default = ["std", "system_math"] std = ["special/std"] system_math = ["special/system_math"] [dependencies] special = {version = "x.y.z", default-features = false} ``` **Changes for contributors:** When adding new code to a library with a default "std" feature, it is easy to inadvertently break the library for `no_std` users without noticing it. This can be prevented by regularly testing without the "std" feature by running `cargo check`, `cargo test`, and `cargo +nightly bench` with the `--no-default-features` flag. To make sure that every new functionality works in `no_std` mode, avoid using the `std` crate when possible. This is usually easy for low-level libraries like `special` since most relevant functionality of the `std` crate is also available in `core` or `alloc`. If you really do need the `std` crate (e.g., for unit tests that write to a file, etc.), then you can add the `#[cfg(feature = "std")]` gate. E.g., for a unit test:
This was referenced Mar 9, 2022
robamler
commented
Mar 10, 2022
| [dependencies] | ||
| libc = "0.2" | ||
| libc = {version = "0.2", optional = true} | ||
| libm = "0.2.2" # Only used if default feature "libc" is *not* activated. |
There was a problem hiding this comment.
Should actually read: # Only used if default feature "system_math" is *not* activated.
|
I apologize for the late response. It was too much to process. Hence, the procrastination. It would be a very useful addition! I have a few questions to begin with.
|
|
Sorry for the late response on my side. I'd be happy to remove the feature gates if you'd prefer that. My motivation for these gates was not so much driven by performance but more by backward and forward compatibility. The following considerations were on my mind:
Of course, removing the feature gates would have the advantage of not splitting the ecosystem, so I'd be happy either way. Concerning PerformanceI added a commit with benchmarking code for four affected functions. Below are my results when I run the benchmark 100 times with default settings (blue) and 100 times with the
It's not a clear picture: the system math library is faster (as one would expect) for 3 out of 4 functions, but the situation is reversed for (The above benchmark results might depend strongly on the operating system that provides the system math library; above results were obtained on Ubuntu 22.04.1 with libc version 2.35 and Rust version 1.65.0-nightly (ce40690a5 2022-08-09)). |
|
Thank you for the answers! I would prefer to have one implementation, and it should be a Rust one whenever possible. As for the backward and forward compatibilities, I would not be afraid to break things at this stage. It is better to have clean code in a state we want it to be in instead of maintaining a bag of special cases. If you have time, please go ahead and reduce it to |
|
I have now updated the crate with respect to my previous comment. Thank you for your PR! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm curious if you'd be open to considering a pair of related pull requests to both
specialandprobabilitythat make both libraries more portable by adding some cargo "features". I think the functionality provided by bothspecialandprobabilityis so fundamental that both crates would be very useful on many platforms (e.g., I currently have two students who're working on a project that will eventually useprobabilityfrom WebAssembly).The downside of supporting these additional compilation targets is that it will require an extra round of testing for each future release (in addition to running
cargo test, one would also have to runcargo test --no-default-features).This PR is split into two commits that add declarations for features
system_mathandstd, respectively, toCargo.toml. Both features are activated by default to make the changes both backward and forward compatible, but they can be deactivated by downstream crates:system_mathreplacesspecial's dependency on the system "libm" C library with a re-implementation in pure rust. This allowsspecialto be used on compilation targets that don't have a system "libm" C library, such as WebAssembly.stdallows applications that usespecialto be compiled inno_stdmode (e.g., for microcontrollers).A positive side effect of these changes is that they make it possible to run
mirion this crate and potentially on downstream crates (miriis a tool to detect undefined behavior in unsafe code): runcargo +nightly miri test --no-default-featuresThe companion PR for
probabilityis at stainless-steel/probability#27.