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

bugfixes in Libc.rand for Windows #32895

Merged
merged 2 commits into from
Sep 4, 2019
Merged

bugfixes in Libc.rand for Windows #32895

merged 2 commits into from
Sep 4, 2019

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Aug 14, 2019

Fixes a bug noted by @mschauer in #32894 (comment), which seems to originate in #24874 by @rfourquet, where the code used a / 2^32 expression that overflows on 32-bit machines. This PR uses * 2.0^-32 instead, which constant folds and is exact in Float64.

Also fixes another problem in the same code on Windows — the code assumed that RAND_MAX is at least 2^16-1, which seems to be true on supported Unix systems (GNU and BSD), but on Windows it is 2^15-1 so we need a third rand call to get 32 bits of randomness (or "randomness").

@stevengj stevengj added bugfix This change fixes an existing bug randomness Random number generation and the Random stdlib backport 1.0 labels Aug 14, 2019
# on non-Windows systems (in practice, it's 2^31-1)
rand(::Type{UInt32}) = ((rand() % UInt32) << 16) ⊻ (rand() % UInt32)
end
rand(::Type{Float64}) = rand(UInt32) * 2.0^-32
Copy link
Member

Choose a reason for hiding this comment

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

Use ldexp?

Copy link
Member Author

@stevengj stevengj Aug 14, 2019

Choose a reason for hiding this comment

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

@code_native g(UInt32(3)) reports an enormous amount of code for g(x) = ldexp(Float64(x), -32) for some reason…

(ldexp(x, n) only currently supports floating-point x.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think ldexp is probably slower here here because it doesn't seem to constant-propagate the exponent?

@stevengj
Copy link
Member Author

Note that Libc.rand seems to be currently unused in Base — it's only use in mktempdir was removed in #31230.

@rfourquet
Copy link
Member

Note that Libc.rand seems to be currently unused in Base — it's only use in mktempdir was removed in #31230.

Indeed a call to Libc.rand in mktempdir was replaced by a call to Base.rand in tempname. IIRC, I wrote Libc.rand in a PR before it was decided to keep rand in Base when Random was put in stdlib, but Libc.rand seemed useful to me in case Base needs to become more independant from stdlib modules.

@stevengj
Copy link
Member Author

Test failure seems unrelated?

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Aug 18, 2019

This is so strange! Windows (libC there) still is struck in 16-bit world (for compatibility with old code, why not other platforms?)? So Libc.rand(Int) is non-portable, "affecting" Julia too, with the different RAND_MAX depending on platforms?

Note that Libc.rand seems to be currently unused in Base

I'm not sure who actually uses it outside of the standard library, but shouldn't Libc.rand just be deprecated, at least less precise Libc.rand(Float64)? Other problems there, e.g. "twice" docs is outdated for Windows:

# To limit dependency on rand functionality implemented in the Random module,
# Libc.rand is used in file.jl, and could be used in error.jl (but it breaks a test)
"""

[..] for non-Windows: why not take advantage of larger Base.rand(UInt32) not needing to call rand twice?

    rand(::Type{UInt32}) = ((rand() % UInt32) << 16) ⊻ (rand() % UInt32)

rand(::Type{Float64}) = rand(UInt32) * 2.0^-32  # seems wrong, Float64 has 53 bits of precision, ok to backport fix with more?

This was referenced Aug 26, 2019
@stevengj
Copy link
Member Author

stevengj commented Sep 4, 2019

Okay to merge?

@JeffBezanson JeffBezanson merged commit 5a496c0 into master Sep 4, 2019
@JeffBezanson JeffBezanson deleted the sgj/crand branch September 4, 2019 18:03
@KristofferC KristofferC mentioned this pull request Dec 3, 2019
56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug randomness Random number generation and the Random stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants