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

Implement FFI bindings #11475

Merged
merged 10 commits into from
Dec 8, 2021
Merged

Conversation

straight-shoota
Copy link
Member

This patch adds a Foreign Function Interface to the compiler via bindings to libffi.

We need this for interpreted mode and, similar to #11434, it imitates the behaviour of ld.so/ld for loading and calling symbols from dynamic libraries at runtime.

Also like #11434 it's a preparatory step for #11159 and adds nothing by itself. But it's an isolated non-trivial component that we can discuss it on its own.

It picks up right after the feature added in #11434, which gives you the address of a loaded symbol, but you can't really do anything with it. Through FFI you can call this function. In order to do that, libffi needs a description of the function interface, similar to a function declaration in a header file in C or a lib fun in Crystal. This description is represented as a CallInterface.

The major part of the implementation is already present in #11159. I just polished it up, added specs and filled out a few gaps (mostly in the C bindings).

This is just a minimal API driven by the needs of the interpreter. It's by far not a generic binding for libffi and supposed to be an internal part of the compiler.

Depends on #11434
/cc #11177

@straight-shoota
Copy link
Member Author

The regular docker build images used for CI are missing libffi, hence the specs don't work there. That's why I'm temporarily switching them out for the crystallang/crystal:ci-libffi-dev image, which is created by a maintenance build of crystal-lang/distribution-scripts#170

@asterite
Copy link
Member

Totally unrelated to the contents of this PR, but if us core team members sent PR with branches in this repo, stacking PRs would be much easier to review and understand: you would be able to select a base branch, and you will be able to see a diff from just this PR, not also the PR it's based on.

Is there a reason we are not doing that? Or, put another way: is there a reason why you all are not doing that? I always do it. It's just easier. You can also use the "copy branch" link at the top to checkout the PR. Everything becomes much easier!

@j8r
Copy link
Contributor

j8r commented Nov 18, 2021

Diffing branches can always be done locally, by adding the remotes of others.

@Fryguy
Copy link
Contributor

Fryguy commented Nov 18, 2021

FWIW you can do a branch compare in GitHub across any forks directly in the URL (it's a little easier than trying to select it in the UI and it's more flexible).

https://github.com/crystal-lang/crystal/compare/FORK:REF...FORK:REF

where FORK is the name of the user (if left out it will default to the user as listed in the URL, so in the example above, crystal-lang), and REF can be any ref (e.g. branch, tag, SHA, and even can include things like ~).

For example
https://github.com/crystal-lang/crystal/compare/straight-shoota:feature/loader...straight-shoota:feature/ffi [link]


Personally, whenever I stack PRs in my branches in my day-to-day projects, I always add a helper link in the OP for reviewers that filters out the underlying PR by comparing to the underlying PR's branch ref similar to the example above... This way, even as I make changes including force pushes, that link will always be accurate.

@straight-shoota
Copy link
Member Author

This is a draft as a preview. It's not ready for review. I'll rebase after #11434 is merged.

Please keep the discussion here on topic. We can talk about any other concerns at https://forum.crystal-lang.org/

straight-shoota and others added 2 commits November 25, 2021 22:09
Add bindings for libffi on linux.

Co-authored-by: Ary Borenszweig <[email protected]>
The changes to the bootstrap images are temporary, until we provide
libffi in the default images.
@straight-shoota straight-shoota marked this pull request as ready for review November 25, 2021 21:11
This was kind of an experiment and it worked on my dev machine,
but apparently is not portable. We do not really use or need this
so we can just remove the test.
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

spec/compiler/data/ffi/sum.c Outdated Show resolved Hide resolved
spec/compiler/data/ffi/sum.c Outdated Show resolved Hide resolved
src/compiler/crystal/loader.cr Show resolved Hide resolved
@straight-shoota
Copy link
Member Author

I've updated the CI configuration to use the regular build images again (the 1.2.2 images have been updated to include libffi).

This should be ready to merge now.

@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 7, 2021
@straight-shoota straight-shoota merged commit 3b89fd6 into crystal-lang:master Dec 8, 2021
@straight-shoota straight-shoota deleted the feature/ffi branch December 8, 2021 11:08
@HertzDevil
Copy link
Contributor

Don't we also need -Dwithout_ffi on Windows CI?

@straight-shoota
Copy link
Member Author

No. The FFI bindings are currently only used from ffi_spec, which is skipped on non-UNIX platforms.

{% skip_file if !flag?(:unix) || flag?(:without_ffi) %}

They'll actually be used with the interpreter. But even then, FFI only makes sense with dynamic loading at runtime (Crystal::Loader), which is not yet implemented on windows.

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.

6 participants