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

Generated POSIX/C bindings + FreeBSD support #2442

Merged
merged 11 commits into from
May 5, 2016

Conversation

ysbaddaden
Copy link
Contributor

I wrote a project to generate POSIX/C bindings for Crystal automatically given a set C headers: posix. It started using CrystalLib, but eventually evolved to just use the CrystalLib parser, to try to cleanup bindings as much as possible. For example to avoid many useless and ugly alias X__SizeT = SizeT; or being able to specify what types and structs to generate so we could have LibC dispatched into many files, not just one; etc.

I eventually tried to use the generated headers with Crystal, cleaning up the core/stdlib source code along the way. Hopefully it only required a few tweaks for each platform. Even integrating a new UNIX target (FreebSD x86_64, e854c2d) was done in a few hours, most of them spent on libgc or waiting for the cross compiled compiler to be linked (~10 minutes in my VM) or dealing with port issues, not grepping include files for each constant or struct definition, which is very human-error prone.

This pull request doesn't integrate the posix project, but rather adds the generated headers for each target. This takes some non negligible space, for mostly redundant code that could be generated cleanly for each OS. Yet, the posix project is a pile of hacks on top of CrystalLib, and I don't think it would fit properly here; it would also add clang as a permanent requirement. The con is that the target LibC folder must be injected into CRYSTAL_PATH. This is done automatically in Crystal::CrystalPath for known targets (see 8138ce7) but obviously the current compiler doesn't do that, so I modified bin/crystal to do the job until the next release of Crystal (b13e5f1).

Crystal only requires a subset of all the headers the posix project generates. I limited the generated headers here, but I think we could limit them more. For example the headers below are only there because POSIX says they should be included autmatically by headers Crystal uses. Maybe they'll come handy, or not? Tell me.

  • limits.h
  • locale.h
  • sched.h
  • math.h —I found out Crystal uses LLVM math
  • stdint.h

Notes:

  • this Pull Request only cares about POSIX. I didn't touch C libraries (eg: LibGC, LibLLVM, LibPCRE...);
  • the Pull Request is rather big (because it includes bindings for 6 targets). Please review individual commits!

closes #1653 #1413

@ysbaddaden
Copy link
Contributor Author

TODO:

  • format src files;
  • broken samples/2048.cr (LibC.cfmakeraw is mising);
  • detect x86-linux-gnu target on TravisCI;
  • missing LibC::RTLD_DEFAULT constant for i686-linux-gnu target
  • broken bindings for i686-linux-gnu
  • broken detection of i686 vs x86
  • use generated math.th
  • reduce the bindings (remove some headers, constants, types and functions)

@asterite
Copy link
Member

Awesome!!

I won't be able to look at this these days, because I'm finishing implementing #2390 and it's a huge change. I have a few comments and questions :-)

  1. Do we need all of the C definitions? The standard library uses just very few C functions and structs. I'm afraid that loading all of that into memory will increase compilation times and used memory a lot... though of course I didn't test it, but you can try and check how much time/memory is takes to compile an empty file with an optimized compiler.
  2. Soon I'll implement the last part of Semi-formal specification of the new "global" type inference algorithm #2390 that deals with instance vars and the diff is huge and affects almost all files (in addition to implementing the new algorithm I'll be removing all the redundant type annotations I added in the past). I really hope you don't mind having to rebase this against those changes later 😊

@ysbaddaden
Copy link
Contributor Author

  1. Yes, I'm wondering about that. We don't need everything, but most of them. I'm looking for a balance between what we need now and what we expect to need in the middle term, to avoid regenerating headers all the time for all platforms, or even have to deal with either the fun exists or not. It's nice if it's already here! I will first remove the headers we don't need, that will remove unused structs and types. I'm not sure removing fun declarations has an impact or not? I'll check!
  2. I had already checked your branch and I don't see much conflicts, so it shouldn't be that hard, hopefully :-)
  3. Please look with @waj at some changes I did, and please tell me if they're acceptable or if should restore some things. I'm all ears!

@ysbaddaden
Copy link
Contributor Author

@asterite here is a bench. On the left is the posix branch, on the right is the master branch against an empty file. There seems to be a small impact due to the increased constants and types (structs, unions). For example GC reports a 5MB memory increase after parsing, but the difference immediately levels (down to 0.3MB eventually). Compile time increases by some milliseconds too. The impact is hardly noticeable on a good CPU, it may be on a slower CPU.

