Missing aliasing detection/handling

[martinholters]

Disclaimer: My interest in this has only been triggered by MeshArrays showing up as one of the packages that would be broken by JuliaLang/julia#26237. I have not used MeshArrays myself for anything and don't know my way around the package.

Given

grid = GridOfOnes("CubeSphere",2,2)[1]
data = [rand(Int, 2,2) for i in 1:2, j in 1:2]
A = MeshArrays.gcmarray(grid, data)
B = MeshArrays.gcmarray(grid, data)

we get

julia> Base.mightalias(A, B)
false

although clearly, A and B do alias as they share the same underlying data. This can result in dot-assigns failing to unalias their arguments, as in:

expected = A .+ B'
A .+= B' # should work on a copy of B', but fails to do so because mightalias(A, B') == false
A == expected # false

This fix would consist in implementing Base.dataids(::gcmarray) such that dataids(A) and dataids(B) share at least one common entry.

Now the change to the fallback dataids in JuliaLang/julia#26237 effectively defines

Base.dataids(A::MeshArrays.gcmarray) = (Base.dataids(A.f)..., Base.dataids(A.fSize)..., Base.dataids(A.fIndex)...)

And with that:

julia> Base.mightalias(A, B)
true

Hooray! But...

julia> A .+= B'
ERROR: StackOverflowError:
Stacktrace:
     [1] copymutable(a::MeshArrays.gcmarray{Int64,2,Matrix{Int64}})
       @ Base ./abstractarray.jl:1030
     [2] copy(a::MeshArrays.gcmarray{Int64,2,Matrix{Int64}})
       @ Base ./abstractarray.jl:974
     [3] unaliascopy(A::MeshArrays.gcmarray{Int64,2,Matrix{Int64}})
       @ Base ./abstractarray.jl:1315
     [4] unalias(dest::MeshArrays.gcmarray{Int64,2,Matrix{Int64}}, A::MeshArrays.gcmarray{Int64,2,Matrix{Int64}})
       @ Base ./abstractarray.jl:1298
     [5] copyto!(dest::MeshArrays.gcmarray{Int64,2,Matrix{Int64}}, src::MeshArrays.gcmarray{Int64,2,Matrix{Int64}})
       @ Base ./abstractarray.jl:898
--- the last 5 lines are repeated 3683 more times ---
# ...

and this is the problem detected in JuliaLang/julia#26237.
What happens is the following: upon detection of the aliasing, the broadcast machinery calls unaliascopy, which falls back to copy, which falls back to copymutable, which falls back to copyto!(similar(A), A). But

julia> Base.mightalias(A, similar(A))
true

This in turn makes copyto! call unaliascopy (via unalias) again, resulting in the stack overflow.

There are a few relatively simple options to break this vicious circle, I'm just not sure which is the right one. The first question to answer is: What should be inside the dataids? I think the rule of thumb is that if setindex!(a, ...) can alter the result of getindex(b, ...), then dataids(a) and dataids(b) should share at least one common entry. setindex!(A::gcmarray, ...) only changes A.f, but getindex(A::gcmarray, ...) also accesses A.fIndex and A.fSize, so I think the extended dataids from above is ok, but correct me if I'm wrong. Then either similar(::gcmarray) should be adjusted to also make copies of fIndex and fSize or unaliascopy or copy should be specilaized; I don't know which is best here. But e.g.

function Base.unaliascopy(A::T) where {T<:MeshArrays.gcmarray} 
    return T(
        A.grid,
        A.meta,
        Base.unaliascopy(A.f),
        Base.unaliascopy(A.fSize),
        Base.unaliascopy(A.fIndex),
        A.version,
    )
end

would fix the problem.

[gaelforget]

Thanks @martinholters for reaching out about this, the context, & the potential fix. Must admit I am not really familiar with Base.dataids. Will take a look asap

[martinholters]

Any news?

[gaelforget]

Hi Martin

Thanks for the reminder!

Sorry but I have not gotten around to this yet. It's crazy how fast one month evaporates these days! The new academic semester kicking in at MIT also did seriously stress my organization. Am putting this in my calendar for next week in case I don't get to it this week ...

[martinholters]

It's crazy how fast one month evaporates these days!

Indeed it is...

[gaelforget]

Not sure I understand all of the subtleties in this but I think #54 implements one of the fixes you had suggested.

I almost added the Base.unaliascopy in the PR too because I could see why not but then thought that maybe I am overlooking something else.

Would you advise that I add the Base.unaliascopy specialization you wrote on top of the changes already made to similar in #54 ?

ps. sorry for the lag. I see that JuliaLang/julia#26237 is still open so maybe I am not too late

[martinholters]

I think the change to similar is sufficient. I'll call this fixed and will report back in case I stumble upon any problems with this.