[Discussion] FFI - require('ffi') - libffi low-level binding

[TooTallNate]

This is the big boy for the ffi label, adding a new public core module: require('ffi'). This is what I consider to be the final piece of the puzzle (other pieces being #1750, #1759, and #1762).

I am the maintainer for the npm package, and while the APIs don't quite line up, I've always felt that the node-ffi API was too high level (doing things like dynamic library loading, and type boxing/unboxing), and what I have done in this PR is more along the lines of the direction I've wanted to take it.

So this is a good opportunity to get the low-level API correct/useful so that higher level facilities can be built on top (i.e. ctypes, ref's Type system, etc.) without needing to compile a native addon (this part is HUGE, especially considering the annoying V8 API changes in latest versions). Ideally we can abstract any V8 API changes away via this module and the npm userspace can contain code for bindings to C libs using the FFI api, which won't change often.

Most of the diff here is just adding a gypified libffi to the deps dir. That's not so useful, so see this more useful diff for the integration code: 1ebc059...TooTallNate:idea/libffi-bindings

A few things which I'd still like to figure out (and would like a comment on if anybody has a particular preference):

• We need to compile some native code for the tests (i.e. to expose static function pointer Buffers to pass to ffi.call(), to invoke a C function pointer to test the ffi.Closure stuff). We could add a new process.binding('ffi_tests') internal binding that would only be loaded during the tests. Or another possibility would be for these ffi tests to rely on the dlopen module, and we can piggyback off of the shared library that gets loaded for the dlopen tests. Either way sounds good to me.
• The async ffi.callAsync() function is not yet implemented. Working on that still though considering Prototype async node-ffi hardware#9. 81f9b88
• Docs still needed as well.
• A helper for creating "struct" ffi_type instances.
• A "variadic" CIF interface (right now ffi.CIF only expects to describe fixed-length functions, but there is a variadic alternative: we just need a parameter to specify the number of fixed arguments vs. variable arguments).

[TooTallNate]

Proposed FFI function call example:

var ffi = require('ffi');
var dlopen = require('dlopen');
var endianness = require('os').endianness();

var libc = dlopen.dlopen('libc.dylib');
var absPtr = dlopen.dlsym(libc, 'abs')['readPointer' + endianness](0);

var cif = ffi.CIF(ffi.type.int, ffi.type.int);

// rvalue must point to storage that is sizeof(long) or larger.
// For smaller return value sizes, the ffi_arg or ffi_sarg
// integral type must be used to hold the return value.
var rValue = new Buffer(Math.max(ffi.sizeof.int, ffi.sizeof.ffi_sarg));

// array of pointers to arguments for `abs()`
var aValues = new Buffer(ffi.sizeof.pointer * 1);

// storage space for the single argument to `abs()`
var arg = new Buffer(ffi.sizeof.int);
aValues['writePointer' + endianness](arg, 0);

// abs(-5)
arg['writeInt32' + endianness](-5, 0);
ffi.call(cif, absPtr, rValue, aValues);
console.log('abs(-5): %j', rValue['readInt32' + endianness](0));

// abs(3)
arg['writeInt32' + endianness](3, 0);
ffi.call(cif, absPtr, rValue, aValues);
console.log('abs(3): %j', rValue['readInt32' + endianness](0));

dlopen.dlclose(libc);

Proposed FFI Closure example:

var ffi = require('ffi');
var endianness = require('os').endianness();

var cif = ffi.CIF(ffi.type.int, ffi.type.int);

// ffi.Closure
var ptr = ffi.Closure(cif, function (ret, args) {
  // args is a Buffer, an array of pointers to the arguments passed
  //   length = sizeof(char*) * cif->nargs;
  var arg = args
    ['readPointer' + endianness](0, ffi.sizeof.int)
    ['readInt32' + endianness](0);

  console.log('args[0]:', arg);

  // ret is a Buffer, with enough space for the desired return value to be written
  //   length = MAX(cif->rtype->size, sizeof(ffi_arg))
  ret['writeInt32' + endianness](-123, 0);
});

// ptr is a Buffer instance pointing to the executable C function pointer.
// pass it to C functions that expect C functions as arguments, etc.

[Qard]

Nice! I'd definitely love to have FFI in core.

I'm leaving myself a note to review this later. :)

[mscdex]

This is a bit unrelated, but perhaps we could add some Buffer methods like buffer.readInt32() that would call the right method depending on os.endianness(), that way we can avoid having to do buf['writeInt32' + endianness]() and such everywhere?

[TooTallNate]

@mscdex I'm 👍 for that, however there's already readIntLE() and readIntBE() that read an int of an arbitrary size (and the unsigned, and write() equivalents), so there's a potential naming conflict there.

[mscdex]

@mscdex I'm for that, however there's already readIntLE() and readIntBE() that read an int of an arbitrary size (and the unsigned, and write() equivalents), so there's a potential naming conflict there.

There wouldn't be a naming conflict since all methods currently have BE or LE in them. The methods I suggested would have the same names, minus the endianness indicator.

[TooTallNate]

Oh right, silly Nate. Still, it could be strange having readInt() have different functionality than readIntBE().

[mscdex]

I was imagining that the new methods would be added like:

var endianness = require('os').endianness;

// ...

Buffer.prototype.readInt = Buffer.prototype['readInt' + endianness];
Buffer.prototype.readUInt = Buffer.prototype['readUInt' + endianness];
Buffer.prototype.readInt8 = Buffer.prototype['readInt8' + endianness];
Buffer.prototype.readUInt8 = Buffer.prototype['readUInt8' + endianness];
// ...

For example, if the host system was little endian, then buffer.readInt === buffer.readIntLE.

[TooTallNate]

The generic "int" one would leverage the sizeof mapping ideally. We need the readInt32() version, rather than the arbitrary-bytesize function 😉

Buffer.prototype.readInt = Buffer.prototype['readInt' + (sizeof.int * 8) + endianness];
Buffer.prototype.readUInt = Buffer.prototype['readUInt' + (sizeof.uint * 8) + endianness];

[mikeal]

if we name the core module ffi don't we break usage of the npm module?

[TooTallNate]

if we name the core module ffi don't we break usage of the npm module?

We will, but I'm personally ok with this (I'm the maintainer of that module). The npm module is too high-level for what a focused ffi module should be. What I have in this PR is more along the lines of what I'd imagine a clean ffi module would look like: low-level, agnostic of abstractions and ready to be abstracted on top of.

What node-ffi is today could more appropriately be called ref-ffi (or reffi 😛), since it's centralized around ref's Type system.

If we wanted to help out the old node-ffi users upgrading to io.js, we could add dummy, deprecated ffi.Library and ffi.ForeignFunction functions that throw an Error an link to a migration guide. Not sure how necessary that part is, but if we think it's a good idea then I'm ok with that.

[brendanashworth]

... and we're back (my fault):

@kobalicek nodejs/NG#3 (comment):

Am I the only one skeptical about this module here? :)

I don't like putting something like ffi into the core. I think it's unsafe, it will probably never be stable, and I personally consider it as a workaround, not a solution. Let's be honest here - the biggest problem in native modules development is not a C++ code, it's breaking changes in V8 that ruin all the effort and turn into maintenance nightmares. Solutions like NaN provide short term stability, but we will have soon NaN2, because the latest changes in V8 are so drastic that it cannot be simply solved by more macros and templates in NaN.

Now let's go back to FFI, I have the following concerns:

It's unsafe - Creating bindings usually mean to maintain some data structures, arrays, etc. FFI requires basically to duplicate many things that are already in C header files. If someone does a wrong job here or new version of some library changes something it won't be visible before FFI runs, it will crash node in the best case. Also one point here - it will allow any module to crash the process, even on purpose.

It solves only some use cases when dealing with C code. It cannot be used to bind C++ code anyway, and a lot of libraries are in C++. I guess windows users would be the happiest here as it will enable to call winapi functions without going native.

It's very slow. I think node should focus on high performance and not add something inherently slow into the core.

It will be really easy to create sync bindings. I think node should try to avoid this as much as possible.

Guys these are my 2 cents. I have nothing against having this library as a module, but I really think it shouldn't go to core.

me: nodejs/NG#3 (comment)

.. the biggest problem in native modules development is not a C++ code, it's breaking changes in V8 that ruin all the effort and turn into maintenance nightmares.

I agree! However I see FFI in core as a solution to this (not as a workaround as you suggest). If we can provide a stable FFI module that doesn't force users to go through the v8 API, it will allow us to foster a stable native ecosystem where we couldn't have before. It also reduces the C++ tyranny of native modules - to people who don't like C++ (like me), it can be very daunting to write a native module, instead allowing you to choose whichever language you prefer, like Rust.

It's very slow.

I haven't done any benchmarking, but I can't see it being significantly slower than v8 bindings, which are already slower than regular JavaScript calls.

It will be really easy to create sync bindings.

Yes it is easy, but it does look like it is even easier to make async calls with ffi than it is to make them with C++.

I don't know about security, but that seems like it should be discussed.

edit: seems like the calls are significantly slower than native bindings.

[kobalicek]

I think --no-ffi flag would make it even worse by breaking modules that depend on ffi so nobody will use it in practice.

[jbergstroem]

@kobalicek that could apply to things like --without-ssl as well.

[bnoordhuis]

@kobalicek I don't disagree with your observations, just the conclusions. :-)

Also one point here - it will allow any module to crash the process, even on purpose.

That isn't any different from the status quo. You can already do ersatz FFI by writing a .node file to disk, then loading that with require().

I did that in a talk a while back: construct an ELF header, assemble some machine code, then load it into memory. The system administrators in the audience went visibly pale. :-)

It solves only some use cases when dealing with C code. It cannot be used to bind C++ code anyway, and a lot of libraries are in C++.

That's not quite true - you can call the mangled symbol with this as the first argument - but apart from that, most libraries provide C bindings.

It won't work for all C++ libraries but FFI doesn't have to cover all use cases, just the most common ones.

It's very slow.

Blanket statement. It doesn't have to be, it depends on the implementation.

It will be really easy to create sync bindings.

If people want to do that, that's fine by me. Core has no business telling people how they should write their code.

I somewhat agree with the sentiment that the FFI module shouldn't go into core, but I think core should have the hooks to make writing one possible without needing a compiler (i.e. without having to use a native add-on.)

[kobalicek]

@bnoordhuis Unless there is an implementation that is not slow the statement is pretty valid :) But my main concern is not really the performance side (even sync issue, I don't care), I have only concerns about security.

I think there is not much more I can contribute here.

[kobalicek]

@jbergstroem It would be off topic to start discussing other command line options I think.

[kobalicek]

Actually I think we should consider option --with-ffi - That would probably satisfy everybody, what you guys think?

[mikeal]

There's been a lot of focus on the performance aspect of FFI. While many modules, especially the current popular native modules, performance is key and they probably won't be moving to FFI. However, there are many cases where the reason someone wants to use FFI is because of its ease of use and the effort required in implementing a C/C++ modules again in JavaScript. This is a completely valid use case, and one that we should support, but while acknowledging that this doesn't replace native modules or solve many of the issues we have related to the native ecosystem and this feature addition shouldn't be judged on those terms.

[jbergstroem]

@kobalicek My point was that making assumptions about how iojs is built is a bad approach.

[kobalicek]

@mikeal Agreed with that, however, I have to ask, why such feature should be in core? I mean what is inherently wrong on having a package ffi on npm?

[TooTallNate]

@kobalicek The benefit of having an ffi interface in core is that no native compilation toolchain needs to be set up on the end users' computers. Currently when you do npm install ffi you have to compile the libffi stuff and binding code, which is what we would avoid here.

This is especially helpful for Windows users - just take a look at the number of issues related to MSVS installations in the node-gyp issue tracker.

[kobalicek]

@TooTallNate But this can be applied to any package that provides C++ bindings. Additionally, I don't really see how ffi in core would fix these broken packages.

I think improving documentation for making C++ addons and also improving documentation related to gyp would be better long-term than adding more packages into the core. But I'm giving up, I won't maintain it :-)

[justinmchase]

I 👍 this proposal. This seems like it should be a core api.

[Qix-]

Poking this proposal. This'd be huge for the Node.js community.

[sindresorhus]

👍 This is sorely needed. I often have the need to interact with native API's in reusable modules, but I want to refrain from depending on native Node.js addons as they cause all kinds of problems. My use-cases aren't very performance sensitive either. Currently, I just spawn a precompiled binary for each OS, but would be much nicer to be able to use FFI.

[amb26]

+1 from me - the version of Visual Studio required to build any native modules on Windows is absolutely gigantic, and becomes a requirement for anyone who even wants to run npm install on the majority of projects out there. Putting node-ffi into core will ease this enormous pain point for thousands of people. This is the answer to people who say "I don't see what is wrong with having an ffi module in npm". Also, the enablement of ffi should be the default - meaning that then the same install instructions will work across platforms. -1 to the "--with-ffi" proposal.

The performance question is a non-issue. It's the job of a person requiring a dependency to determine whether its performance meets their requirements. If you're not happy with the performance of ffi, don't use it, and don't use a dependency that uses it. It imposes no penalty on code which doesn't use it for binding.

[Qix-]

@TooTallNate any chance of rebasing this PR on head? 👍

[kobalicek]

Are these even arguments?

[TooTallNate]

@Qix- Definitely, though it would be worth landing the necessary smaller PR bits first (which also I'm sure need rebasing at this point).

[aayushkapoor206]

👍

[jasnell]

Closing given the lack of activity and response. This is likely something that should still be looked at tho.