POSIX BRANCH                                                      │ MASTER BRANCH
Parse:                             00:00:00.0402310 (  19.93MB)   │ Parse:                             00:00:00.0379900 (  14.77MB)
Semantic (top level):              00:00:00.0154930 (  19.93MB)   │ Semantic (top level):              00:00:00.0097270 (  19.70MB)
Semantic (abstract def check):     00:00:00.0006590 (  19.93MB)   │ Semantic (abstract def check):     00:00:00.0005090 (  19.70MB)
Semantic (type declarations):      00:00:00.0048230 (  19.93MB)   │ Semantic (type declarations):      00:00:00.0031490 (  19.70MB)
Semantic (initializers):           00:00:00.0005070 (  19.93MB)   │ Semantic (initializers):           00:00:00.0003390 (  19.70MB)
Semantic (main):                   00:00:00.0458650 (  26.57MB)   │ Semantic (main):                   00:00:00.0211300 (  26.27MB)
Semantic (cleanup):                00:00:00.0000090 (  26.57MB)   │ Semantic (cleanup):                00:00:00.0000060 (  26.27MB)
Semantic (recursive struct check): 00:00:00.0003810 (  26.57MB)   │ Semantic (recursive struct check): 00:00:00.0064120 (  26.27MB)
Codegen (crystal):                 00:00:00.0244030 (  26.57MB)   │ Codegen (crystal):                 00:00:00.0187900 (  26.27MB)
Codegen (bc+obj):                  00:00:00.0798950 (  26.57MB)   │ Codegen (bc+obj):                  00:00:00.0722790 (  26.27MB)
Codegen (linking):                 00:00:00.0293600 (  26.57MB)   │ Codegen (linking):                 00:00:00.0373650 (  26.27MB)
        Command being timed: "bin/crystal build --stats empty.cr" │         Command being timed: "bin/crystal build --stats empty.cr"
        User time (seconds): 0.21                                 │         User time (seconds): 0.18
        System time (seconds): 0.25                               │         System time (seconds): 0.22
        Percent of CPU this job got: 156%                         │         Percent of CPU this job got: 165%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.30      │         Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.24
        Average shared text size (kbytes): 0                      │         Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0                    │         Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0                            │         Average stack size (kbytes): 0
        Average total size (kbytes): 0                            │         Average total size (kbytes): 0
        Maximum resident set size (kbytes): 40860                 │         Maximum resident set size (kbytes): 39908
        Average resident set size (kbytes): 0                     │         Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0                      │         Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 54513             │         Minor (reclaiming a frame) page faults: 49480
        Voluntary context switches: 908                           │         Voluntary context switches: 936
        Involuntary context switches: 172                         │         Involuntary context switches: 129
        Swaps: 0                                                  │         Swaps: 0
        File system inputs: 0                                     │         File system inputs: 0
        File system outputs: 1536                                 │         File system outputs: 1528
        Socket messages sent: 0                                   │         Socket messages sent: 0
        Socket messages received: 0                               │         Socket messages received: 0
        Signals delivered: 0                                      │         Signals delivered: 0
        Page size (bytes): 4096                                   │         Page size (bytes): 4096
        Exit status: 0                                            │         Exit status: 0

@ysbaddaden
Copy link
Contributor Author

Green, eventually :D

BTW: encoding specs ar failing on FreBSD because both GB2312 AND UCS-2LE aren't supported, either in encoding or decoding (or so it seems). musl-libc has less failures, since only encoding (or was it decoding?) to GB3212 isn't supported.

@ysbaddaden
Copy link
Contributor Author

The difference is more noticeable when compiling Crystal: posix branch on the left, master branch on the right, each one compiling its own branch. 40MB difference in memory usage isn't negligible. Otherwise compilation time is roughly on par.

POSIX BRANCH                                                    │ MASTER BRANCH
Parse:                             00:00:00.1492000 (  50.62MB) │Parse:                             00:00:00.1338810 (  50.30MB)
Semantic (top level):              00:00:00.0794760 (  74.62MB) │Semantic (top level):              00:00:00.0752520 (  74.30MB)
Semantic (abstract def check):     00:00:00.0009910 (  74.62MB) │Semantic (abstract def check):     00:00:00.0009140 (  74.30MB)
Semantic (type declarations):      00:00:00.0194880 (  74.62MB) │Semantic (type declarations):      00:00:00.0202900 (  82.30MB)
Semantic (initializers):           00:00:00.0023210 (  74.62MB) │Semantic (initializers):           00:00:00.0023330 (  82.30MB)
Semantic (main):                   00:00:04.3043480 ( 638.55MB) │Semantic (main):                   00:00:04.7031270 ( 614.24MB)
Semantic (cleanup):                00:00:00.0004670 ( 638.55MB) │Semantic (cleanup):                00:00:00.0004870 ( 614.24MB)
Semantic (recursive struct check): 00:00:00.0011390 ( 638.55MB) │Semantic (recursive struct check): 00:00:00.0008770 ( 614.24MB)
Codegen (crystal):                 00:00:02.0749410 ( 722.55MB) │Codegen (crystal):                 00:00:02.0801440 ( 682.24MB)
Codegen (bc+obj):                  00:00:29.7894070 ( 722.55MB) │Codegen (bc+obj):                  00:00:28.2382270 ( 682.24MB)
Codegen (linking):                 00:00:03.9869030 ( 722.55MB) │Codegen (linking):                 00:00:04.0275530 ( 682.24MB)
        Command being timed: "make stats=1"                     │        Command being timed: "make stats=1"
        User time (seconds): 14.32                              │        User time (seconds): 14.80
        System time (seconds): 46.38                            │        System time (seconds): 44.14
        Percent of CPU this job got: 148%                       │        Percent of CPU this job got: 148%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:40.90    │        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:39.68
        Average shared text size (kbytes): 0                    │        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0                  │        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0                          │        Average stack size (kbytes): 0
        Average total size (kbytes): 0                          │        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 1153532             │        Maximum resident set size (kbytes): 1105712
        Average resident set size (kbytes): 0                   │        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0                    │        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 928665          │        Minor (reclaiming a frame) page faults: 926981
        Voluntary context switches: 3800                        │        Voluntary context switches: 3775
        Involuntary context switches: 12378                     │        Involuntary context switches: 12237
        Swaps: 0                                                │        Swaps: 0
        File system inputs: 0                                   │        File system inputs: 0
        File system outputs: 141400                             │        File system outputs: 141344
        Socket messages sent: 0                                 │        Socket messages sent: 0
        Socket messages received: 0                             │        Socket messages received: 0
        Signals delivered: 0                                    │        Signals delivered: 0
        Page size (bytes): 4096                                 │        Page size (bytes): 4096
        Exit status: 0                                          │        Exit status: 0

@asterite
Copy link
Member

Yeah, 40MB difference is a lot, thought maybe it can be reduced by changing the internal structure of an ASTNode (right now the base type occupies about 72 bytes, a fun definition about 144 bytes), and by reusing some nodes (for example a same type is mentioned over and over in a lib), but I'm not sure.

@ysbaddaden
Copy link
Contributor Author

I think some added constants and functions may be welcomed in the near feature (eg: sockets, fcntl, unistd) to expand th stdlib, some will stay useless (eg: limits?, obscure functions from stdio, string...), while some others can be more tricky: will we need the pthread types (attr, barrier) or process scheduling (sched, pthread sched params)? Or are those too far fetched that it's useless to include them now?

@asterite
Copy link
Member

With @waj we always had the idea of removing as much C bindings as possible from the standard library in order to make it more portable and independent. That's why I don't think it's a good idea to include a lot of C bindings, specially if they aren't going to be used right now.

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Apr 27, 2016

I merged/squashed from master and reduced the generated bindings. There are still a few more constants and C functions than stricly required, but most are related to constants we use now, or some missing stdlib features (eg: chown).

For example I junked limits.h, most of stdio, most of constants from unistd, etc. Tell me if you want to drop even more things.

@ysbaddaden
Copy link
Contributor Author

Simplified a bit more, by removing bindings for math.h (LibM already does and there aren't noticeable differences between targets). I also fixed an issue for FreeBSD.

A Crystal binary for FreeBSD (that can compile this branch) is also available:
https://dl.dropboxusercontent.com/u/53345358/freebsd/setup.txt

@ysbaddaden
Copy link
Contributor Author

@asterite I now don't notice a difference in memory usage with the master branch anymore!

@asterite
Copy link
Member

@ysbaddaden Great!

One question: why is CRYSTAL_PATH modified in bin/crystal? I think this should be possible without having to change that. I see the comment that it's just for the next release, but I don't understand how would that work.

I think this PR really cleans up things and organizes code (C code is separate from Crystal code). However, I would still like to keep C bindings as a minimum and not add extra bindings that aren't needed right now.

Ideally, the compiler should auto-generate these bindings as needed. What we had in the mind is that you'd write something like:

lib LibC
  @[Import]
  fun write
end

or something like that, and the compiler would complete the function with the correct argument types, even maybe declaring the necessary structs (although it would be better to have to explicitly import them too because renaming things from C to Crystal is hard). The compiler would use libclang for thing, so the compiler will always depend on it. In this way we don't have to maintain sources for each platform that we support, and we only import functions/structs that are really needed. Some of this is already done by crystal_lib, integrating it nicely in the compiler is the hard/tricky part.

(crystal_lib allows importing many functions at one with a prefix, but I think that's an anti-feature because of name conflicts, it's better if we are explicit about which functions/structs are imported)

The compiler could cache the generated bindings so it doesn't have to generate them all the time, unless the source file that declares them changes.

The only problem with this is that to cross-compile to a new platform you won't be able to do it, because a compiler for such platform doesn't exist. Maybe in that case we could allow specifying a path to lookup C headers and when cross-compiling to a new platform you'd specify that.

Do you think that all of this makes sense and is possible to implement? I really wouldn't mind depending on libclang if writing C bindings will become easier and less source code is needed. Also note that this will also be useful for any C library in any shard. I really think this is the ideal solution for this problem.

@asterite
Copy link
Member

I see your posix project generates these bindings from a bunch of YAML files. If I'm not mistaken, what we would need to do is to generate these bindings from lib declarations + some attributes in crystal's source code. With a first pass we could gather all of these and then pass them to posix (which would be inside the compiler's source code now... we can later think how to extract it into a separate project and make the compiler depend on it, if we want to). So the whole thing would consist of:

  1. Think of a way to express what you have in YAML in Crystal code (maybe a bunch of attributes, or just one attribute that contains a hash-literal for such info). crystal_lib tries to do this but I don't feel it's quite good right now.
  2. Make the pass to gather these annotations
  3. Generate the bindings (the real lib with all the declarations)
  4. Cache them somewhere, and "link" them to the project
  5. Make the compiler depend on libclang for this

Makes sense? Possible? Any reason not to go in this direction?

@asterite
Copy link
Member

So, a syntax for how to specify what and how to import can be:

# This attribute tells the compiler to generate the body of the given lib.
@[Import({
  includes:  "dlfcn",
  libraries: {
    darwin: "dl",
    gnu:    "dl",
  },
  constants: %w(RTLD_LAZY RTLD_NOW RTLD_GLOBAL RTLD_LOCAL),
  structs:   "Dl_info",
  functions: %w(dlclose dlerror dlopen dlsym dladdr),
})]
lib LibC
end

That's more or less what you have in your YAML, but translated as an attribute value. The compiler can be permissive about some things, like allowing a single string literal instead of an array of string literals when expecting an array, or internally doing ifdef if it finds a hash (like the case of "libraries").

I think this way (a single attribute) is better than crystal_lib's current approach because it's more compact and easier to tweak and understand.

And of course this can be used for other C libraries:

@[Import({
  includes:  "gmp",
  libraries: "gmp",
  structs:   %w(MPZ MPQ MPF),
  functions: %w(__gmpz_init __gmpz_init2 __gmpz_add),
})]
lib LibGMP
end

@ysbaddaden
Copy link
Contributor Author

This PR is a first step towards a nicer LibC integration: an overall cleanup (separate C and CR) and a simpler way to target new platforms, namely BSD, Solaris, Cygwin and Android (NDK) or iOS.

Maybe we don't need every target to be included in the source code. Maybe we could generate them for the target when distributing Crystal only?

Always generating the bindings for the platform we're compiling on would be great. Especially it would avoid to have static bindings in the code and to update them for each target whenever we add a single fun or type, which is boring.

It's still better than the existing, though, since we don't have to fiddle manually into C headers to find every constant, struct definition, ... which is time consuming and prone to stupid human errors.

Always generating bindings may make cross compilation harder, thought, or maybe not if we use a cache folder? It means targeting a new platform may be harder, or compiling to Android (NDK) or iOS. We thus still need a way to have many targets in parallel, so I guess the CrystalPath changes would still be required.

BTW: the bin/crystal patch is because the next Crystal will detect the target and add it to CRYSTAL_PATH, but the current Crystal doesn't do that. I'm thus using this temporary solution, so the current Crystal will include the target into CRYSTAL_PATH, hence be capable to build the next Crystal. Without this patch CI would fail to compile.

@asterite
Copy link
Member

Ideally all code should be written in Crystal except things that would be very hard to do like that. Specifically: syscalls. That's why I don't think it's a good idea to bring so many C functions and constants into LibC. For example there's no point in using string.h since all of String's operations are coded in Crystal. We do use strlen but only because it's there and we can use it, but we could code that too in Crystal (though probably strlen is coded in assembly, but we can do that too).

The worry we have about cross-compiling to a new platform: how did you generate C bindings for freebsd without a compiler for it? Or did you first created a compiler and then ran posix there to generate the definitions?

@ysbaddaden
Copy link
Contributor Author

Sorry, I should have explained that: I "cross-compiled" the FreeBSD bindings, as well as most of the bindings! I copy-pasted /usr/include from a FreeBSD install to my Linux host, then ran:

$ CPATH=/path/to/freebsd_include make crystal ARCH=x86_64 SYS=portbld ABI=freebsd

And libclang took the provided headers for FreeBSD, not the system ones. Then I could cross-compile a bootstrap compiler that I linked on the FreeBSD install (linking took 10 minutes!) and then it could compile itself! Well, it took a bunch of cross compilation to achieve it, most of them because of libgc —I struggled to understand I needed the gc-threaded variant.

It only failed to build correct bindings for i686-linux-gnu, but even on precise32 I had to force the C_INCLUDE_DIR environment variable with different folders for this target... maybe using a chroot and a static build of posix would have work too, and not require a compiler for the destination platform.

Look at the bindings now, I dropped most of string.h and only kept the few str* syscalls that are required in Crystal (yes, they are) I did the same for almost everything. I can still remove a few more things, and I'll do it, but not that much.

I kept the naming of headers so we have a clear mapping of CR -> C. It would also allow to have POSIX become a c shard that would build complete POSIX bindings when installed for projects that need more syscalls, yet still be compatible with the stdlib (that would require bindings from the shard, not embedded with Crystal).

@asterite
Copy link
Member

asterite commented May 3, 2016

@ysbaddaden Could you rebase one more time? I'll try to discuss this with @waj today and see if we merge. I think even without the smart bindings that we want to have this is still a very good change as it separates C bindings from other code, and of course it makes it work with FreeBSD.

So it seems that for cross-compiling to a new platform we could still use the smart, on-the-fly, bindings by specifying CPATH and C_INCLUDE_DIR, right? :-)

@ysbaddaden
Copy link
Contributor Author

Yes, I cleaned a bunch more definitions from bindings, and they should now be down to just what's required plus one or two extras like CLOCK_MONOTONIC and gmtime_r.

I didn't update the PR yet. Please tell me what you and @waj think and then I'll rebase (or not, sob)!

@asterite
Copy link
Member

asterite commented May 3, 2016

@ysbaddaden I just talked with @waj. We concluded that:

  1. We don't know if this is how things we'll end up
  2. But we don't know when we'll make an improvement (the on-the-fly generated code, or other ideas)
  3. Your PR definitely improves the current situation and makes Crystal work in other platforms (and adding a new platform should also be easier)

So, yes, please rebase :-)

Then I'll test this locally, check if it there are no problems with omnibus-crystal and finally merge this.

Thank you! ❤️

@ysbaddaden
Copy link
Contributor Author

@asterite rebased & tidied up!

@asterite
Copy link
Member

asterite commented May 5, 2016

@ysbaddaden Awesome! Travis is green, so I'm merging this. Should we keep the commits or should I squash them into a single one?

@asterite
Copy link
Member

asterite commented May 5, 2016

I think I'll merge, the commits are separated really nicely :-)

Thank you!! ❤️

@asterite asterite merged commit a2ba374 into crystal-lang:master May 5, 2016
@ysbaddaden ysbaddaden deleted the posix branch May 5, 2016 15:52
@valpackett
Copy link
Contributor

I think I just built it on my FreeBSD 11-CURRENT laptop…

screen_2016-05-05-19 21 52

The build was suspiciously fast though! Is Crystal supposed to be this fast? :D

@asterite
Copy link
Member

asterite commented May 5, 2016

@myfreeweb how much time did it take? For me it takes about 15~20 seconds. So yes, I guess that's fast :-)

@valpackett
Copy link
Contributor

@asterite yeah, about 15 seconds. I'm used to small Rust projects taking much longer!

@asterite
Copy link
Member

asterite commented May 5, 2016

@myfreeweb let's hope MIR improves things for Rust :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants