crystal-lang / crystal_lib

Automatic binding generator for native libraries in Crystal
138 stars 30 forks source link

va_list type support (stdargs.h) #41

Closed olbat closed 6 years ago

olbat commented 7 years ago

Hello,

I'm trying to port a library that uses a va_list type (from stdargs.h) using crystal_lib.

The problem is that it generates an alias to LibC::VaList that doesn't seems to exists in any of crystal's lib C bindings so I get an undefined constant LibC::VaList at compile time.

Example:

$ cat /tmp/test.h
#include <stdarg.h>
void test(va_list vl);

$ cat test.cr
require "lib_c"
@[Include("/tmp/test.h", prefix: "test", remove_prefix: false)]
lib LibTest
end

$ crystal src/main.cr -- test.cr
require "lib_c"
lib LibTest
  fun test(vl : VaList)
  alias X__GnucVaList = LibC::VaList
  alias VaList = X__GnucVaList
end

Am I doing something wrong ?

mverzilli commented 7 years ago

I investigated this (by the way, thank you so much for including a self contained example in the issue ❤️ ), and I think you're doing nothing wrong.

If, for example, in the generated code you replace LibC::VaList with LibC::ULongLong, it compiles.

I think the problem is Crystal's LibC bindings don't define VaList: https://github.com/crystal-lang/crystal/blob/master/src/lib_c.cr

At the same time crystal_lib asumes that it's going to be defined somewhere: https://github.com/crystal-lang/crystal_lib/blob/d8aca65805db3994e539bf591a99a0064b1caea8/src/crystal_lib/type_mapper.cr#L59

So a workaround would be to define it (you can reopen lib LibC as many times as you want)

olbat commented 7 years ago

Thank you for your answer !

I'm not sure that replacing LibC::VaList by LibC::ULongLong is the best way to solve this issue since it depends on the system's lib C implementation (so it might not work on every platform) but I think I'll do so as a temporary fix for sure.

If I understand it properly, I think that crystal's LibC lib is generated using @ysbaddaden's posix generator that does not support stdargs.h yet (maybe because there is only macro and types definition in the header ?).

A way to solve this may be to add / ask for it's support in the crystal's LibC/posix binding ?

mverzilli commented 7 years ago

Sorry, maybe my reply wasn't clear. I'm not proposing to replace LibC::Valist with LibC::ULongLong, I just used that as a way of checking against the bindings that come with Crystal. Anyway, I think we're on the same page with that :).

About the LibC lib being generated with posix, I don't really know, so we can wait for him to confirm. If that's the case, maybe we can move this issue there.

There's quite a lot of types that crystal_lib still doesn't support, maybe we should throw a NotImplementedException when we stumble upon them?

ysbaddaden commented 7 years ago

We don't support va_list. We don't use it in Crystal, and it's interface is a bunch of macros that would have to be rewritten for each platform.

You may use ... in fun definitions instead, which will be handled by LLVM.

mverzilli commented 7 years ago

Thanks for the context info @ysbaddaden! The thing that got me is that it is explicitly listed here: https://github.com/crystal-lang/crystal_lib/blob/d8aca65805db3994e539bf591a99a0064b1caea8/src/crystal_lib/type_mapper.cr#L58

So we need to translate it to a variadic function

ysbaddaden commented 7 years ago

I believe the VaList type is for the ... syntax, not the va_list definitions that would be interpreted as C (functions or macros) by clang, which is exactly what happens in your initial example.

We can translate/map the implicit ... but we can't map the explicit va_list. I'm afraid that won't change. Are there no other ways to call the C functions?

mverzilli commented 7 years ago

I see! Watching crystal_lib issues is really forcing me out of my comfort zone with C, so I really appreciate your guidance 🙇 (and your issues @olbat !).

I've read a bit more about va_list and I now understand what you mean: it doesn't make any sense for a header other than stdargs.h itself to use va_list, given it's actually an implementation detail to support .... That makes me wonder, @olbat, what lib are you trying to bind to? Can you share that?

olbat commented 7 years ago

Are there no other ways to call the C functions?

Sadly, not for the functions I've checked ...

Anyway I think I'll leave them for now since it's not the most important functions of the library and I still have other problems to fix (such as symbols renaming/suffixing in shared libraries, I'll probably post something on the google group for this one).

That makes me wonder, @olbat, what lib are you trying to bind to? Can you share that?

Ofc: I'm working on a binding to the ICU library. If it goes well, I'll probably write a crystal wrapper too (at least for the main features since the lib is pretty big).

Fryguy commented 6 years ago

@olbat Can this be closed now that we have https://github.com/crystal-lang/crystal/pull/5103 ?

olbat commented 6 years ago

Yes, I think it can be closed.

I'll try to run it on the ICU binding to see how it goes :)