Incorrect use of reinterpret

[yuyichao]

The corrent code contains invalid use of reinterpret to cast an array of less alignment to an array of larger alignment causing test failure after JuliaLang/julia#21831

reinterpret: Error During Test
  Test threw an exception of type ArgumentError
  Expression: size(reinterpret(UInt32, rand(BGRA{N0f8}, 5, 5))) == (5, 5)
  ArgumentError: reinterpret from alignment 1 to alignment 4 not allowed
  Stacktrace:
   [1] reinterpret(::Type{UInt32}, ::Array{ColorTypes.BGRA{FixedPointNumbers.Normed{UInt8,8}},2}, ::Tuple{Int64,Int64}) at ./array.jl:153
   [2] reinterpret at /home/yuyichao/.julia/v0.7/ImageCore/src/convert_reinterpret.jl:28 [inlined]
   [3] macro expansion at /home/yuyichao/projects/contrib/ImageCore.jl/test/convert_reinterpret.jl:129 [inlined]
   [4] macro expansion at ./test.jl:856 [inlined]
   [5] anonymous at ./<missing>:?
reinterpret: Error During Test
  Test threw an exception of type ArgumentError
  Expression: reinterpret(UInt32, a) == UInt32[0xf0884422, 0x04030201]
  ArgumentError: reinterpret from alignment 1 to alignment 4 not allowed
  Stacktrace:
   [1] reinterpret(::Type{UInt32}, ::Array{ColorTypes.BGRA{FixedPointNumbers.Normed{UInt8,8}},1}, ::Tuple{Int64}) at ./array.jl:153
   [2] reinterpret(::Type{UInt32}, ::Array{ColorTypes.BGRA{FixedPointNumbers.Normed{UInt8,8}},1}) at /home/yuyichao/.julia/v0.7/ImageCore/src/convert_reinterpret.jl:24
   [3] macro expansion at /home/yuyichao/projects/contrib/ImageCore.jl/test/convert_reinterpret.jl:133 [inlined]
   [4] macro expansion at ./test.jl:856 [inlined]
   [5] anonymous at ./<missing>:?
Test Summary: | Pass  Error  Total
reinterpret   |  125      2    127
ERROR: LoadError: LoadError: Some tests did not pass: 125 passed, 0 failed, 2 errored, 0 broken.
while loading /home/yuyichao/projects/contrib/ImageCore.jl/test/convert_reinterpret.jl, in expression starting on line 4
while loading /home/yuyichao/projects/contrib/ImageCore.jl/test/runtests.jl, in expression starting on line 11

I'm not sure what is the right fix here. If this is a public interface that should be maintained, the reinterpret needs to be done differently. The load cannot be a getindex(::Array), which must have the right alignment, but needs to be constructed from smaller loads or use unaligned load i.e. unsafe_load(::Ptr). I put a UnalignedVector implementation in CRC.jl that is barely good enough for the private usecase in that package but should demonstrate the right way to deal with the alignment issue. I imagine the Array experts here should be able to implement a better interface/more complete array from there if necessary.

[timholy]

In many ways this is a legacy interface; the UInt32 is really intended for consumption by Cairo, but as I've argued elsewhere I think our Cairo binding should use RGB24 and ARGB32 rather than UInt32.

But I'm a little confused about the specific location of the failure. Why doesn't

reinterpret(BGRA{N0f8}, [0x22, 0x44, 0x88, 0xf0, 0x01, 0x02, 0x03, 0x04])

fail? BGRA{N0f8} is 4 bytes.

[timholy]

It's also worth pointing out that one of the main things ImageCore did was develop the ColorView and ChannelView types that allow you to get reinterpret-like functionality even in cases where reinterpret is not legal. Thus, this isn't going to be a big deal whatsoever.

[yuyichao]

BGRA{N0f8} is 4 bytes.

The size is not the issue, the alignment is. That's why the error is reinterpret from alignment 1 to alignment 4 not allowed instead of reinterpret from eltype size 1 to eltype size 4 not allowed. You can reinterpret anything to NTuple{4,UInt8} without triggering the error.

[timholy]

OK, I just assumed that an NTuple{4,UInt8} would require 4-byte alignment.

I can fix this, so don't sweat it yourself.

[yuyichao]

I can fix this, so don't sweat it yourself.

Cool. Thanks. That's what I assumed which is why this is the only package that I didn't open a PR (JuliaLang/julia#21831 (comment))

[timholy]

Hmm, quick question: is there any reason we can't require

a = Matrix{BGRA{N0f8}}(5,5)

to have an alignment of 4 bytes? If we can, then there's a really easy fix here.

[yuyichao]

Matrix{BGRA{N0f8}}(5, 5) will have larger alignment. In fact it'll probably have 64 or at least 16bytes alignment. However, there are multiple ways to construct a Matrix{BGRA{N0f8}} that doesn't have that alignment (unsafe_wrap or reinterpret) so we either need to disallow that or make the error more conditional.

Disallowing constructing a Matrix{BGRA{N0f8}} with less alignment would not be necessary for the compiler and since it's not required by the ABi, it'll break valid use of reinterpret.

Making the error more conditional is possible but after seeing the users of reinterpret in packages I've confirmed my belief that doing that will just make debugging harder without helping anyone. For users that get the less-aligned array privately (range index of another array for example), they should use other functions for it (I've been using unsafe_load but we could have a better wrapper if needed) which would be faster and well-defined. For users that get the less-aligned array from the user, it'll be better to unconditionally throw the error since otherwise they'll just hit this error at some time when the user passed in an array that isn't well aligned.

[timholy]

Matrix{BGRA{N0f8}}(5, 5) will have larger alignment.

               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: https://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.7.0-DEV.258 (2017-05-19 19:17 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit a72aad4 (0 days old master)
|__/                   |  x86_64-linux-gnu

julia> using Colors, FixedPointNumbers

julia> a = Matrix{BGRA{N0f8}}(5,5)
5×5 Array{ColorTypes.BGRA{FixedPointNumbers.Normed{UInt8,8}},2}:
 BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)
 BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)
 BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)
 BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)
 BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)  BGRA{N0f8}(0.0,0.0,0.0,0.0)

julia> reinterpret(UInt32, a)
ERROR: ArgumentError: reinterpret from alignment 1 bytes to alignment 4 bytes not allowed
Stacktrace:
 [1] reinterpret(::Type{UInt32}, ::Array{ColorTypes.BGRA{FixedPointNumbers.Normed{UInt8,8}},2}, ::Tuple{Int64,Int64}) at ./array.jl:153
 [2] reinterpret(::Type{UInt32}, ::Array{ColorTypes.BGRA{FixedPointNumbers.Normed{UInt8,8}},2}) at ./array.jl:139

[yuyichao]

I mean, the pointer is aligned. The type is not and won't be. And for why the error is raised, see the detail below that sentence.