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

RFC: ccall using clang (structs 2) #2236

Closed
wants to merge 2 commits into from
Closed

RFC: ccall using clang (structs 2) #2236

wants to merge 2 commits into from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 8, 2013

This supports all struct types for ccall. I know it needs a rebase before merging. Note that the required clang headers are not typically installed (and clang is not available at all in Travis so that fails).

@JeffBezanson

@StefanKarpinski
Copy link
Member

Is the plan to merge this before the v0.1 deadline or not? I'm not quite comfortable with the timeframe for such a big change, but we do have two outstanding issues that are related to this (#2177, #2154).

@andreasnoack
Copy link
Member

#2154 is not really a problem for v0.1. I've changed complex dot to use the Julia implementation and hence there is no error anymore. The error from #2177 is slightly more problematic because the complex Faddeeva functions used a C wrapper which would be annoying to reintroduce temporarily until complex return works.

@JeffBezanson
Copy link
Member

Very impressive!! Jameson you are a beast.
This is too disruptive for 0.1. We should just disable the problematic complex versions of the Faddeeva functions for 0.1, so there isn't a segfaulting function lying around.

@StefanKarpinski
Copy link
Member

Very impressive!! Jameson you are a beast.
This is too disruptive for 0.1. We should just disable the problematic complex versions of the Faddeeva functions for 0.1, so there isn't a segfaulting function lying around.

Yes! This is great. We'll merge it immediately after the 0.1 branch is forked.

@vtjnash
Copy link
Member Author

vtjnash commented Feb 8, 2013

We might want to just revert to using the Faddeeva wrapper then for 0.1 and make ccall with Structs an error (and fix the null pointer bug in alloc.c)

I wanted to get this done so that @sebastien-villemot could comment on how to package clang for this (since we need to compile against some internal clang headers)

@JeffBezanson
Copy link
Member

Such packaging issues are exactly why we won't aim to put this in 0.1. Those kinds of things are much more problematic than a few thousand lines of code :)

If there are bug fixes here that apply to what's already in master, let's pull them over.

Reverting to using the Faddeeva wrapper seems annoying, and these functions aren't crucial.

@ViralBShah
Copy link
Member

I would much rather revert to Faddeeva wrapper, and not ship code that we know will segfault. Alternatively, we can disable this functionality altogether for 0.1.

It can easily be got rid of after 0.1.

@andreasnoack
Copy link
Member

The complex Faddeeva functions have been disabled for 32 bit systems. Hence no segfaults.

@ViralBShah
Copy link
Member

I did notice WORD_SIZE==64 check in math.jl. This means that the Faddeeva functions are only available on 64-bit until ccall is fixed.

@JeffBezanson
Copy link
Member

I did that. I think it's a good compromise.

@ViralBShah
Copy link
Member

Yes, I think it is a good compromise.

@stevengj
Copy link
Member

In addition to struct arguments and return values, it would be good to support accessing fields of pointers to structs (either as return values or converted from Ptr{Void}). Presumably this would use much of the same machinery.

(I'm running into this in the NumPy C API, in which a lot of functions are actually macros to access fields from a struct pointer, which makes it challenging to call from pure Julia.)

@vtjnash
Copy link
Member Author

vtjnash commented Feb 17, 2013

clang isn't necessarily required to support that, although it does have the potential benefit of parsing header files for you

I still need to add this to the documentation, but unsafe_ref will copy data from a Ptr{T} to a T object. I also still want to extend it to support the syntax unsafe_ref( struct_ptr, :field_name ), equivalent to unsafe_ref(struct_ptr).field_name
unsafe_assign(ptr, value, index) does the reverse operation (currently only supported for BitsKinds)

… struct return (disabled). unfortunately this breaks ComplexPair{Int} (ComplexPair{Float64} seems OK)""

This reverts commit 4fa9364.
@vtjnash
Copy link
Member Author

vtjnash commented Feb 18, 2013

remaining known issues:

  • define NDEBUG correctly (currently assumes llvm --enable-assertions)
  • documentation

@vtjnash
Copy link
Member Author

vtjnash commented Mar 7, 2013

./usr/bin/llvm-config.exe --cflags includes NDEBUG if its needed

@Keno
Copy link
Member

Keno commented Mar 12, 2013

@vtjnash What's the status of this?

@JeffBezanson
Copy link
Member

I think clang is just too large a dependency. But if we want to include it, we can get things like parsing header files, and automatically handling packages that come with C code. It's a big decision.

@Keno
Copy link
Member

Keno commented Mar 12, 2013

And calling C++

@vtjnash
Copy link
Member Author

vtjnash commented Mar 13, 2013

I'm waiting for Jeff to decide he is OK with it, then I'll rebase and update to handle immutable types. Although adding clang is a bit much for just call-by-value structs, it has some nice benefits posible if it is installed (like CIndex https://github.com/ihnorton/Clang.jl)

@stevengj
Copy link
Member

It seems like this would hugely simplify distribution of external packages that wrap C or C++ code, which is an enormous headache right now because you can't rely on Mac or Windows systems having a compiler.

@StefanKarpinski
Copy link
Member

You also can't rely on them having header files, so it only helps so much, but yes, it would be better.

@stevengj
Copy link
Member

@StefanKarpinski, good point. Without libc header files a compiler is basically useless for Pkg dependencies.

@vtjnash vtjnash closed this Jun 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants