fix #283: add possibility to include num-traits/libm for sin and cos …

[wucke13]

…in no_std builds

[iliekturtles]

Thanks for the PR! I kicked off the build. I expect we'll see a couple errors come up from that.

[wucke13]

Fixed yet another bug, now it could work

[wucke13]

Probably this is still not ready, I got some errors (even so the test suite runs through flawlesly:

error[E0599]: no method named `cbrt` found for type `f64` in the current scope
   --> /home/wucke13/.cargo/git/checkouts/uom-c45462707dc2a646/f82bc45/src/system.rs:844:47
    |
844 |   ...                   value: self.value.cbrt(),
    |                                           ^^^^ method not found in `f64`
    |
   ::: /home/wucke13/.cargo/git/checkouts/uom-c45462707dc2a646/f82bc45/src/si/mod.rs:10:1
    |
10  | / system! {
11  | |     /// [International System of Quantities](https://jcgm.bipm.org/vim/en/1.6.html) (ISQ).
12  | |     ///
13  | |     /// ## Generic Parameters
...   |
104 | |     }
105 | | }
    | |_- in this macro invocation
    |
    = help: items from traits can only be used if the trait is in scope
    = note: the following traits are implemented but not in scope; perhaps add a `use` for one of them:
            candidate #1: `use crate::num_traits::Float;`
            candidate #2: `use crate::num_traits::real::Real;`
    = note: this error originates in the macro `system` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no method named `powi` found for type `f64` in the current scope
   --> /home/wucke13/.cargo/git/checkouts/uom-c45462707dc2a646/f82bc45/src/system.rs:910:47
    |
910 |   ...                   value: self.value.powi(E::to_i32()),
    |                                           ^^^^ method not found in `f64`
    |
   ::: /home/wucke13/.cargo/git/checkouts/uom-c45462707dc2a646/f82bc45/src/si/mod.rs:10:1
    |
10  | / system! {
11  | |     /// [International System of Quantities](https://jcgm.bipm.org/vim/en/1.6.html) (ISQ).
12  | |     ///
13  | |     /// ## Generic Parameters
...   |
104 | |     }
105 | | }
    | |_- in this macro invocation
    |
    = help: items from traits can only be used if the trait is in scope
    = note: the following traits are implemented but not in scope; perhaps add a `use` for one of them:
            candidate #1: `use crate::ConversionFactor;`
            candidate #2: `use crate::num_traits::float::FloatCore;`
            candidate #3: `use crate::num_traits::Float;`
            candidate #4: `use crate::num_traits::real::Real;`
            candidate #5: `use crate::typenum::Pow;`
    = note: this error originates in the macro `system` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no method named `sqrt` found for type `f64` in the current scope
   --> /home/wucke13/.cargo/git/checkouts/uom-c45462707dc2a646/f82bc45/src/system.rs:948:47
    |
948 |   ...                   value: self.value.sqrt(),
    |                                           ^^^^ method not found in `f64`
    |
   ::: /home/wucke13/.cargo/git/checkouts/uom-c45462707dc2a646/f82bc45/src/si/mod.rs:10:1
    |
10  | / system! {
11  | |     /// [International System of Quantities](https://jcgm.bipm.org/vim/en/1.6.html) (ISQ).
12  | |     ///
13  | |     /// ## Generic Parameters
...   |
104 | |     }
105 | | }
    | |_- in this macro invocation
    |
    = help: items from traits can only be used if the trait is in scope
    = note: the following traits are implemented but not in scope; perhaps add a `use` for one of them:
            candidate #1: `use crate::num_traits::Float;`
            candidate #2: `use crate::num_traits::real::Real;`
    = note: this error originates in the macro `system` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0599`.
error: could not compile `uom` due to 3 previous errors
warning: build failed, waiting for other jobs to finish...

[iliekturtles]

What command did you run to get that error?

[wucke13]

I got this from running cargo build from a project where I use uom, however, I spent half an hour now trying to recreate it and couldn't.

[wucke13]

Anyway, I replaced the three offending instances of freestanding calls to the float functions with explicit mentioning of the num_traits::Float trait.

[wucke13]

@iliekturtles Ok, now I could use your help. Never worked with Complex<_>.

[iliekturtles]

Complex doesn't impl Float, but it does implement most of Float's methods. Changing to directly calling the function seems to be the trick. See the patch below that does this, adds a few missing libm feature checks, and uses libm in the CI tests.

diff --git a/.github/workflows/ci-full-test-suite.yml b/.github/workflows/ci-full-test-suite.yml
index 8d6c70f..2d70e0f 100644
--- a/.github/workflows/ci-full-test-suite.yml
+++ b/.github/workflows/ci-full-test-suite.yml
@@ -48,7 +48,7 @@ jobs:
         uses: actions-rs/cargo@v1
         with:
           command: test
-          args: --verbose --no-default-features --features "autoconvert f32 si use_serde"
+          args: --verbose --no-default-features --features "autoconvert f32 si use_serde libm"
 
       - name: Test si with underlying storage types
         uses: actions-rs/cargo@v1
diff --git a/src/system.rs b/src/system.rs
index 05ac4ef..e68bc37 100644
--- a/src/system.rs
+++ b/src/system.rs
@@ -841,7 +841,7 @@ macro_rules! system {
                         Quantity {
                             dimension: $crate::lib::marker::PhantomData,
                             units: $crate::lib::marker::PhantomData,
-                            value: $crate::num_traits::Float::cbrt(self.value),
+                            value: self.value.cbrt(),
                         }
                     }
 
@@ -907,7 +907,7 @@ macro_rules! system {
                         Quantity {
                             dimension: $crate::lib::marker::PhantomData,
                             units: $crate::lib::marker::PhantomData,
-                            value: $crate::num_traits::Float::powi(self.value, E::to_i32()),
+                            value: self.value.powi(E::to_i32()),
                         }
                     }
 
@@ -945,7 +945,7 @@ macro_rules! system {
                         Quantity {
                             dimension: $crate::lib::marker::PhantomData,
                             units: $crate::lib::marker::PhantomData,
-                            value: $crate::num_traits::Float::sqrt(self.value),
+                            value: self.value.sqrt(),
                         }
                     }}
                 }
diff --git a/src/tests/quantity.rs b/src/tests/quantity.rs
index 00ceced..159d8ce 100644
--- a/src/tests/quantity.rs
+++ b/src/tests/quantity.rs
@@ -468,7 +468,7 @@ mod float {
             Test::assert_eq(&3.3.fract(), &m1.fract::<kilogram>().get::<kilogram>());
         }
 
-        #[cfg(feature = "std")]
+        #[cfg(any(feature = "std", feature = "libm"))]
         quickcheck! {
             #[allow(trivial_casts)]
             fn hypot_same(l: V, r: V) -> bool {
@@ -477,7 +477,7 @@ mod float {
             }
         }
 
-        #[cfg(all(feature = "std", feature = "autoconvert"))]
+        #[cfg(all(any(feature = "std", feature = "libm"), feature = "autoconvert"))]
         quickcheck! {
             #[allow(trivial_casts)]
             fn hypot_mixed(l: V, r: V) -> bool {
diff --git a/src/tests/system.rs b/src/tests/system.rs
index 1b5399f..46aa6dc 100644
--- a/src/tests/system.rs
+++ b/src/tests/system.rs
@@ -286,7 +286,7 @@ mod float {
                 v.classify() == Length::new::<meter>(*v).classify()
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn cbrt(v: A<V>) -> bool {
                 let l: Quantity<Q<P1, Z0, Z0>, U<V>, V> = Quantity::<Q<P3, Z0, Z0>, U<V>, V> {
@@ -298,7 +298,7 @@ mod float {
                 Test::eq(&v.cbrt(), &l.value)
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn hypot(l: A<V>, r: A<V>) -> bool {
                 Test::eq(&Length::new::<meter>(l.hypot(*r)),
@@ -315,7 +315,7 @@ mod float {
                 v.is_sign_negative() == Length::new::<meter>(*v).is_sign_negative()
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn mul_add(s: A<V>, a: A<V>, b: A<V>) -> bool {
                 let r: Quantity<Q<P2, Z0, Z0>, U<V>, V> = Length::new::<meter>(*s).mul_add(
@@ -340,13 +340,13 @@ mod float {
                 Test::eq(&v.recip(), &a.value)
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn powi(v: A<V>) -> bool {
                 Test::eq(&v.powi(3), &Length::new::<meter>(*v).powi(P3::new()).value)
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn sqrt(v: A<V>) -> TestResult {
                 if *v < V::zero() {
@@ -647,7 +647,7 @@ mod complex {
                 v.is_normal() == Length::new::<meter>(*v).is_normal()
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn cbrt(v: A<V>) -> bool {
                 let l: Quantity<Q<P1, Z0, Z0>, U<V>, V> = Quantity::<Q<P3, Z0, Z0>, U<V>, V> {
@@ -659,7 +659,7 @@ mod complex {
                 Test::eq(&v.cbrt(), &l.value)
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn mul_add(s: A<V>, a: A<V>, b: A<V>) -> bool {
                 #[allow(unused_imports)]
@@ -676,13 +676,13 @@ mod complex {
                 Test::eq(&s.mul_add(*a, *b), &r.value)
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn powi(v: A<V>) -> bool {
                 Test::eq(&v.powi(3), &Length::new::<meter>(*v).powi(P3::new()).value)
             }
 
-            #[cfg(feature = "std")]
+            #[cfg(any(feature = "std", feature = "libm"))]
             #[allow(trivial_casts)]
             fn sqrt(v: A<V>) -> TestResult {
                 let l: Quantity<Q<P1, Z0, Z0>, U<V>, V> = Quantity::<Q<P2, Z0, Z0>, U<V>, V> {

[wucke13]

@iliekturtles I applied the (unfortunately broken) patch, and rebased. Unfortunately some test still fail, now due to stack-overflows. It's not obvious to me what causes the stack-overflow. Possibly a recursion for the proxied functions, such as crate::num::Float::atan ?

[iliekturtles]

Sounds like the proxy is calling itself instead of proxying the underlying function. Turbofish, like shown below, or explicitly using the full namespace to the function should resolve. I kicked off the builds to see which functions are having an issue.

uom/src/lib.rs

Lines 544 to 546 in 7c4b27b

	fn powi(self, e: i32) -> Self {
	<V as crate::num::Float>::powi(self, e)
	}

[codecov-commenter]

Codecov Report

Merging #309 (7c4b27b) into master (5fd23fa) will increase coverage by 0.22%.
The diff coverage is 100.00%.

❗ Current head 7c4b27b differs from pull request most recent head 92834dc. Consider uploading reports for the commit 92834dc to get more accurate results

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
+ Coverage   97.52%   97.75%   +0.22%     
==========================================
  Files          67       89      +22     
  Lines        3035     3338     +303     
==========================================
+ Hits         2960     3263     +303     
  Misses         75       75              

Impacted Files	Coverage Δ
src/si/acceleration.rs	100.00% <ø> (ø)
src/si/heat_capacity.rs	100.00% <ø> (ø)
src/si/mod.rs	100.00% <ø> (ø)
src/si/molar_heat_capacity.rs	100.00% <ø> (ø)
src/si/areal_mass_density.rs	100.00% <100.00%> (ø)
src/si/areal_number_density.rs	100.00% <100.00%> (ø)
src/si/areal_number_rate.rs	100.00% <100.00%> (ø)
src/si/electric_charge_areal_density.rs	100.00% <100.00%> (ø)
src/si/electric_charge_linear_density.rs	100.00% <100.00%> (ø)
src/si/electric_charge_volumetric_density.rs	100.00% <100.00%> (ø)
... and 18 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[wucke13]

But why do some tests succeeed without having the expicit V as crate::num::Float, while other pass? Where is the semantical difference between asin and asinh?

Edit:

So havin this, it still stack-overflows:

Angle::new::<radian>(<V as crate::num::Float>::atan2(self.value, other.value))

To pinpoint which test caused the overflow, I disabled threading for tests:

cargo test --verbose --no-default-features --features "autoconvert f32 si use_serde libm" -- --test-threads=1

yields

running 225 tests
test si::absement::tests::f32::check_dimension ... ok
test si::absement::tests::f32::check_units ... ok
test si::acceleration::tests::f32::check_dimension ... ok
test si::acceleration::tests::f32::check_units ... ok
test si::angle::tests::f32::check_units ... ok
test si::angle::tests::trig::f32::atan2 ... 
thread 'main' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass '--lib'

[iliekturtles]

I tried running with RUST_BACKTRACE=1 and most of the time it just hangs. Sometimes I get the stack overflow. Running without RUST_BACKTRACE=1 always gives me the stack overflow. Possibly a libm or rustc bug? I didn't get much time to look at this issue today. Could you see if you could produce a minimal example?

[iliekturtles]

I tried letting the run sit for a while with RUST_BACKTRACE=1 and never got any output. I think the next step is attaching with a debugger and manually reviewing the stack.

[wucke13]

I let an instance of cargo test --verbose --no-default-features --features "autoconvert f32 si use_serde libm" -- --test-threads=1 run for 5 minutes now. It stuck on si::angle::tests::trig::f32::atan2. It hangs, but it does not overflow its stack and it has a constant memory utilization, resident memory is exactly 7316. Still it consistently pulls 100% CPU load on a single core. What is this?

Edit:
Calling the same thing again stack overflows. In approx 1 out of 5 tries it hangs though. Is there UB in the libm bindings?

Edit 2:
I launched the test in question with gdb. It seems that this is a failure of quickcheck, waiting for a return from

quickcheck::tester::{impl#13}::result::shrink_failure<bool, f32, f32> (g=0x7fffffffaa80, self_=0x5555564226a0 <uom::si::angle::tests::trig::f32::atan2::prop>, a=...)

takes forever.

Edit 3:
There is "Stack overflow in quickcheck case shrinking" BurntSushi/quickcheck#285
and "Infinite Repetition/Never Ending Test with f32 and f64" BurntSushi/quickcheck#295
. So this is what we are seeing here. So, if quickcheck sees an error, it does weird things. This opens up the question: why does quickcheck observe errors when using libm, but not when using std?

[wucke13]

@iliekturtles I now understand the issue, I believe. Our problem is twofold. For one, quickcheck does weird things, when a test fails. But why do tests fail? We are checking the outputs of rusts std lib and libm for equivalence. But they are not equivalent. What do you think is the correct way forward here? Should we also use libm for the other side of the check if it is compiled in (instead of std)?

[iliekturtles]

@wucke13 Sorry for the delay in responding. I've had the tab open for a while never quite get to it.

Yes, if libm is being used then we should make sure it's being used in tests as well. I don't think that would explain the test hanging however.

[wucke13]

I don't think that would explain the test hanging however.

Yes, I believe that is just BurntSushi/quickcheck#295 I guess.f

Yes, if libm is being used then we should make sure it's being used in tests as well. I don't think that would explain the test hanging however.

I'll try to implement it.

Edit:
This crates some problem. The main issue which I see is this: AFAIK activating std and libm at the same time forces us to use libm directly (instead of num_traits::Float) to access the libm specific implementations. This however is tricky to inside of the macros, since there are distinctive, different functions for f32 and f64.

[moritz-meier]

fix in rust-lang/libm#275

[iliekturtles]

Thanks for the latest update. Some errors in the build. I have a busy weekend coming up but will see if I can track down where they're coming from.

[wucke13]

@iliekturtles Do you see any chance we get this merged in the closer future?