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

Foreign function interface at Compile Time #10150

Merged
merged 9 commits into from
Feb 23, 2019
Merged

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jan 1, 2019

just in time for the new year!

[EDIT]

nimble install libffi
nim c -d:nimHasLibFFI compiler/nim.nim
compiler/nim c --experimental:compiletimeFFI -r tests/vm/tevalffi.nim

the test shows, as an example, that we can use these at CT:

proc c_exp(a: float64): float64 {.importc: "exp", header: "<math.h>".}
proc c_printf(frmt: cstring): cint {.importc: "printf", header: "<stdio.h>", varargs, discardable.}
proc c_snprintf*(buffer: pointer, buf_size: uint, format: cstring): cint {.importc: "snprintf", header: "<stdio.h>", varargs .}
proc c_malloc(size:uint):pointer {.importc:"malloc", header: "<stdlib.h>".}
proc c_free(p: pointer) {.importc:"free", header: "<stdlib.h>".}

sure, there may be edge cases not yet supported or buggy, but that's already super useful

note

#10611 should be merged first (ignore it in the diff); it's needed because of #10632
EDIT: removed it

@timotheecour timotheecour mentioned this pull request Jan 1, 2019
compiler/evalffi.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Jan 1, 2019

Now with .experimental we have a way to add this to Nim. Still makes me uneasy though. Maybe there is a way to put it behind a keyword for easy audits, like cast tries to do.

@dom96
Copy link
Contributor

dom96 commented Jan 2, 2019

yay! One step closer to getting a fast Nim REPL :D

@krux02 krux02 changed the title [WIP] fix #9253 : enable FFI at CT FFI at CT Jan 7, 2019
@krux02 krux02 added the Feature label Jan 7, 2019
@krux02
Copy link
Contributor

krux02 commented Jan 7, 2019

Please elaborate on the "edge cases not yet supported or buggy". That test is currently a bit minimal. Can you run an interactive SDL application with an OpenGL context in a window at compile time?

@zah
Copy link
Member

zah commented Jan 7, 2019

I'm curious, does this fix #8405 as well?
If it does, I suggest enabling it by default because the issue above is quite a blocker for every day usage of the new hot code reloading branch.

@krux02
Copy link
Contributor

krux02 commented Jan 7, 2019

@zah The reason why this should not be enabled by default is, because we have doubts that this will work reliably. So yes when this really works well for all use cases, it should be enabled by default, because yes it is useful. But I looked into how FFI works and my suspicion is that this feature is a can of worms that should be locked behind an experimental flag.

Timothee uses libffi, so he outsources the wormy problem to an external library.

@Araq
Copy link
Member

Araq commented Jan 7, 2019

The unwritten policy is that new features start behind an .experimental switch even if we all think "it's really solid and will cause no problems".

@timotheecour
Copy link
Member Author

The unwritten policy is that new features start behind an .experimental switch even if we all think "it's really solid and will cause no problems".

  • --experimental:allowFFI does what you'd expect (enables/disables the feature)
  • however it can't be localized via push: {.push experimental: "allowFFI".} does not work as I'm showing in tests/vm/tevalffi.nim (
    # todo: this has no effect (unlike `--experimental:allowFFI`)
    ), since VM Context is different from regular Context

(still need to push my latest changes btw)

@alaviss
Copy link
Collaborator

alaviss commented Jan 8, 2019

--experimental:allowFFI does what you'd expect (enables/disables the feature)

This might be intepreted that you would need this flag to use any kind of FFI in Nim. With that said, I can't come up with any better name.

compiler/options.nim Outdated Show resolved Hide resolved
@mratsim
Copy link
Collaborator

mratsim commented Jan 8, 2019

AllowStaticFFI or AllowCTFFI?

@timotheecour
Copy link
Member Author

timotheecour commented Feb 12, 2019

ok finally getting back to this:

  • rebased
  • added support for more code to work at CT, eg printf, sprintf, and other fixes, see tests.
  • FFI at CT is now being tested in CI to make sure it keeps working; it now requires -d:nimHasLibFFI (for building compiler; requires nimble install libffi) and --experimental:compiletimeFFI (for compiling nim code)

@zah

I'm curious, does this fix #8405 as well?

mostly, but not all cases; we can add missing support in subsequent PRs.
eg: this doesn't work with -d:useNimRtl (ie, an instance of #8405) but works with this PR:

import strutils
# const a = "abc" in "abcdef" # dosen't yet work because `tyRange` isn't mapped in `mapType`
const a = "abc".countLines # this works fine
echo(a)
# build compiler
nim c -d:nimHasLibFFI compiler/nim.nim
# build RTL: lib/libnimrtl.dylib
nim c -d:release lib/nimrtl.nim

export DYLD_LIBRARY_PATH=lib # so that `libnimrtl.dylib` can be found; there are other ways though
nim c --experimental:compiletimeFFI -d:useNimRtl main.nim

echo "output OK"
removeFile("outputGotten.txt")
removeFile(megatestFile)
#testSpec r, makeTest("megatest", options, cat)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is here so much diff in categories.nim? AFAIK this file doesn't need to be touched to test compile time FFI.

Copy link
Member Author

Choose a reason for hiding this comment

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

@krux02
I explained this in top-post:

#10611 should be merged first (ignore it in the diff); it's needed because of #10632

Copy link
Contributor

Choose a reason for hiding this comment

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

No it's not needed. As far as I can tell #10611 is unrelated to this PR. Please disconnect this PR from the other one.

Copy link
Member Author

@timotheecour timotheecour Feb 13, 2019

Choose a reason for hiding this comment

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

No it's not needed.

I just removed it.
As a result, as explained in #10611, I can't test this PR using my own travis instance, so my previous workflow doesn't work anymore:

  • A1: test a commit locally; if green:
  • A2: push on a private branch; if my personal travis instance is green:
  • A3: push on PR branch; this runs on shared travis resources

the point of A2 being to avoid wasting shared travis resources for the benefit of everyone. Now that A2 doesn't work, I have to keep rebasing/removing that PR each time, or skip A2 altogether

Copy link
Contributor

@krux02 krux02 Feb 13, 2019

Choose a reason for hiding this comment

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

I noted in a previous comment to elaborate what local travis is. This pretty much explais it to me. Still it won't harm if you describe it in the PR anyway. But no you don't need to "keep rebasing/removing". Just skip A2 altogether, you can always reintroduce it if you find a way to make it work.

@Araq
Copy link
Member

Araq commented Feb 23, 2019

Merging so that we don't lose this work but I'll disable it afterwards.

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

Successfully merging this pull request may close these issues.

enable FFI at CT
7 participants