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.

Sign up for GitHub

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

Jump to bottom

fix incorrect results in expm1(::Union{Float16, Float32}) #50989

Merged
merged 2 commits into from
Aug 21, 2023
Merged

fix incorrect results in expm1(::Union{Float16, Float32}) #50989

merged 2 commits into from
Aug 21, 2023

Conversation

oscardssmith
Copy link
Member

@oscardssmith oscardssmith commented Aug 20, 2023 •
edited

unsafe_trunc(UInt, -1.0) is unspecified behavior but worked fine on apple and AMD so we didn't notice??? This has been very broken since 1.7

@oscardssmith
fix UB in `expm1(::Union{Float16, Float32})
b37bc9a 
`unsafe_trunc(UInt, -1.0)` is UB but worked fine on apple and AMD so we didn't notice??? This has been very broken since 1.7
@oscardssmith oscardssmith added status:priority This should be addressed urgently kind:bugfix This change fixes an existing bug kind:embarrassing-bugfix Whoops! backport 1.6 Change should be backported to release-1.6 backport 1.8 Change should be backported to release-1.8 backport 1.9 Change should be backported to release-1.9 backport 1.10 Change should be backported to the 1.10 release kind:correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing labels Aug 20, 2023
@oscardssmith oscardssmith mentioned this pull request Aug 20, 2023
Merged
@oscardssmith
Copy link
Member Author

Note that there are no tests for this because we were already pretty extensively testing these functions and this bug only shows up on intel and none of our CI machines are intel aparently...

@DilumAluthge DilumAluthge added needs tests Unit tests are required for this change and removed needs tests Unit tests are required for this change labels Aug 20, 2023
@giordano
Copy link
Contributor

Note that there are no tests for this because we were already pretty extensively testing these functions and this bug only shows up on intel and none of our CI machines are intel aparently...

I don't think that's a justification for not adding tests, nothing prevents us from having CI agents running on Intel CPUs in the future, and having tests that can potentially catch bugs on users machine is useful regardless of what current CI setup is.

@oscardssmith
Copy link
Member Author

I was mis-remembering. I thought we had a ton of tests for expm1 but it turns out that we we just ran a ton of tests when implementing it (which passed cause AMD) and CI has very few expm1 tests. Test added.

@oscardssmith oscardssmith changed the title fix UB in expm1(::Union{Float16, Float32}) fix incorrect results in expm1(::Union{Float16, Float32}) Aug 21, 2023
gbaraldi
gbaraldi reviewed Aug 21, 2023
View reviewed changes
base/special/exp.jl
@@ -460,7 +460,7 @@ function expm1(x::Float32)
end
x = Float64(x)
N_float = round(x*Ln2INV(Float64))
N = unsafe_trunc(UInt64, N_float)
N = unsafe_trunc(Int64, N_float)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine MAX_EXP is much smaller than the maximum integer here? If so LGTM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. N will end up as the exponent for the float so it will be in [-128,128]

@brenhinkeller brenhinkeller added the status:merge me PR is reviewed. Merge when all tests are passing label Aug 21, 2023
@oscardssmith oscardssmith merged commit 61ebaf6 into master Aug 21, 2023
9 checks passed
@oscardssmith oscardssmith deleted the oscardssmith-expm1-UB branch August 21, 2023 03:29
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Aug 21, 2023
@devmotion
Copy link
Contributor

AFAICT the backport-1.6 should be removed and instead a backport-1.7 label be added since the issue seems to affect only Julia >= 1.7.0-beta1: JuliaStats/LogExpFunctions.jl#74 (comment)

KristofferC pushed a commit that referenced this pull request Aug 23, 2023
@oscardssmith
fix incorrect results in expm1(::Union{Float16, Float32}) (#50989)
f58e1eb 
`unsafe_trunc(UInt, -1.0)` is unspecified behavior but worked fine on
apple and AMD so we didn't notice??? This has been very broken since 1.7.

(cherry picked from commit 61ebaf6)
@KristofferC
Copy link
Sponsor Member

For reference, a bit of previous discussion about unsafe_trunc: #42031 (comment)

@KristofferC KristofferC mentioned this pull request Oct 3, 2023
Merged
31 tasks
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Oct 3, 2023
KristofferC pushed a commit that referenced this pull request Oct 11, 2023
@oscardssmith
fix incorrect results in expm1(::Union{Float16, Float32}) (#50989)
8790bc1 
`unsafe_trunc(UInt, -1.0)` is unspecified behavior but worked fine on
apple and AMD so we didn't notice??? This has been very broken since 1.7.

(cherry picked from commit 61ebaf6)
This was referenced Oct 11, 2023
Merged
Open
KristofferC added a commit that referenced this pull request Nov 2, 2023
@KristofferC
Backports release 1.10 (#51563)
1391444 
Backported PRs:
- [x] #50932 <!-- types: fix hash values of Vararg -->
- [x] #50975 <!-- Use rr-safe `nopl; rdtsc` sequence -->
- [x] #50989 <!-- fix incorrect results in `expm1(::Union{Float16,
Float32})` -->
- [x] #51284 <!-- Avoid infinite loop when doing SIGTRAP in arm64-apple
-->
- [x] #51332 <!-- Add s4 field to Xoshiro -->
- [x] #51397 <!-- call Pkg precompile hook in latest world -->
- [x] #51405 <!-- Remove fallback that assigns a module to inlined
frames. -->
- [x] #51491 <!-- Throw clearer ArgumentError for strip with two string
args -->
- [x] #51531 <!-- fix `_tryonce_download_from_cache` (busybox.exe
download error) -->
- [x] #51541 <!-- Fix string index error in tab completion code -->
- [x] #51530 <!-- Don't mark nonlocal symbols as hidden -->
- [x] #51557 <!-- Fix last startup & shutdown precompiles -->
- [x] #51512 <!-- avoid limiting Type{Any} to Type -->
- [x] #51595 <!-- reset `maxprobe` on `empty!` -->
- [x] #51582 <!-- Aggressive constprop in LinearAlgebra.wrap -->
- [x] #51592 <!-- correctly track element pointer in heap snapshot -->
- [x] #51326 <!-- complete false & true more generally as vals -->
- [x] #51376 <!-- make `hash(::Xoshiro)` compatible with `==` -->
- [x] #51557 <!-- Fix last startup & shutdown precompiles -->
- [x] #51845 
- [x] #51840 
- [x] #50663 <!-- Fix Expr(:loopinfo) codegen -->
- [x] #51863 <!-- LLVM 15.0.7-9 -->

Contains multiple commits, manual intervention needed:

- [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are
now first class -->
- [ ] #51092 <!-- inference: fix bad effects for recursion -->

Non-merged PRs with backport label:
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #51414 <!-- improvements on GC scheduler shutdown -->
- [ ] #51366 <!-- Handle infix operators in REPL completion -->
- [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib"
check regardless of the value of `ispath(f)` -->
- [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating
functions in Base -->
- [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
nalimilan pushed a commit that referenced this pull request Nov 5, 2023
@oscardssmith @nalimilan
fix incorrect results in expm1(::Union{Float16, Float32}) (#50989)
ec76d27 
`unsafe_trunc(UInt, -1.0)` is unspecified behavior but worked fine on
apple and AMD so we didn't notice??? This has been very broken since 1.7.

(cherry picked from commit 61ebaf6)
KristofferC added a commit that referenced this pull request Nov 7, 2023
@KristofferC
[release-1.9] Backports for Julia 1.9 (#50977)
bc55f44 
Backported PRs:
- [x] #49357 <!-- Fix unclosed code fence in src/manual/methods.md -->
- [x] #50842 <!-- Avoid race conditions with recursive rm -->
- [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor
-->
- [x] #50730 <!-- Fix integer overflow in `isapprox` -->
- [x] #50823 <!-- Make ranges more robust with unsigned indexes. -->
- [x] #50915 <!-- Add note the `Task` about sticky bit -->
- [x] #50989 <!-- fix incorrect results in `expm1(::Union{Float16,
Float32})` -->
- [x] #50912 <!-- Separate foreign threads into a :foreign threadpool
-->
- [x] #51019 <!-- fix a case of potentially use of undefined variable
when handling error in distributed message processing -->
- [x] #51222 <!-- Check again if the tty is open inside the IO lock -->
- [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite
trailing dot -->
- [x] #51284 <!-- Avoid infinite loop when doing SIGTRAP in arm64-apple
-->
- [x] #51491 <!-- Throw clearer ArgumentError for strip with two string
args -->
- [x] #51531 <!-- fix `_tryonce_download_from_cache` (busybox.exe
download error) -->
@devmotion devmotion mentioned this pull request Nov 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbaraldi gbaraldi gbaraldi left review comments

Assignees
No one assigned
Labels
backport 1.6 Change should be backported to release-1.6 backport 1.8 Change should be backported to release-1.8 backport 1.9 Change should be backported to release-1.9 kind:bugfix This change fixes an existing bug kind:correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing kind:embarrassing-bugfix Whoops! status:priority This should be addressed urgently
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@oscardssmith @giordano @devmotion @KristofferC @gbaraldi @DilumAluthge @brenhinkeller