rust-lang / libm

A port of MUSL's libm to Rust.
Other
542 stars 97 forks source link

Make this work with overflow checks #4

Closed japaric closed 5 years ago

japaric commented 6 years ago

Some math functions currently panic when compiled with overflow checks enabled (which is the default for dev builds). To fix this problem the Wrapping newtype and/or wrapping ops needs to be used in some parts of the implementations.

To test that this works with overflow checks enabled uncomment this line in ci/script.sh.

vjackson725 commented 6 years ago

I've done some work on this. It's not very principled at the moment, I just ran the tests on debug mode.

I'm investigating fuzzing to see if I can get better coverage.

Link: issue-4

japaric commented 6 years ago

Just want to note that this is not needed to get math support in wasm / core because the math functions will be compiled in release mode. It is however required to use the libm crate with the dev profile.

little-arhat commented 6 years ago

What 's the status of this issue (and #142)? AFAIK, it blocks https://github.com/rust-num/num-traits/issues/75 .

vjackson725 commented 6 years ago

I haven't done anything since I commented, fuzzing was taking to long on my laptop. I'll send a pull request for what I did, which won't be guaranteed to fix all the problems, but should be an improvement.

vjackson725 commented 6 years ago

I'm getting linker errors, so I will be delayed a bit.

vjackson725 commented 6 years ago

I am getting linker errors when running cross test --target x86_64-unknown-linux-gnu, and I'm not sure how to fix the problem. There seem to be two issues here.

One is that it can't find thou_shalt_not_panic. unwind_error.txt

Replacing thou_shalt_not_panic with an infinite loop doesn't help. This one's probably related to https://github.com/rust-lang/rust/issues/47493. unwind_error2.txt

MarkSwanson commented 6 years ago

Just a fyi: I found a case that may need to use wrapping:

use libm::F64Ext; // adds methods to `f64`
pub fn sqrt(x: f64) -> f64 {
    x.sqrt()
}

Calling sqrt(2.0) in debug mode gives me: (using git master branch Oct 31, 2018)

thread 'math1' panicked at 'attempt to add with overflow', /home/mswanson/.cargo/git/checkouts/libm-62d0d08057355aaa/3559e70/src/math/sqrt.rs:180:18
Razican commented 5 years ago

I also get addition overflow panics with sqrt() at lines 138, 144 and 159 of the file.

Razican commented 5 years ago

I did some fixes here that fix the issues I was having using the vsop87 crate. Not sure if this is how we should fix them, given that the Wrapping API is nightly-only.

jackmott commented 5 years ago

I got a subtract with overflow panic on floorf.rs just now. Is there a way to use a function attribute to disable these checks?

dbeckwith commented 5 years ago

I am getting overflow errors from sin (libm-0.1.2/src/math/sqrt.rs:168:18) in debug mode , on the expression (45. * (core::f64::consts::PI / 180.)).sin(). Is there a way around this, or a fix coming soon for this?

jackmott commented 5 years ago

I ended up just forking this whole thing and fixing it by hand. You can see some of the PRs for examples of how to address it. Seems the maintainer is not around anymore.

vjackson725 commented 5 years ago

To clear some things up: I did some initial work on this, but it was never pushed.

Then holidays ended, and I had less time to work on it. When I tried to come back to it, I got linker errors I couldn't solve.

Schultzer commented 5 years ago

Hi @japaric,

Is this the right way to solve the overflow/underflow problems? #153

for me it seams more clean than using a wrapper type.

I'm willing to tackle this issue.

vks commented 5 years ago

I believe this can be closed now that #168 was merged.