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

Ccallmacro #32748

Merged
merged 2 commits into from
Feb 28, 2020
Merged

Ccallmacro #32748

merged 2 commits into from
Feb 28, 2020

Conversation

ninjaaron
Copy link
Contributor

@ninjaaron ninjaaron commented Jul 31, 2019

I wrote a macro quite a while back for wrapping ccall in more "julian" syntax and posted it in this thread on discourse.

People liked it and there was some suggestion it should be in Base, and @StefanKarpinski chimed in that he'd proposed exactly this macro in the past, but never got around to implementing it. He suggested I should create a package first and do some testing before making a PR. I did that, but didn't do more with it for a while. Anyway, I talked to Stefan and some others about it more at JuliaCon last week and got it ready (or, so I believe) to submit to the project.

I put implementation details inside of a module to avoid namespace pollution on some very generically named functions. I don't see this pattern used much elsewhere for small things like this, but it seemed like the cleanest way to do it, so I did it.

At JuliaCon, @vtjnash suggested a much superior way to implement varargs than what I have done using foreigncall. However, foreigncall isn't well documented (that I've found) and I wasn't really able to get a handle on it from the source code (it's implemented in Scheme), and I also wasn't sure off all the additional lowering that would need to be done between the @ccall macro and foriegncall, so I've simply provided the same semantics ccall provides for varargs, which unfortunately means all varargs must be of the same type. For the record (and perhaps for future development), this was Jameson's suggested interface:

@ccall printf("%s = %d\n"::Cstring ; "value of x"::Cstring, x::Cint)::Cint

(mind the semicolon)

The interface I currently provide is this:

@ccall printf("%d, %d, %d\n"::Cstring, 1, 2, 3; varargs=Cint)::Cint

This is obviously not as good, but it's all I can do with the ccall interface, as far as I know. Of course, the two syntactic forms could both be supported by the same macro, but I'm not the person to be implementing that at my current level of Julia ability (well below 9000). I either need some guidance from someone who knows foreigncall (preferably in the form of public documentation) or this functionality needs to be added to ccall and documented--or someone who knows more than I do could just do it, but I guess everyone who knows how foreigncall works has other things to occupy their time.

base/c.jl Outdated Show resolved Hide resolved
test/ccall.jl Outdated Show resolved Hide resolved
base/c.jl Outdated Show resolved Hide resolved
test/ccall.jl Outdated Show resolved Hide resolved
test/ccall.jl Outdated Show resolved Hide resolved
@StefanKarpinski
Copy link
Member

This is a beautiful first PR! Bravo. A couple of suggestions. It would be good to get some tests for the error cases too, to verify that they error and that they do so in the expected fashion. There are some error cases that should probably be explicitly handled rather than failing in the internals of the @ccall macro, e.g.:

julia> @ccall printf("%d\n"::Cstring, 1)::Cvoid;
ERROR: LoadError: type Int64 has no field head

@StefanKarpinski StefanKarpinski added the forget me not PRs that one wants to make sure aren't forgotten label Jul 31, 2019
@StefanKarpinski StefanKarpinski self-assigned this Jul 31, 2019
@ninjaaron
Copy link
Contributor Author

ninjaaron commented Jul 31, 2019

Yikes! I guess so! This case was supposed to be handled on line 544 and 561, but obviously it doesn't work how I think it does!

@StefanKarpinski StefanKarpinski added the feature Indicates new feature / enhancement requests label Jul 31, 2019
@bramtayl
Copy link
Contributor

Is there room to generalize this for calls into other statically typed languages?

@StefanKarpinski
Copy link
Member

Let's discuss that in a separate issue rather than muddying the waters in this PR. This does a fairly straightforward, well-defined thing: it adds a more intuitive syntax for the existing ccall.

@ninjaaron
Copy link
Contributor Author

ninjaaron commented Jul 31, 2019

I'm not the expert here, but ccall (and this @ccall, by extention) should work with any language that can compile C-compatible dynamic libraries. (i.e. most statically compiled languages)

The difficulty is that most static languages require some extra effort create these dynamic libraries; i.e. converting their own internal types to C-types and providing a flat interface that doesn't depend on the language's own notions of objects, generics, high-order functions, etc. (much like you have to wrap Julia functions with @cfunction). Each language tends to have its own way to handle these issues and for that reason, they can't all be treated in a uniform way. It would theoretically be possible to build backends for other compiled languages, but I assume they would each have to be separate projects.

An interesting thing to think about would be how to construct a flat API to do this--but that's exactly what it would have to be: an API. It would be the responsibility of each language backend to implement that API.

Very likely, such an API would rely heavily on Julia's C interface internally, but as Stefan says, it's pretty well outside the scope of this PR...

... But it's an interesting idea.

base/c.jl Outdated Show resolved Hide resolved
@simonbyrne
Copy link
Contributor

The semicolon is perhaps the simplest option, but I think the closest semantically would be something like:

@ccall printf(fmt::Cstring, (x::Cdouble, y::Cint)...)::Cint

@StefanKarpinski
Copy link
Member

I think that @ccall printf(fmt::Cstring, (x::Cdouble, y::Cint)...)::Cint looks too much like it meands the same thing as @ccall printf(fmt::Cstring, x::Cdouble, y::Cint)::Cint. But semicolon does parse differently, so one could potentially write this as:

@ccall printf("%f %d"::Cstring; 1.5::Cdouble, 123::Cint)::Cint

So everything after the semicolon is passed using varargs calling convention. Does that seem feasible, @vtjnash? I have to say, I'm a bit confused about how this works and don't really understand the varargs calling convention and its limitations.

@bramtayl
Copy link
Contributor

bramtayl commented Aug 1, 2019

Could you have a dict to translate Int => CInt etc?

@ninjaaron
Copy link
Contributor Author

The semicolon is perhaps the simplest option, but I think the closest semantically would be something like:

@ccall printf(fmt::Cstring, (x::Cdouble, y::Cint)...)::Cint

@simonbyrne It wouldn't be my preference because, while it does resemble C and Julia syntax around varargs, it's doing something subtly different. I'd prefer something that's less susceptible to misinterpretation by someone learning the API (varargs are strictly not splat-able). Still, I like how this looks and wouldn't be too bothered if the consensus went that way. It would require rebasing the macro on foreigncall, which, as mentioned, is a bit above my pay grade.

@StefanKarpinski the syntax you're suggesting is exactly what @vtjnash suggested to me at JuliaCon, and it's definitely possible with foreigncall.

@simonbyrne
Copy link
Contributor

Could you have a dict to translate Int => CInt etc?

I don't understand what you mean. What purpose would that serve?

@bramtayl
Copy link
Contributor

bramtayl commented Aug 1, 2019

So that you could write @ccall printf(fmt::String, (x::Float, y::Int)...)::Int

@simonbyrne
Copy link
Contributor

It would be very confusing to have Int mean different things in different contexts.

@ninjaaron
Copy link
Contributor Author

ninjaaron commented Aug 1, 2019

@bramtayl

So that you could write @ccall printf(fmt::String, (x::Float, y::Int)...)::Int

It's definitely possible, and it might even be desirable to have a 3rd-party macro that could do something like @c printf(fmt::char[], (x::float, y::int)...)::int (with lowercase types so as not to be conflated with Julia's types), but the idea with this macro is that it's as close ccall as possible while providing more natural, Julia-like syntax. Especially because I'm proposing this for inclusion in Base, I think it's important to be as conservative and obvious as possible with the semantics.

@StefanKarpinski
Copy link
Member

@StefanKarpinski the syntax you're suggesting is exactly what @vtjnash suggested to me at JuliaCon, and it's definitely possible with foreigncall.

I don't quite get why foreigncall is necessary. Can we not support varargs with different types otherwise?

@ninjaaron
Copy link
Contributor Author

ninjaaron commented Aug 1, 2019

The documented convention is this:

ccall(:printf, Cint, (Cstring, Cint...), "%d, %d, %d\n", 1, 2, 3)

