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

invalid ccall signatures in base #2901

Closed
vtjnash opened this issue Apr 20, 2013 · 19 comments
Closed

invalid ccall signatures in base #2901

vtjnash opened this issue Apr 20, 2013 · 19 comments
Labels
bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@vtjnash
Copy link
Member

vtjnash commented Apr 20, 2013

in the files in base, array.jl through expr.jl, there are the following errors:

ccall(:memset, Ptr{T}, (Ptr{T}, Int32, Int32)
ccall((:__gmpf_set_ui, :libgmp)
ccall((:__gmpf_set_si, :libgmp)
ccall((:__gmpf_pow_ui, :libgmp), Void, (Ptr{Void}, Ptr{Void}, Uint),
ccall((:__gmpz_set_si, :libgmp), Void, (Ptr{Void}, Int),
ccall((:__gmpz_set_ui, :libgmp), Void, (Ptr{Void}, Uint),
ccall((:__gmpz_get_si, :libgmp), Int, (Ptr{Void},)
ccall((:__gmpz_get_ui, :libgmp), Uint, (Ptr{Void},),
call((:__gmpz_mul_2exp, :libgmp),
(at this point I'm going to stop listing gmp, since it is almost all invalid)

ccall(:jl_readBuffer,Void,(Ptr{Void},Int32),
(jl_strerror is exported but doesn't seem to exist?)

(todo: fftw.jl through util.jl)

@JeffBezanson
Copy link
Member

I think #2814 is fixing the gmp ones.
Could you please fix these as you find them instead of just listing them in an issue? I honestly don't understand this. All the work here is identifying these cases; pushing a fix is no harder than making this issue.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 20, 2013

#2814 has the same incorrect type signatures I'm recording here. I was they can be fixed in second pass. I'm noting the signature of any ccall that shows up in grep ccall base/* which doesn't match the signature of the corresponding C function

file.jl, float.jl, fs.jl, grisu.jl, inference.jl, intset.jl, iobuffer.jl
@windows_only p = ccall(:_getcwd, Ptr{Uint8}, (Ptr{Uint8}, Uint)
DLLEXPORT char* jl_uv_fs_t_ptr_offset(uv_fs_t* req, int offset)
err = ccall(:uv_fs_close,Int32,(Ptr{Void},Ptr{Void},Int32,Ptr{Uint8},Int32,Int64,Ptr{Void}),
ccall(:bitvector_next, Int64, (Ptr{Uint32}, Uint64, Uint64),
ccall(:memchr,Ptr{Uint8},(Ptr{Uint8},Int32,Int32)

(todo: fftw.jl, io.jl through util.jl)

@JeffBezanson
Copy link
Member

I understand what you're doing, and it's great, I just don't see the need for two passes. You, or somebody else, is going to have to go through all this again to actually fix it. Instead of writing your findings here, write them in the code and commit. Seems much faster to me.

JeffBezanson added a commit that referenced this issue Apr 21, 2013
jl_uv_fs_t_ptr_offset, and memchr, as identified in #2901
@vtjnash
Copy link
Member Author

vtjnash commented Apr 21, 2013

Partly, I opened this to be able to ask questions such as:
--> Should sizeof methods return size_t or int?
(and partly to try to avoid getting bogged down and partly to identify things like issue #2814 where the incorrect code has been copied and partly just because)

@vtjnash
Copy link
Member Author

vtjnash commented Apr 21, 2013

jl_sizeof*, io, iobuffer, libc, loading, mmap, multi, path, task, utf8, util, sysimg, string

sys.c:DLLEXPORT int jl_sizeof_off_t(void) { return sizeof(off_t); }
sys.c:DLLEXPORT int jl_sizeof_ios_t(void) { return sizeof(ios_t); }
sys.c:DLLEXPORT int jl_sizeof_uv_fs_t(void) { return sizeof(uv_fs_t); }
sys.c:DLLEXPORT int jl_sizeof_stat(void) { return sizeof(uv_statbuf_t); }
~~ccall(:jl_ios_fd, Int, (Ptr{Void},), ~~
ccall(:ios_fd, Void, (Ptr{Uint8}, Int, Int32, Int32),
ccall(:ios_write, Int, (Ptr{Void}, Ptr{Void}, Uint), (Uint?)
ccall(:jl_takebuf_string, ByteString, (Ptr{Void},), s.ios) (return type should be Any)
ccall(:jl_takebuf_array, Vector{Uint8}, (Ptr{Void},), s.ios) (ibid)
call(:jl_readuntil, Array{Uint8,1}, (Ptr{Void}, Uint8), (ibid)
ccall(:memcpy, Void, (Ptr{Void}, Ptr{Void}, Int) (Csize_t / Uint?)
ccall(:memmove, Void, (Ptr{Void},Ptr{Void},Int),
ccall(:malloc, Ptr{Void}, (Int,) (Csize_t / Uint?)
ccall(:pwrite, Int, (Int32, Ptr{Void}, Int, FileOffset),
ccall(:u8_strlen, Int, (Ptr{Uint8},), (Csize_t / Uint?)
ccall(:SC_CLK_TCK, Int, ())
ccall(:jl_cstr_to_string, ByteString, (Ptr{Uint8},) (return type should be Any)
ccall(:jl_pchar_to_string, ByteString, (Ptr{Uint8},Int), (ibid)
ccall(:u8_strwidth, Int, (Ptr{Uint8},) (Csize_t / Uint?)

(todo: fftw, librandom, math, pcre, pointer, printf, process, reflection, repl, serialize, show, socket, stat, stream)

@andrioni
Copy link
Member

I'm back. Is this about using Clong and Culong? If so, I hope d3e65cb2f568e06654ad65e0b9cff5ab1966f921 was enough to solve it in bigint.jl.

@JeffBezanson
Copy link
Member

The sizeof functions all return very small integers so it doesn't really matter.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 21, 2013

@JeffBezanson should we at least be consistent, or is it unimportant? I realize sizeof will pretty much always return small integers, but it is declared size_t in the standard.

@andrioni that appears to be it. thanks.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 22, 2013

fftw, librandom, math, pcre, pointer, printf, process, reflection, repl, serialize, show, socket, stream, stat
(I didn't believe fftw needed to be checked -- from the julia code it looks like it only takes simple int and pointer arguments)
ccall(:jl_ptr_to_array, Array{T,N}, (Any, Ptr{T}, Any, Int32), (return type should be Any)
ccall(:jl_sizeof_uv_process_t,Int64,(),

    error = ccall(:jl_spawn, Int32,
        (Ptr{Uint8}, Ptr{Ptr{Uint8}}, Ptr{Void}, Ptr{Void}, Any, Int32,
         Ptr{Void},    Int32,       Ptr{Void},     Int32,       Ptr{Void},
         Int32),
         cmd,        argv,            loop,      proc,      pp,  uvtype(in),
         uvhandle(in), uvtype(out), uvhandle(out), uvtype(err), uvhandle(err),
         pp.cmd.detach)

(uvhandle and argv should be implicit conversions -- for improved gc)
ccall(:jl_module_names, Array{Symbol,1}, (Any,Int32), (return type should be Any)
ccall(:jl_lookup_code_address, Any, (Ptr{Void}, Bool) (Bool should be Int32)
ccall(:jl_sockaddr_is_ip4,Int,(Ptr{Void},)
jl_sockaddr_host6 (this is already has a separate issue)
ccall(:jl_init_pipe, Ptr{Void}, (Ptr{Void},Bool,Bool,Any), (Bool should be Int32)
DLLEXPORT int16_t jl_start_reading(uv_stream_t *handle) (how did that end up int16_t???)
ccall(:uv_read_stop,Bool,(Ptr{Void},) (Bool should be Int32)
DLLEXPORT int jl_pututf8(uv_stream_t *s, uint32_t wchar ) (possibly incorrect given #2907)
call(:jl_write, Int, (Ptr{Void}, Ptr{Void}, Uint32) (two errors)
call(:jl_write, Int, (Ptr{Void}, Ptr{Void}, Uint)
ccall(:jl_stat_size, Int, (Ptr{Uint8},) (return type should be FileOffset)
stat(fd::Integer) = @stat_call jl_fstat fd (type of first arg is wrong)

(all jl_sizeof* functions need to be paired, since some aren't, even when i didn't mark them here)
(todo: linalg/*)

@JeffBezanson
Copy link
Member

Unfortunately it is really nice to have the "easter egg" of allowing Array{T,N} to be a ccall return type, since this allows a type check to be skipped. I know this is kind of awful, and not a general solution.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 22, 2013

I'm fine with it as long as it works. I was just concerned it could get messed up, since it could be a concrete leaf type, which would be copied by-value when returning from the ccall.

@vtjnash
Copy link
Member Author

vtjnash commented Apr 22, 2013

all of these need to be converted to using Cwchar_t instead of Char:
ccall(:wcwidth, Int32, (Char,),

for name = ("alnum", "alpha", "cntrl", "digit", "graph",
"lower", "print", "punct", "space", "upper")
f = symbol(string("is",name))
@eval ($f)(c::Char) = bool(ccall($(string("isw",name)), Int32, (Char,), c))
end

ccall(:towupper, Char, (Char,)
ccall(:towlower, Char, (Char,)

(should FileOffset be renamed Coff_t)?

@vtjnash
Copy link
Member Author

vtjnash commented Apr 27, 2013

extern size_t jl_cholmod_common_size(size_t x) (what is that parameter there for?)
(I didn't check linalg very carefully, but it all seemed to use BlasInt so it should be OK. This concludes my survey of ccall in base)

@vtjnash
Copy link
Member Author

vtjnash commented Apr 27, 2013

Complete summary of remaining issues from above is reproduced below:

@windows_only p = ccall(:_getcwd, Ptr{Uint8}, (Ptr{Uint8}, Uint)
err = ccall(:uv_fs_close,Int32,(Ptr{Void},Ptr{Void},Int32,Ptr{Uint8},Int32,Int64,Ptr{Void}),
ccall(:bitvector_next, Int64, (Ptr{Uint32}, Uint64, Uint64),
ccall(:ios_write, Int, (Ptr{Void}, Ptr{Void}, Uint), (Uint?)
ccall(:jl_takebuf_string, ByteString, (Ptr{Void},), s.ios) (return type should be Any)
ccall(:jl_takebuf_array, Vector{Uint8}, (Ptr{Void},), s.ios) (ibid)
call(:jl_readuntil, Array{Uint8,1}, (Ptr{Void}, Uint8), (ibid)
ccall(:memcpy, Void, (Ptr{Void}, Ptr{Void}, Int) (Csize_t / Uint?)
ccall(:memmove, Void, (Ptr{Void},Ptr{Void},Int),
ccall(:malloc, Ptr{Void}, (Int,) (Csize_t / Uint?)
ccall(:pwrite, Int, (Int32, Ptr{Void}, Int, FileOffset),
ccall(:u8_strlen, Int, (Ptr{Uint8},), (Csize_t / Uint?)
ccall(:jl_cstr_to_string, ByteString, (Ptr{Uint8},) (return type should be Any)
ccall(:jl_pchar_to_string, ByteString, (Ptr{Uint8},Int), (ibid)
ccall(:u8_strwidth, Int, (Ptr{Uint8},) (Csize_t / Uint?)
ccall(:jl_ptr_to_array, Array{T,N}, (Any, Ptr{T}, Any, Int32), (return type should be Any)
ccall(:jl_sizeof_uv_process_t,Int64,(),
ccall(:jl_module_names, Array{Symbol,1}, (Any,Int32), (return type should be Any)
jl_sockaddr_host6 (this is already has a separate issue)
call(:jl_write, Int, (Ptr{Void}, Ptr{Void}, Uint32) (two errors)
ccall(:jl_stat_size, Int, (Ptr{Uint8},) (return type should be FileOffset)

    error = ccall(:jl_spawn, Int32,
        (Ptr{Uint8}, Ptr{Ptr{Uint8}}, Ptr{Void}, Ptr{Void}, Any, Int32,
         Ptr{Void},    Int32,       Ptr{Void},     Int32,       Ptr{Void},
         Int32),
         cmd,        argv,            loop,      proc,      pp,  uvtype(in),
         uvhandle(in), uvtype(out), uvhandle(out), uvtype(err), uvhandle(err),
         pp.cmd.detach)
(uvhandle and argv should be implicit conversions -- for improved gc)

all jl_sizeof* functions need to be paired, since some aren't, even when i didn't mark them here

(should FileOffset be renamed Coff_t)?

andrioni added a commit to andrioni/julia that referenced this issue May 3, 2013
Now it uses Culong and Clong when needed.
andrioni added a commit to andrioni/julia that referenced this issue May 3, 2013
JeffBezanson added a commit that referenced this issue May 21, 2013
@JeffBezanson
Copy link
Member

Ok all done.

@vtjnash
Copy link
Member Author

vtjnash commented May 21, 2013

err = ccall(:uv_fs_close,Int32,(Ptr{Void},Ptr{Void},Int32,Ptr{Uint8},Int32,Int64,Ptr{Void}),
ccall(:jl_stat_size, Int, (Ptr{Uint8},) (return type should be FileOffset)

(uvhandle and argv should be implicit conversions -- for improved gc)
    error = ccall(:jl_spawn, Int32,
        (Ptr{Uint8}, Ptr{Ptr{Uint8}}, Ptr{Void}, Ptr{Void}, Any, Int32,
         Ptr{Void},    Int32,       Ptr{Void},     Int32,       Ptr{Void},
         Int32),
         cmd,        argv,            loop,      proc,      pp,  uvtype(in),
         uvhandle(in), uvtype(out), uvhandle(out), uvtype(err), uvhandle(err),
         pp.cmd.detach)

(should FileOffset be renamed Coff_t)?
(double checkout that all jl_sizeof* methods are paired)

@vtjnash vtjnash reopened this May 21, 2013
@JeffBezanson
Copy link
Member

The uv_fs_close call is fixed; AFAICT it was supposed to be a uv_fs_write call.
I checked all the jl_sizeof methods. I don't think the jl_spawn call needs to be changed.

@vtjnash
Copy link
Member Author

vtjnash commented May 21, 2013

I thought that jl_spawn wasn't providing proper gc-rooting for its arguments (especially argv could be fixed).

@JeffBezanson
Copy link
Member

It's rooted in the caller. The risky thing there is _jl_pre_exec, which probably works as it is, but if it didn't it wouldn't be the fault of the ccall to jl_spawn.

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
Projects
None yet
Development

No branches or pull requests

3 participants