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

Illegal instruction in collect #25907

Closed
samoconnor opened this issue Feb 6, 2018 · 16 comments · Fixed by #25932
Closed

Illegal instruction in collect #25907

samoconnor opened this issue Feb 6, 2018 · 16 comments · Fixed by #25932
Labels
arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version

Comments

@samoconnor
Copy link
Contributor

   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: https://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.7.0-DEV.3639 (2018-01-29 21:58 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 447ed272d4* (7 days old master)
|__/                   |  x86_64-apple-darwin16.7.0

julia> const unhex = [isxdigit(c) ? tryparse(UInt, "0x$c") : 0 for c in Char(1):Char(256)]
Unreachable reached at 0x10e004398

signal (4): Illegal instruction: 4
in expression starting at no file:0
collect_to! at ./array.jl:583
unknown function (ip: 0x10e004484)
jl_call_fptr_internal at /Users/sam/julia/julia/src/./julia_internal.h:383 [inlined]
jl_call_method_internal at /Users/sam/julia/julia/src/./julia_internal.h:402 [inlined]
jl_apply_generic at /Users/sam/julia/julia/src/gf.c:2089
collect_to! at ./array.jl:585
collect_to_with_first! at ./array.jl:562
unknown function (ip: 0x10e002f34)
jl_call_fptr_internal at /Users/sam/julia/julia/src/./julia_internal.h:383 [inlined]
jl_call_method_internal at /Users/sam/julia/julia/src/./julia_internal.h:402 [inlined]
jl_apply_generic at /Users/sam/julia/julia/src/gf.c:2089
collect at ./array.jl:543
unknown function (ip: 0x10e00110f)
jl_call_fptr_internal at /Users/sam/julia/julia/src/./julia_internal.h:383 [inlined]
jl_call_method_internal at /Users/sam/julia/julia/src/./julia_internal.h:402 [inlined]
jl_apply_generic at /Users/sam/julia/julia/src/gf.c:2089
do_call at /Users/sam/julia/julia/src/interpreter.c:323
eval_body at /Users/sam/julia/julia/src/interpreter.c:509
jl_interpret_toplevel_thunk_callback at /Users/sam/julia/julia/src/interpreter.c:720
unknown function (ip: 0xfffffffffffffffe)
unknown function (ip: 0x11872ad4f)
unknown function (ip: 0xb)
jl_interpret_toplevel_thunk at /Users/sam/julia/julia/src/interpreter.c:729
jl_toplevel_eval_flex at /Users/sam/julia/julia/src/toplevel.c:806
jl_toplevel_eval_in at /Users/sam/julia/julia/src/builtins.c:626
eval at ./boot.jl:295 [inlined]
eval at /Users/sam/julia/julia/usr/share/julia/site/v0.7/REPL/src/REPL.jl:3
jl_call_fptr_internal at /Users/sam/julia/julia/src/./julia_internal.h:383 [inlined]
jl_call_method_internal at /Users/sam/julia/julia/src/./julia_internal.h:402 [inlined]
jl_apply_generic at /Users/sam/julia/julia/src/gf.c:2089
eval_user_input at /Users/sam/julia/julia/usr/share/julia/site/v0.7/REPL/src/REPL.jl:82
jl_call_fptr_internal at /Users/sam/julia/julia/src/./julia_internal.h:383 [inlined]
jl_call_method_internal at /Users/sam/julia/julia/src/./julia_internal.h:402 [inlined]
jl_apply_generic at /Users/sam/julia/julia/src/gf.c:2089
macro expansion at /Users/sam/julia/julia/usr/share/julia/site/v0.7/REPL/src/REPL.jl:113 [inlined]
#29 at ./event.jl:92
jl_call_fptr_internal at /Users/sam/julia/julia/src/./julia_internal.h:383 [inlined]
jl_call_method_internal at /Users/sam/julia/julia/src/./julia_internal.h:402 [inlined]
jl_apply_generic at /Users/sam/julia/julia/src/gf.c:2089
jl_apply at /Users/sam/julia/julia/src/./julia.h:1523 [inlined]
start_task at /Users/sam/julia/julia/src/task.c:268
Allocations: 3154295 (Pool: 3152918; Big: 1377); GC: 6
Illegal instruction: 4
@ararslan ararslan added bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version arrays [a, r, r, a, y, s] labels Feb 6, 2018
@martinholters
Copy link
Member