In fact, the documentation explicitly says it is not possible to have different types. https://docs.julialang.org/en/v1.3-dev/manual/calling-c-and-fortran-code/#man-bits-types-1 (scroll down a bit to the bottom of the first table there.)

There's no reason why it couldn't be done, since foreigncall does it, it's just not supported for ccall.

@StefanKarpinski
Copy link
Member

It seems to me like a good approach then would be to use the more general syntax and require type annotations on all the varargs arguments but throw an error if they're not all the same, saying "varargs @ccall with different argument types not yet supported" or something like that and then leave an open issue for implementing that with foreigncall that someone else can take on. If that isn't supposed to work with ccall then why does it work for printf? Dumb luck?

@ninjaaron
Copy link
Contributor Author

c.f.

julia> f() = ccall(:printf, Cint, (Cstring, Cint...), "%d, %d, %d\n", 1, 2, 3)
f (generic function with 1 method)

julia> @code_lowered f()
CodeInfo(
1%1 = (Base.cconvert)(Main.Cstring, "%d, %d, %d\n")
│   %2 = (Base.cconvert)(Main.Cint, 1)
│   %3 = (Base.cconvert)(Main.Cint, 2)
│   %4 = (Base.cconvert)(Main.Cint, 3)
│   %5 = (Base.unsafe_convert)(Main.Cstring, %1)
│   %6 = (Base.unsafe_convert)(Main.Cint, %2)
│   %7 = (Base.unsafe_convert)(Main.Cint, %3)
│   %8 = (Base.unsafe_convert)(Main.Cint, %4)
│   %9 = $(Expr(:foreigncall, :(:printf), Int32, svec(Cstring, Vararg{Int32,N} where N), :(:ccall), 4, :(%5), :(%6), :(%7), :(%8), :(%4), :(%3), :(%2), :(%1)))
└──      return %9
)

@StefanKarpinski
Copy link
Member

I guess the issue may be that we just lack a good ccall syntax for explaining what to do.

@ninjaaron
Copy link
Contributor Author

In January, @vtjnash said foreigncall didn't support it, so this may be a relatively recent change.

If that isn't supposed to work with ccall then why does it work for printf? Dumb luck?

Something like that. It depends on your C compiler or platform or something.

I'll implement what you suggest; i.e. using the semicolon to delimit varargs and throwing an error if they aren't all of the same type.

@ninjaaron
Copy link
Contributor Author

Actually, the more I look at this output from the lowering of ccall, I don't really see how foreigncall could support varargs with different types with its current calling convention... (The Varargs{Int32,N} part)

It's possible that I misunderstood Jameson and he was just telling me to re-implement foreigncall in a way that supported different argument types. He was definitely encouraging me to mess with the foreigncall implementation--an offer which I shall politely decline, having only basic Scheme knowledge and none at all of LLVM.

NEWS.md Show resolved Hide resolved
@Gnimuc
Copy link
Contributor

Gnimuc commented Feb 22, 2020

What's the status of this PR? Is there an alternative way to call C variadic functions in Julia even without this PR? #32800 was merged 5 months ago, but we still don't have an easy-to-use syntax to use this feature.

@StefanKarpinski
Copy link
Member

I'm in favor of brushing this off and merging it. A conclusive decision is needed.

@ninjaaron
Copy link
Contributor Author

I was thinking about this the other day.

@StefanKarpinski StefanKarpinski added triage This should be discussed on a triage call and removed forget me not PRs that one wants to make sure aren't forgotten labels Feb 22, 2020
@c42f
Copy link
Member

c42f commented Feb 25, 2020

Agreed, I was thinking just last week that this had been forgotten. Aaron put a lot of work into the implementation; I've reviewed in detail and I'm happy with it, as well as the API which I think has had a fair amount of discussion.

