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 with ccall to :memcpy #31073

Closed
timholy opened this issue Feb 14, 2019 · 6 comments · Fixed by #31464
Closed

Illegal instruction with ccall to :memcpy #31073

timholy opened this issue Feb 14, 2019 · 6 comments · Fixed by #31464
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior compiler:codegen Generation of LLVM IR and native code regression Regression in behavior compared to a previous version

Comments

@timholy
Copy link
Member

timholy commented Feb 14, 2019

Here's a bug we're struggling with over at JuliaInterpreter. We've recently set up a test harness to run Julia's own tests under the interpreter (JuliaDebug/JuliaInterpreter.jl#13). Several of the tests are marked with ☠️, which we're using to indicate that running that test kills Julia. For one particular case we have a MWE in JuliaDebug/JuliaInterpreter.jl#28.

This particular bug can be triggered even without JuliaInterpreter:

julia> a, b = ['0'], ['a'];

julia> arr = Vector{Char}(undef, 2)
2-element Array{Char,1}:
 '\x74\xd8\x61\xf0'
 '\x00\x00\x7f\x60'

julia> ptr = pointer(arr)
Ptr{Char} @0x00007f6075562330

julia> elsz = sizeof(Char)
4

julia> na = length(a)
1

julia> nba = na * elsz
4

julia> ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt), arr, a, nba)
Unreachable reached at 0x7f606c259cae

signal (4): Illegal instruction
in expression starting at REPL[7]:1
top-level scope at ./REPL[7]:1
jl_fptr_args at /home/tim/src/julia-master/src/gf.c:1906
jl_fptr_trampoline at /home/tim/src/julia-master/src/gf.c:1896
jl_toplevel_eval_flex at /home/tim/src/julia-master/src/toplevel.c:791
jl_toplevel_eval_flex at /home/tim/src/julia-master/src/toplevel.c:746
jl_toplevel_eval at /home/tim/src/julia-master/src/toplevel.c:806
jl_toplevel_eval_in at /home/tim/src/julia-master/src/toplevel.c:826
eval at ./boot.jl:328
jl_fptr_args at /home/tim/src/julia-master/src/gf.c:1906
jl_apply_generic at /home/tim/src/julia-master/src/gf.c:2251
eval_user_input at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.2/REPL/src/REPL.jl:86
run_backend at /home/tim/.julia/dev/Revise/src/Revise.jl:773
#61 at ./task.jl:261
jl_fptr_args at /home/tim/src/julia-master/src/gf.c:1906
jl_fptr_trampoline at /home/tim/src/julia-master/src/gf.c:1896
jl_apply_generic at /home/tim/src/julia-master/src/gf.c:2251
jl_apply at /home/tim/src/julia-master/src/julia.h:1578
start_task at /home/tim/src/julia-master/src/task.c:572
unknown function (ip: 0xffffffffffffffff)
Allocations: 9313301 (Pool: 9311285; Big: 2016); GC: 20
/home/tim/bin/julia-master: line 2: 14664 Illegal instruction     (core dumped) ~/src/julia-master/julia "$@"
@vtjnash
Copy link
Member

vtjnash commented Feb 14, 2019

Cute, I guess LLVM doesn't like that #28707 implemented the wrong return value:

julia> a, b = ['0'], ['a'];; arr = Vector{Char}(undef, 2); ptr = pointer(arr); elsz = sizeof(Char); na = length(a); nba = na * elsz;

julia> f() = ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt), arr, a, nba)

julia> @code_llvm f()
...
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %57, i8* %54, i64 %value_phi, i32 1, i1 false)
  call void @llvm.trap()
...

@vtjnash vtjnash added bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version compiler:codegen Generation of LLVM IR and native code labels Feb 14, 2019
@timholy
Copy link
Member Author

timholy commented Feb 15, 2019

This seems likely to be an easy fix. But where does one find doxygen docs for llvm-6? I can only find version 9 online.

@timholy
Copy link
Member Author

timholy commented Feb 17, 2019

I tried changing that to return ghostValue(jl_voidpointer_type); and got this:

...
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %44, i8* %42, i64 %value_phi, i32 1, i1 false)
  %45 = getelementptr %jl_value_t addrspace(10)*, %jl_value_t addrspace(10)** %gcframe, i32 1
  %46 = load %jl_value_t addrspace(10)*, %jl_value_t addrspace(10)** %45
  %47 = getelementptr %jl_value_t**, %jl_value_t*** %ptls, i32 0
  %48 = bitcast %jl_value_t*** %47 to %jl_value_t addrspace(10)**
  store %jl_value_t addrspace(10)* %46, %jl_value_t addrspace(10)** %48
  ret i64 undef
}

However,

julia> f()
Ptr{Nothing} @0x00007f2a1724d2b0

julia> ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, UInt), arr, a, nba)

signal (11): Segmentation fault
in expression starting at REPL[5]:1
init_bits_value at /home/tim/src/julia-master/src/cgutils.cpp:1905
init_bits_cgval at /home/tim/src/julia-master/src/cgutils.cpp:1918 [inlined]
boxed at /home/tim/src/julia-master/src/cgutils.cpp:2242
emit_function at /home/tim/src/julia-master/src/codegen.cpp:6156
jl_compile_linfo at /home/tim/src/julia-master/src/codegen.cpp:1145
jl_fptr_trampoline at /home/tim/src/julia-master/src/gf.c:1861
jl_toplevel_eval_flex at /home/tim/src/julia-master/src/toplevel.c:791
jl_toplevel_eval_flex at /home/tim/src/julia-master/src/toplevel.c:746
jl_toplevel_eval_in at /home/tim/src/julia-master/src/toplevel.c:826
eval at ./boot.jl:327
jl_apply_generic at /home/tim/src/julia-master/src/gf.c:2251
eval_user_input at /home/tim/src/julia-master/usr/share/julia/stdlib/v1.2/REPL/src/REPL.jl:86
run_backend at /home/tim/.julia/dev/Revise/src/Revise.jl:773
#61 at ./task.jl:261
jl_fptr_trampoline at /home/tim/src/julia-master/src/gf.c:1896
jl_apply_generic at /home/tim/src/julia-master/src/gf.c:2251
jl_apply at /home/tim/src/julia-master/src/julia.h:1593 [inlined]
start_task at /home/tim/src/julia-master/src/task.c:572
unknown function (ip: 0xffffffffffffffff)
Allocations: 9084779 (Pool: 9082932; Big: 1847); GC: 20
Segmentation fault (core dumped)

@vtjnash
Copy link
Member

vtjnash commented Feb 17, 2019

jl_voidpointer_type isn't a ghost object (such as jl_void_type), so the compiler's going to be rather confused there. it should return the first argument

@timholy
Copy link
Member Author

timholy commented Feb 17, 2019

My flailing has gotten me here:

diff --git a/src/ccall.cpp b/src/ccall.cpp
index 657d5ee1b7..0340705416 100644
--- a/src/ccall.cpp
+++ b/src/ccall.cpp
@@ -1781,15 +1781,15 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs)
         const jl_cgval_t &dst = argv[0];
         const jl_cgval_t &src = argv[1];
         const jl_cgval_t &n = argv[2];
+        Value *destp = emit_unbox(ctx, T_size, dst, (jl_value_t*)jl_voidpointer_type);
         ctx.builder.CreateMemCpy(
-                ctx.builder.CreateIntToPtr(
-                    emit_unbox(ctx, T_size, dst, (jl_value_t*)jl_voidpointer_type), T_pint8),
+                ctx.builder.CreateIntToPtr(destp, T_pint8),
                 ctx.builder.CreateIntToPtr(
                     emit_unbox(ctx, T_size, src, (jl_value_t*)jl_voidpointer_type), T_pint8),
                 emit_unbox(ctx, T_size, n, (jl_value_t*)jl_ulong_type), 1,
                 false);
         JL_GC_POP();
-        return ghostValue(jl_void_type);
+        return mark_or_box_ccall_result(ctx, destp, retboxed, rt, unionall, static_rt);
     }
 
     jl_cgval_t retval = sig.emit_a_ccall(

or here

$ git diff
diff --git a/src/ccall.cpp b/src/ccall.cpp
index 657d5ee1b7..3a2a5da548 100644
--- a/src/ccall.cpp
+++ b/src/ccall.cpp
@@ -1781,7 +1781,7 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs)
         const jl_cgval_t &dst = argv[0];
         const jl_cgval_t &src = argv[1];
         const jl_cgval_t &n = argv[2];
-        ctx.builder.CreateMemCpy(
+        CallInst *ret = ctx.builder.CreateMemCpy(
                 ctx.builder.CreateIntToPtr(
                     emit_unbox(ctx, T_size, dst, (jl_value_t*)jl_voidpointer_type), T_pint8),
                 ctx.builder.CreateIntToPtr(
@@ -1789,7 +1789,7 @@ static jl_cgval_t emit_ccall(jl_codectx_t &ctx, jl_value_t **args, size_t nargs)
                 emit_unbox(ctx, T_size, n, (jl_value_t*)jl_ulong_type), 1,
                 false);
         JL_GC_POP();
-        return ghostValue(jl_void_type);
+        return mark_or_box_ccall_result(ctx, ret, retboxed, rt, unionall, static_rt);
     }
 
     jl_cgval_t retval = sig.emit_a_ccall(

but neither works. Any more concrete tips? As should be apparent, I don't know what I'm doing.

@timholy
Copy link
Member Author

timholy commented Mar 8, 2019

I'm sufficiently swamped (and behind on other tasks due to prioritizing the debugger) that I don't think I'll get to this soon. @Keno, can I assign this to you? You obviously have the relevant expertise, whereas I do not.

Keno added a commit that referenced this issue Mar 24, 2019
POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
`void`. Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073
Keno added a commit that referenced this issue Mar 24, 2019
POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
`void`. Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073
JeffBezanson pushed a commit that referenced this issue May 9, 2019
POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
`void`. Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073
Keno added a commit that referenced this issue May 10, 2019
POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
`void`. Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073
KristofferC pushed a commit that referenced this issue May 13, 2019
POSIX specifies that memcpy returns its first argument, which is
a bit of a useless feature (e.g. the llvm intrinsic just returns
`void`. Nevertheless, since we're intercepting the ccall here, we
should allow it. For convenience, still allow the Cvoid return though.

Fixes #31073

(cherry picked from commit 8d2727b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler:codegen Generation of LLVM IR and native code regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants