3 ms·

Regarding the @inbounds problem. If removing @inbounds leads to out of bounds memory access, doesn't that mean it was masking a bug? Shouldn't it crash with a m

Regarding the @inbounds problem. If removing @inbounds leads to out of bounds memory access, doesn't that mean it was masking a bug? Shouldn't it crash with a message like an assert?

balencpp 3y ago
I think the implication is that @inbounds turns out of bound memory accesses into undefined, silent behavior, which can stay hidden and cause weird bugs later on. Out of bounds without using @inbounds leads to an exception indeed.
jakobnissen 3y ago
What do you mean precisely? Removing @inbounds should never leads to OOB access. The @inbounds macro disables the boundscheck. If what you mean was that: If the act of removing an @inbounds leads to a bounds exception, then the code was buggy even without the macro. That's true. IIUC, the main issue that Yuri talks about is that Julia is a highly generic language where people share abstractions quite a lot, but there is little agreement on what those abstractions are, and the language provide zero tooling or help with that
- phkahler 3y ago
  >> What do you mean precisely? Removing @inbounds should never leads to OOB access. The @inbounds macro disables the boundscheck. Oh, that explains it. I thought it enabled a bounds check that somehow allowed code to run rather than fault. So it is a promise to the compiler that the code is correct and doesn't need checking. Composition with arbitrary bounds silently breaks that promise. Go it.
eigenspace 3y ago
If @inbounds wasn't used, then yes, it would throw an error, but @inbounds is specifically for removing those error paths. What happened here is that julia has a package called OffsetArrays.jl which can be used to create arrays with arbitrary index offsets. Julia arrays start at index 1 (1-based indexing), but with OffsetArrays you could make 0-based arrays, or 10-based arrays or whatever you want. There happened to be a bunch of code laying around which was older than OffsetArrays.jl and did things like function f(v::AbstractVector) for i in 1:length(v) @inbounds do_something(v[i]) end end which implictly assumed that all abstract vectors could be indexed according to normal 1-based indexing rules. This created a lot of silent problems when people then went and fed offset arrays into functions that were using such incorrect indexing schemes. Instead, modern julia code that works on AbstractArray is supposed to be written using functions like eachindex, or axes, i.e. function f(v::AbstractVector) for i in eachindex(v) @inbounds do_something(v[i]) end end though Julia's compiler is now smart enough that using @inbounds like this is typically unnecessary, since it can see that eachindex(v) can't produce an out-of-bounds index.
- renox 3y ago
  Couldn't this kind of things be caught by type checking? 1-indexed vectors should be different type than arbitrary offset vector (implicitly convertible in one way only).
  - eigenspace 3y ago
    They are different types, but they share a common supertype (AbstractArray). AbstractArray includes thing like regular Array, and also a whole zoo of other array types (including OffsetArray). The problematic code was incorrectly saying it worked on any subtype of AbstractArray, but was actually written assuming that the arrays were 1-based which is an invalid assumption.

Regarding the @inbounds problem. If removing @inbounds leads to out of bounds memory access, doesn't that mean it was masking a bug? Shouldn't it crash with a message like an assert?

balencpp 3y ago

I think the implication is that @inbounds turns out of bound memory accesses into undefined, silent behavior, which can stay hidden and cause weird bugs later on. Out of bounds without using @inbounds leads to an exception indeed.

jakobnissen 3y ago

What do you mean precisely? Removing @inbounds should never leads to OOB access. The @inbounds macro disables the boundscheck. If what you mean was that: If the act of removing an @inbounds leads to a bounds exception, then the code was buggy even without the macro. That's true. IIUC, the main issue that Yuri talks about is that Julia is a highly generic language where people share abstractions quite a lot, but there is little agreement on what those abstractions are, and the language provide zero tooling or help with that

phkahler 3y ago
>> What do you mean precisely? Removing @inbounds should never leads to OOB access. The @inbounds macro disables the boundscheck. Oh, that explains it. I thought it enabled a bounds check that somehow allowed code to run rather than fault. So it is a promise to the compiler that the code is correct and doesn't need checking. Composition with arbitrary bounds silently breaks that promise. Go it.

eigenspace 3y ago

If @inbounds wasn't used, then yes, it would throw an error, but @inbounds is specifically for removing those error paths. What happened here is that julia has a package called OffsetArrays.jl which can be used to create arrays with arbitrary index offsets. Julia arrays start at index 1 (1-based indexing), but with OffsetArrays you could make 0-based arrays, or 10-based arrays or whatever you want. There happened to be a bunch of code laying around which was older than OffsetArrays.jl and did things like function f(v::AbstractVector) for i in 1:length(v) @inbounds do_something(v[i]) end end which implictly assumed that all abstract vectors could be indexed according to normal 1-based indexing rules. This created a lot of silent problems when people then went and fed offset arrays into functions that were using such incorrect indexing schemes. Instead, modern julia code that works on AbstractArray is supposed to be written using functions like eachindex, or axes, i.e. function f(v::AbstractVector) for i in eachindex(v) @inbounds do_something(v[i]) end end though Julia's compiler is now smart enough that using @inbounds like this is typically unnecessary, since it can see that eachindex(v) can't produce an out-of-bounds index.

renox 3y ago
Couldn't this kind of things be caught by type checking? 1-indexed vectors should be different type than arbitrary offset vector (implicitly convertible in one way only).
- eigenspace 3y ago
  They are different types, but they share a common supertype (AbstractArray). AbstractArray includes thing like regular Array, and also a whole zoo of other array types (including OffsetArray). The problematic code was incorrectly saying it worked on any subtype of AbstractArray, but was actually written assuming that the arrays were 1-based which is an invalid assumption.