The only thing remaining is to make a decision that this is the right future API for ccall. If you'd like some points to guide the discussion:

  • The syntax has received quite a lot of discussion already and I think we've converged. The main point of difficulty was the varargs notation but I think we've landed on a pretty reasonable syntax for that.
  • Support for calling conventions was intentionally omitted so that we can decide on general syntax for that separately.
  • We have much simpler semantics for @ccall $function_pointer(...) compared to ccall (only supporting pointers). This is quite a tricky part of ccall internals so it would be nice to confirm that the semantics implemented here are the right way forward.

@c42f
Copy link
Member

c42f commented Feb 26, 2020

Test failures seem due to an unrelated change. Perhaps you could squash all these commits @ninjaaron? I think this is self contained enough that it makes sense to land as a single commit.

@ninjaaron
Copy link
Contributor Author

Er... That didn't go right. I'm not an experienced squasher. This may take a minute.

@ninjaaron
Copy link
Contributor Author

ninjaaron commented Feb 26, 2020

I think I finally got it right, but the process seemed a bit weird. I did this:

$ git pull https://github.com/JuliaLang/julia master
$ git reset --soft 9d4e8ae013f05269750035e9483d17fb02a34791  # commit before the first ccall macro commit
$ git commit -am "some message"
$ git pull https://github.com/JuliaLang/julia master
# ... fix some merge conflicts ...
# ... run the tests ...
$ git commit -am "some other message"
$ git push origin +ccallmacro

Is that kind of right?

@StefanKarpinski
Copy link
Member

There's lots of ways to squash. My two favorites:

  • Use git rebase -i master and mark all but one as fixup commits
  • Roughly what you did.

@JeffBezanson
Copy link
Member

From triage:
We discussed @ccall printf("%d, %s"::Cstring, (1::Cint, "abc"::Cstring)…) as a possible varargs syntax, since ; here doesn't have its usual meaning in an argument list. But we decided ; is ok since there's nothing else in C it could mean, and the syntax is nice.
We also discussed @ccall printf("%d, %d, %d"::Cstring, 1::Cint…, 2, 3) as a possible future extension to avoid writing types on every trailing argument when they are the same, but that would be backwards-compatible so it doesn't really matter now.

@ninjaaron
Copy link
Contributor Author

I can implement the second thing you mention there for varargs of the same type fairly easily, but I guess we'd have to start the review process over again.

@KristofferC
Copy link
Member

Imo let's get this in first

@JeffBezanson
Copy link
Member

Yes, I was just recording the conversation, I definitely don't think we need to add more features now.

@c42f
Copy link
Member

c42f commented Feb 28, 2020

Thanks for recording the triage discussion, It's very appreciated for those of us not on the call.

I believed triage approved of this, so I've done a final in-depth review and I still like what I see (keeping a couple of tiny nits to myself ;-) ).

@c42f c42f merged commit 1c24d30 into JuliaLang:master Feb 28, 2020
@c42f c42f removed the triage This should be discussed on a triage call label Feb 28, 2020
@fredrikekre fredrikekre added the needs compat annotation Add !!! compat "Julia x.y" to the docstring label Feb 28, 2020
@ninjaaron ninjaaron deleted the ccallmacro branch February 28, 2020 11:15
ravibitsgoa pushed a commit to ravibitsgoa/julia that referenced this pull request Apr 9, 2020
Here we implement a syntax for ccall with Julia-like type annotations on the arguments.

Compared to ccall:
* The new syntax is more familiar to Julia programmers
* The new syntax gives a notation for calling C varargs functions
* Support for specifying calling convention is not yet implemented as that will require another syntax discussion
* The semantics for interpolating "function like things" is much simplified (only function pointers are allowed)
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
Here we implement a syntax for ccall with Julia-like type annotations on the arguments.

Compared to ccall:
* The new syntax is more familiar to Julia programmers
* The new syntax gives a notation for calling C varargs functions
* Support for specifying calling convention is not yet implemented as that will require another syntax discussion
* The semantics for interpolating "function like things" is much simplified (only function pointers are allowed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests needs compat annotation Add !!! compat "Julia x.y" to the docstring
Projects
None yet
Development

Successfully merging this pull request may close these issues.