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

Add tgamma and tgammaf #482

Merged
merged 2 commits into from
Jul 30, 2022
Merged

Add tgamma and tgammaf #482

merged 2 commits into from
Jul 30, 2022

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Jul 28, 2022

@ankane
Copy link
Contributor Author

ankane commented Jul 28, 2022

Not sure I fully understand the failure, but guessing it may be due to tgamma and tgammaf not having this line in libm:

#[cfg_attr(all(test, assert_no_panic), no_panic::no_panic)]

@Amanieu
Copy link
Member

Amanieu commented Jul 29, 2022

I believe the issue that is this line in tgamma.rs:

n = (n + 1) / 2;

In debug builds this introduces a call to panic because we still emit a check against division by zero. This can be fixed by using the div! macro defined in libm/src/math/mod.rs.

@ankane
Copy link
Contributor Author

ankane commented Jul 29, 2022

Thanks @Amanieu! I updated the call in this branch. It looks like there may be another panic.

env RUSTFLAGS='-Ccodegen-units=1' cargo test --all --release --features 'unstable musl-reference-tests'

(on Ubuntu 20.04) gives:

libm.458c1a1f-cgu.0:(.text._ZN4core3ptr58drop_in_place$LT$libm..math..tgamma..tgamma..__NoPanic$GT$17ha3e84c801b2d4a51E+0x3): undefined reference to `

ERROR[no-panic]: detected panic in function `tgamma`
'
/usr/bin/ld: /home/vagrant/libm/target/release/deps/libm-dddd41468bd528f3.libm.458c1a1f-cgu.0.rcgu.o: in function `core::ptr::drop_in_place<libm::math::tgammaf::tgammaf::__NoPanic>':
libm.458c1a1f-cgu.0:(.text._ZN4core3ptr60drop_in_place$LT$libm..math..tgammaf..tgammaf..__NoPanic$GT$17h69ee5eb5a9c950f0E+0x3): undefined reference to `

ERROR[no-panic]: detected panic in function `tgammaf`
'
collect2: error: ld returned 1 exit status

Will try to track it down, but please let me know if you have any ideas.

Edit: Looks like it may be in the s function

@ankane
Copy link
Contributor Author

ankane commented Jul 29, 2022

Looks like it was indexing. Replaced with i! macro, and will submit a PR to libm. Thanks again for the help!

@Amanieu
Copy link
Member

Amanieu commented Jul 30, 2022

I've merged the libm PR, you can update the submodule in this PR.

@ankane
Copy link
Contributor Author

ankane commented Jul 30, 2022

Great, thanks! Looks like it fixed the test suite.

@Amanieu Amanieu merged commit 318de34 into rust-lang:master Jul 30, 2022
@Amanieu
Copy link
Member

Amanieu commented Jul 30, 2022

I published a new version of the crate, you can now make PR to rust-lang/rust to update it there.

@ankane
Copy link
Contributor Author

ankane commented Jul 30, 2022

Perfect, I appreciate all the help (first time contributing to Rust).

@ankane ankane mentioned this pull request Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants