Skip to content
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

Make this work with overflow checks #4

Closed
japaric opened this issue Jul 12, 2018 · 15 comments
Closed

Make this work with overflow checks #4

japaric opened this issue Jul 12, 2018 · 15 comments
Labels
help wanted Extra attention is needed

Comments

@japaric
Copy link
Member

japaric commented Jul 12, 2018

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.

@japaric japaric added the help wanted Extra attention is needed label Jul 12, 2018
@vjackson725
Copy link
Contributor

vjackson725 commented Jul 15, 2018

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
Copy link
Member Author

japaric commented Jul 15, 2018

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
Copy link

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

@vjackson725
Copy link
Contributor

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
Copy link
Contributor

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

@vjackson725
Copy link
Contributor

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 rust-lang/rust#47493.
unwind_error2.txt

@MarkSwanson
Copy link

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
Copy link

Razican commented Nov 20, 2018

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

@Razican
Copy link

Razican commented Nov 25, 2018

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
Copy link
Contributor

jackmott commented Jan 21, 2019

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
Copy link

dbeckwith commented Feb 14, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Schultzer commented Feb 26, 2019

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
Copy link

vks commented May 17, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

10 participants