As a datapoint, this works: [isxdigit(c) ? tryparse(UInt, "0x$c") : UInt(0) for c in Char(1):Char(256)] (note the UInt(0) instead of just 0)

@iblislin
Copy link
Member

iblislin commented Feb 6, 2018

Is crashing caused by type inference stuffs?

julia> @code_warntype f()
Variables:
                                                                                                                                                  
Body:                                                                                                                                             
  begin                                                                                                                                           
      # meta: location REPL[15] f 2                                                                                                               
      # meta: location strings/unicode.jl isxdigit 561                                                                                            
      goto 4                                                                                                                                      
      4:                                                                                                                                          
      5:                                                                                                                                          
      goto 7                                                                                                                                      
      7:                                                                                                                                          
      goto 9                                                                                                                                      
      9:                                                                                                                                          
      10:                                                                                                                                         
      goto 12                                                                                                                                     
      12:                                                                                                                                         
      goto 14                                                                                                                                     
      14:                                                                                                                                         
      # meta: pop location                                                                                                                        
      goto 17                                                                                                                                     
      17:                                                                                                                                         
      # meta: pop location                                                                                                                        
      return 0                                                                                                                                    
  end::Int64                                                                                                                                      
                                                                                                                                                  
julia> function f(c = Char(1))
        isxdigit(c) ? tryparse(UInt, "0x$c") : 0
       end

@martinholters
Copy link
Member

That looks ok, it's the result of constant-propagating the Char(1). @code_warntype f(Char(1)) shows the expected type-unstable function.

@iblislin
Copy link
Member

iblislin commented Feb 6, 2018

No idea why (I just randomly comment out something), but this patch solves this issue.

diff --git a/base/array.jl b/base/array.jl
index 3dbaa5957e..6b0da32d1b 100644
--- a/base/array.jl
+++ b/base/array.jl
@@ -575,7 +575,7 @@ function collect_to!(dest::AbstractArray{T}, itr, offs, st) where T
         el, st = next(itr, st)
         S = typeof(el)
         if S === T || S <: T
-            @inbounds dest[i] = el::T
+            @inbounds dest[i] = el
             i += 1
         else
             R = promote_typejoin(T, S)

@nalimilan
Copy link
Member

FWIW, #25828 doesn't make the crash disappear either (too bad!).

@iblislin
Copy link
Member

iblislin commented Feb 6, 2018

@nalimilan I applied your patch from #25828
Performance is fine, and it did not crash.

diff --git a/base/array.jl b/base/array.jl
index 3dbaa5957e..93111a0c61 100644
--- a/base/array.jl
+++ b/base/array.jl
@@ -574,12 +574,13 @@ function collect_to!(dest::AbstractArray{T}, itr, offs, st) where T
     while !done(itr, st)
         el, st = next(itr, st)
         S = typeof(el)
-        if S === T || S <: T
-            @inbounds dest[i] = el::T
+        if el isa T || typeof(el) === T
+            @inbounds dest[i] = el
             i += 1
         else
             R = promote_typejoin(T, S)

@nalimilan
Copy link
Member

I meant that #25828 on its own doesn't make any difference if you don't remove ::T in addition. But indeed we can also remove that assertion, and in my tests it doesn't make a performance difference either with the PR.

@martinholters
Copy link
Member

I guess this should be all that's needed:

        if el isa T
            @inbounds dest[i] = el

I think inference should be able to figure out that el::T there, so no need for the type-assertion. (What good is the || typeof(el) === T to do, BTW?)

But the code as it is now also should not crash. So might be worthwhile figuring out what's wrong there.

@nalimilan
Copy link
Member

What good is the || typeof(el) === T to do, BTW?

See #25828 (comment).

But we should probably remove the ::T assertion given that it works around the bug and doesn't seem to introduce any performance regression.

@iblislin
Copy link
Member

iblislin commented Feb 7, 2018

I tested el isa T vs el isa T || typeof(el) === T.
Performance is quite different.

@martinholters
Copy link
Member

That's interesting. But most probably unrelated. Meanwhile I've managed to come up with a fairly minimal reproducer for this issue's underlying bug:

julia> @noinline f() = let c=rand(1:2); c == 1 ? 0 : c == 2 ? UInt(0) : nothing; end # @noinline for uncluttered code_warntype output
f (generic function with 1 method)

julia> function foo!(dest::AbstractArray{Integer})
           el = f()
           dest[1] = el::Integer
           return dest
       end
foo! (generic function with 1 method)

julia> @code_warntype foo!(Vector{Integer}(uninitialized, 1))
Variables:
  dest::Array{Integer,1}
  el::Union{Nothing, Int64, UInt64}

Body:
  begin
      el::Union{Nothing, Int64, UInt64} = $(Expr(:invoke, MethodInstance for f(), :(Main.f)))::Union{Nothing, Int64, UInt64}
      #= line 3 =#
      Core.SSAValue(0) = (Core.typeassert)(el::Union{Nothing, Int64, UInt64}, Main.Integer)::Integer
      # meta: location array.jl setindex! 689
      (Base.arrayset)(true, dest::Array{Integer,1}, Core.SSAValue(0), 1)::Array{Integer,1}
      # meta: pop location
      #= line 4 =#
      return dest::Array{Integer,1}
  end::Array{Integer,1}

julia> foo!(Vector{Integer}(uninitialized, 1))
Unreachable reached at 0x7f2d95930f91

signal (4): Illegal instruction
in expression starting at no file:0
foo! at ./REPL[2]:3
jl_call_fptr_internal at /home/hol/Projects/julia-master/src/julia_internal.h:383 [inlined]
jl_call_method_internal at /home/hol/Projects/julia-master/src/julia_internal.h:402 [inlined]
jl_apply_generic at /home/hol/Projects/julia-master/src/gf.c:2089
do_call at /home/hol/Projects/julia-master/src/interpreter.c:323
# ...

Changing f() to be inferred as Union{Int64,Nothing} makes the crash go away, while the output of @code_warntype looks the same, except for the changed Union type, obviously. The output of @code_llvm (which I cannot really interpret), OTOH, looks rather different.

@martinholters
Copy link
Member

Addition: Not restricted to arrays; this crashes, too:

mutable struct Foo; x::Integer; end
@noinline f() = let c=rand(1:2); c == 1 ? 0 : c == 2 ? UInt(0) : nothing; end # @noinline for uncluttered code_warntype output
function foo!(dest)
    el = f()
    dest.x = el::Integer
end
foo!(Foo(1))

@chethega
Copy link
Contributor

chethega commented Feb 7, 2018

Even shorter:

versioninfo()
#Julia Version 0.7.0-DEV.3736
#Commit 4b1cf22f98* (2018-02-06 16:41 UTC)

@noinline f() = let c=rand(1:2); c == 1 ? 0 : c == 2 ? UInt(0) : nothing; end
function g()
       el=f()
       el::Integer
end
g();
#Illegal instruction (core dumped)

@chethega
Copy link
Contributor

chethega commented Feb 7, 2018

A=Union{Int64, UInt64, Nothing}[1];
g(x)=x[1]::Integer
g(A)
#core dumped

edit: same for g(x)=x[1]::Number.

@martinholters
Copy link
Member

A couple of observations:

  • The Union has to contain at least two subtypes of Integer, at least one type that is not a subtype of Integer, and only concrete types.
  • It's important that the type-asserted value is returned (or otherwise used); g(x)=(x[1]::Integer; nothing) does not crash.
  • Setting A[1] to 1.0 makes g throw the expected TypeError, no crash.

@martinholters
Copy link
Member

Bisection points at 88a4db2, cc @vtjnash

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

Successfully merging a pull request may close this issue.

6 participants