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

feat: ffi to replace plugins #11152

Merged
merged 66 commits into from Aug 6, 2021
Merged

feat: ffi to replace plugins #11152

merged 66 commits into from Aug 6, 2021

Conversation

eliassjogreen
Copy link
Contributor

@eliassjogreen eliassjogreen commented Jun 27, 2021

This pr implements dynamic library ffi (Deno.dlopen and Deno.dlcall), removes the old plugins, an example and renames the allow-plugin permission to allow-ffi. There is still some things to be done, mainly support for more FFITypes like structs, tuples and variadics. Currently the ffi only supports calling foreign functions but some things such as viewing the value of pointers and static exported variables/symbols are not supported due to concerns with both security but mainly the whole thing segfaulting.

cc @bartlomieju

@bartlomieju
Copy link
Member

removes the old plugins, an example and renames the allow-plugin permission to allow-ffi

There's already #10908 which will remove native plugins, so I suggest to skip this part in your PR - it will be easier to review.

@eliassjogreen
Copy link
Contributor Author

Sure, do so it was just to make development a bit easier (not having to deal with the old plugin system permissions mainly).

@eliassjogreen
Copy link
Contributor Author

eliassjogreen commented Jun 27, 2021

Weird that it doesn't build on ubuntu, looks to be related to tov/libffi-rs#20 but on ubuntu (and WSL when I try it out locally) somehow? 🤔

@bnoordhuis
Copy link
Contributor

The error is this:

autogen.sh: 2: exec: autoreconf: not found

Meaning it's failing because autoconf isn't installed on the buildbot (autoreconf is part of autoconf.) You probably need to slot that in .github/workflows/ci.yml somewhere.

@caspervonb
Copy link
Contributor

Excellent, thanks for getting this going @eliassjogreen.

This is a bit low level compared to what I'd like to see, it's nearly there but passing type on each invocation seems like a very low level interface to give the user.

I had imagined it to be more of a top level object one would instantiate once with a descriptor, for example:

const library = new ForeignFunctionInterface.Library("lib.so", {
  symbols: {
    foo: { parameters: ["i32"], results: ["i32"]},
    bar: { parameters: ["i32", "i32"], results: ["i32"]},
  },
});

library.symbols.foo(1, 2);

For reference, the WebAssembly js api define an interface for typed functions:
https://github.com/WebAssembly/js-types/blob/master/proposals/js-types/Overview.md#addition-of-webassemblyfunction

Would be nice to keep close to this API where possible.

@bartlomieju bartlomieju mentioned this pull request Jun 28, 2021
23 tasks
@ry
Copy link
Member

ry commented Jun 29, 2021

@eliassjogreen With this FFI would you be able to implement Deno webview programs?

@rracariu
Copy link

How would an async call look like?

@eliassjogreen
Copy link
Contributor Author

I'll look into it but async ffi return types will probably not be supported as standard dynamic libraries dont have any future or async type.

Implementering webview should be more than doable and theoretically shouldn't even require any connecting rust or c code instead relying on the webview binaries provided by the main project.

@rracariu
Copy link

I'll look into it but async ffi return types will probably not be supported as standard dynamic libraries dont have any future or async type.

How about a special FFI Deno type that allows to return an async future? Meaning FFI would take care of doing the machinery for waiting for the future to be resolved, but in the C function you get an opaque type that you can resolve with a FFI Value later.

dlcall_args.args.into_iter().map(|arg| arg.into()).collect();

Ok(match dlcall_args.return_type {
FFIType::Void => json!(unsafe { cif.call::<()>(fn_code_ptr, &args) }),

Choose a reason for hiding this comment

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

Is serde_v8 a better option here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the json! macro automatically makes it into a serde_v8 serializable Value if I'm not mistaken

@andreubotella
Copy link
Contributor

andreubotella commented Jun 29, 2021

Yesterday I mentioned in Discord that it didn't seem right that the JS caller had to specify the function signatures – they should be specified in the library instead. (The WebAssembly.Function proposal @caspervonb mentioned above is for providing types for JS functions which are imported into wasm, rather than for exporting wasm functions, since those already have type signatures embedded in the wasm binary.)

My idea was to have some standard function that all Deno ffi libraries are expected to have, and that returns a list with every exported function and their signatures. Maybe something like this:

// Returns a 'static null-terminated list of 'static pointers.
export "C" fn deno_ffi_exports() -> *const [*const DenoFfiExport];

#[repr(C)]
struct DenoFfiExport {
  // 'static, null-terminated, UTF-8. The unmangled name of the function.
  name: *const char,
  args_num: usize,
  args: *const DenoFfiType,
  return_type: DenoFfiType
}

#[repr(int)]
enum DenoFfiType {
  Void => 0,
  U8,
  I8,
  U16,
  // ...
}

For a Rust ffi library, this description could then be auto-generated by a Rust crate, similar to what wasm-bindgen does. And we'd want to provide something similar for C(++) libraries.

@bartlomieju bartlomieju added this to the 1.12.0 milestone Jun 30, 2021
@eliassjogreen
Copy link
Contributor Author

Yesterday I mentioned in Discord that it didn't seem right that the JS caller had to specify the function signatures – they should be specified in the library instead. (The WebAssembly.Function proposal @caspervonb mentioned above is for providing types for JS functions which are imported into wasm, rather than for exporting wasm functions, since those already have type signatures embedded in the wasm binary.)

My idea was to have some standard function that all Deno ffi libraries are expected to have, and that returns a list with every exported function and their signatures. Maybe something like this:

// Returns a 'static null-terminated list of 'static pointers.
export "C" fn deno_ffi_exports() -> *const [*const DenoFfiExport];

#[repr(C)]
struct DenoFfiExport {
  // 'static, null-terminated, UTF-8. The unmangled name of the function.
  name: *const char,
  args_num: usize,
  args: *const DenoFfiType,
  return_type: DenoFfiType
}

#[repr(int)]
enum DenoFfiType {
  Void => 0,
  U8,
  I8,
  U16,
  // ...
}

For a Rust ffi library, this description could then be auto-generated by a Rust crate, similar to what wasm-bindgen does. And we'd want to provide something similar for C(++) libraries.

I don't believe this is such a good idea as it would not allow loading of dynamic libraries not intended for deno without recompiling. With the current approach of defining the types at runtime in deno (or at dlopen) we could load most dynamic libraries. Also doing it like this is also very similar to the old plugin system and not very compiler agnostic (how would someone do it from c, c++, zig, objc or whatever other language which compiles to a dynamic library).

@eliassjogreen
Copy link
Contributor Author

I'll look into it but async ffi return types will probably not be supported as standard dynamic libraries dont have any future or async type.

How about a special FFI Deno type that allows to return an async future? Meaning FFI would take care of doing the machinery for waiting for the future to be resolved, but in the C function you get an opaque type that you can resolve with a FFI Value later.

Supporting callback functions in the ffi would probably be easier and more language agnostic. The libffi crate already supports passing function closures to ffi calls, with this i could make it possible to pass js functions to ffi calls using an async op which resolves when it's time to call the closure. A less language agnostic option would be a crate like async-ffi which makea futures ffi compatible.

@manyuanrong
Copy link
Contributor

manyuanrong commented Jul 3, 2021

FYI: how to make CI compile pass, please refer to my previous PR: About FFI https://github.com/denoland/deno/pull/9173/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR140-R157

@manyuanrong
Copy link
Contributor

I'm very interested in this work, but unfortunately I didn't get any feedback on the PR I submitted before, until bartlomieju replied that it was being discussed internally😞

@ChayimFriedman2
Copy link
Contributor

Do you think that the --allow-ffi flag should accept a list of allowed libraries, like --allow-run?

@bartlomieju
Copy link
Member

Well strings (or even better, buffers) should be rather easy to implement and i started work on such a feature back at home (away on vacation until 6th of August). Implementing buffer support using zero copy buffers should be rather easy and useful for other things such as passing and returning references to structs (using something like denosaurs/byte_type) to ffi calls.

That sounds good, let's try to get strings/raw buffers working for 1.13 as well.

@rracariu
Copy link

rracariu commented Aug 2, 2021

I'll look into it but async ffi return types will probably not be supported as standard dynamic libraries dont have any future or async type.

How about a special FFI Deno type that allows to return an async future? Meaning FFI would take care of doing the machinery for waiting for the future to be resolved, but in the C function you get an opaque type that you can resolve with a FFI Value later.

Supporting callback functions in the ffi would probably be easier and more language agnostic. The libffi crate already supports passing function closures to ffi calls, with this i could make it possible to pass js functions to ffi calls using an async op which resolves when it's time to call the closure. A less language agnostic option would be a crate like async-ffi which makea futures ffi compatible.

IMHO this should be part of the first release also.

*
* Opens a dynamic library and registers symbols
*/
export function dlopen<S extends Record<string, ForeignFunction>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

ffiopen has a nice ring

@cztomsik
Copy link

cztomsik commented Aug 4, 2021

Well strings (or even better, buffers) should be rather easy to implement and i started work on such a feature back at home (away on vacation until 6th of August). Implementing buffer support using zero copy buffers should be rather easy and useful for other things such as passing and returning references to structs (using something like denosaurs/byte_type) to ffi calls.

That sounds good, let's try to get strings/raw buffers working for 1.13 as well.

@eliassjogreen I guess passing strings/buffers to ffi library might be easy but what if you do native plugin in rust and want to return a string - how would that work? I mean you have to free that string eventually and it has to be done from the native plugin again, right?

for example, let's say you have webview, you load the page and then you want to evaluate some JS code there and get the result (JSON stringified)

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 this will open a lot of possibilities for integration with other languages

Thank you very much @eliassjogreen for bringing this home and all other parties involved. Let's add support for passing strings in a follow up PR as this one is already huge.

@cztomsik
Copy link

FYI if you return u64::MAX from rust and then use dlopen() with { ..., result: u64 } you will get a different value in JS.

It does exactly the same thing if you input it to a deno console but that's probably unrelated:

Deno 1.14.0
exit using ctrl+d or close()
> 18446744073709551615
18446744073709552000
> 

Maybe this is expected behavior but it should be at least documented and I would say it's not terribly useful then for native plugins. Maybe it should return BigInt instead?

@bartlomieju
Copy link
Member

@cztomsik can you please open an issue with this report?

@cztomsik
Copy link

@bartlomieju sure #12212

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

Successfully merging this pull request may close these issues.

None yet