JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.63k stars 5.48k forks source link

Non thread-safe use of `setrounding` in construction of `BigFloat` from `Rational` #52859

Open Joel-Dahne opened 9 months ago

Joel-Dahne commented 9 months ago

The method for constructing a BigFloat from a Rational uses setrounding in a way that is not thread-safe. I could consistently reproduce the issue with the following code:

r = 1 // 3
x = (BigFloat(r, RoundDown), BigFloat(r, RoundUp))
Threads.@threads for i in 1:10000
    y = BigFloat(r, ifelse(isodd(i), RoundDown, RoundUp))
    @assert x[mod1(i, 2)] == y
end

If running Julia with only one thread the assert is always ok, but if using 2 (or more) threads it fails (most of the time).

My Julia version is

julia> versioninfo()
Julia Version 1.10.0
Commit 3120989f39b (2023-12-25 18:01 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, tigerlake)
  Threads: 2 on 8 virtual cores

The issue is this function in MPFR.jl

function BigFloat(x::Rational, r::MPFRRoundingMode=ROUNDING_MODE[]; precision::Integer=DEFAULT_PRECISION[])
    setprecision(BigFloat, precision) do
        setrounding_raw(BigFloat, r) do
            BigFloat(numerator(x))::BigFloat / BigFloat(denominator(x))::BigFloat
        end
    end
end

The use of setrounding_raw is not thread safe. In fact we can notice that setprecision is also not used in a thread-safe way. I think this can be fairly easily fixed by calling the mpfr_div function directly, and specifying the rounding mode. I would be happy to prepare a PR for this, but wanted to check if people agree with my analysis first. I can also note that his method is the only use of setprecision and setrounding in MPFR.jl, so this should be the only problematic place.

However, there is also the question of what rounding means in this situation. Currently the rounding is applied on the conversion of the numerator and denominator as well as on the division. This is not necessarily the same as rounding the mathematically exact rational to the nearest representable floating point. I don't think there is a good solution to this in general though.

nsajko commented 9 months ago

PR #49749, that I'm working on, should fix this.