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

whether Core.checked_dims decides to throw depends on dimension order (for empty arrays) #54244

Closed
nsajko opened this issue Apr 25, 2024 · 2 comments · Fixed by #54255
Closed
Labels
domain:arrays [a, r, r, a, y, s] kind:bug Indicates an unexpected problem or unintended behavior kind:regression Regression in behavior compared to a previous version

Comments

@nsajko
Copy link
Contributor

nsajko commented Apr 25, 2024

julia> b = typemax(Int) - 1
9223372036854775806

julia> Array{Int}(undef, b, b, 0)
ERROR: ArgumentError: invalid Array dimensions
Stacktrace:
 [1] checked_dims
   @ ./boot.jl:585 [inlined]
 [2] Array
   @ ./boot.jl:598 [inlined]
 [3] (Array{Int64})(::UndefInitializer, m::Int64, n::Int64, o::Int64)
   @ Core ./boot.jl:611
 [4] top-level scope
   @ REPL[2]:1

julia> Array{Int}(undef, 0, b, b)
0×9223372036854775806×9223372036854775806 Array{Int64, 3}

julia> versioninfo()
Julia Version 1.12.0-DEV.387
Commit b5bfd83a3d0 (2024-04-22 13:12 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × AMD Ryzen 3 5300U with Radeon Graphics
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver2)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)

Two alternative solutions:

  1. When there's a zero among the dimension sizes, only check that the nonzero sizes are nonnegative and less than typemax(Int). Don't check for overflow during multiplication. This would make both examples above return.

  2. Remove the zero sizes before checking for overflow. This would make both examples above throw.

@nsajko nsajko added kind:bug Indicates an unexpected problem or unintended behavior domain:arrays [a, r, r, a, y, s] needs decision A decision on this change is needed labels Apr 25, 2024
@nsajko
Copy link
Contributor Author

nsajko commented Apr 25, 2024

Affected code:

julia/base/boot.jl

Lines 553 to 587 in 8f6418e

# checked-multiply intrinsic function for dimensions
_checked_mul_dims() = 1, false
_checked_mul_dims(m::Int) = m, Intrinsics.ule_int(typemax_Int, m) # equivalently: (m + 1) < 1
function _checked_mul_dims(m::Int, n::Int)
b = Intrinsics.checked_smul_int(m, n)
a = getfield(b, 1)
ovflw = getfield(b, 2)
ovflw = Intrinsics.or_int(ovflw, Intrinsics.ule_int(typemax_Int, m))
ovflw = Intrinsics.or_int(ovflw, Intrinsics.ule_int(typemax_Int, n))
return a, ovflw
end
function _checked_mul_dims(m::Int, d::Int...)
@_foldable_meta # the compiler needs to know this loop terminates
a = m
i = 1
ovflw = false
while Intrinsics.sle_int(i, nfields(d))
di = getfield(d, i)
b = Intrinsics.checked_smul_int(a, di)
ovflw = Intrinsics.or_int(ovflw, getfield(b, 2))
ovflw = Intrinsics.or_int(ovflw, Intrinsics.ule_int(typemax_Int, di))
a = getfield(b, 1)
i = Intrinsics.add_int(i, 1)
end
return a, ovflw
end
# convert a set of dims to a length, with overflow checking
checked_dims() = 1
checked_dims(m::Int) = m # defer this check to Memory constructor instead
function checked_dims(d::Int...)
b = _checked_mul_dims(d...)
getfield(b, 2) && throw(ArgumentError("invalid Array dimensions"))
return getfield(b, 1)
end

@oscardssmith oscardssmith added kind:regression Regression in behavior compared to a previous version and removed needs decision A decision on this change is needed labels Apr 25, 2024
@oscardssmith
Copy link
Member

Given that 1.10 allowed both, I think this is a regression. Fix incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] kind:bug Indicates an unexpected problem or unintended behavior kind:regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants