[Discussion] FFI - require('dlopen') - libuv dynamic library loader binding

[TooTallNate]

This is a continuation of #1750 and #1759.

This PR adds a new public module: require('dlopen'). I'm the owner of the current npm module, and I'm fine with relinquishing control of it in favor of allowing core to use the name (I'm open to alternative names as well).

The current impl is very close to a 1-1 mapping of the C uv_dlopen() and friends API, with very minimal JS wrapping to make the API more JS-friendly. We could also/instead do an object-oriented Library class API if there is consensus that that would be more node-like.

Basic usage looks something like:

var dl = require('dlopen');

var lib = dl.dlopen('libc.dylib');
// lib is a Buffer instance, the `uv_lib_t *` struct

var printfPtr = dl.dlsym(lib, 'printf');
// pass the printf function pointer to FFI, etc.

// ... some time later... we're done with `lib`
dl.dlclose(lib);

Similar to the other PRs, this still needs docs and tests. Mostly just looking for preliminary feedback/comments while I get those in.

[petkaantonov]

I don't think what are essentially raw pointers should be exposed to JS like this. Maybe just have some fieldless object only used for its identity that can be mapped back to pointer in C++.

For example:

Local<FunctionTemplate> t = FunctionTemplate::New(env->isolate());
t->InstanceTemplate()->SetInternalFieldCount(1);
// For '[object Library]' .toString behavior.
t->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Library"));

Then you can later create objects like:

Local<Object> pointer_holder = t->GetFunction()->NewInstance();
pointer_holder->SetAlignedPointerInInternalField(0, ptr); // Or nullptr if you don't have ptr yet

You cannot access them in js but you can later get it back in C++:

void* ptr = pointer_holder->GetAlignedPointerFromInternalField(0);

[bnoordhuis]

The HandleScope isn't necessary (here and elsewhere.)

[TooTallNate]

@petkaantonov If you as a user are worried about accessing the contents within the Buffer, then set its size to 0. This is how require('buffer').NULL will work in #1750, and you can not access its contents (since doing so would cause a segfault).

Essentially, these are just pointers. Though I'm definitely aware of the fact that we're going to need to be very explicit about the unsafe nature of messing with these APIs unless you know what you're doing, once the documentation part comes in.

[TooTallNate]

@bnoordhuis Nits fixed (hopefully 😄) on this one.

[TooTallNate]

@petkaantonov I'll add that it's important for users to have access within the contents of the pointer. Consider a C lib that exports an int.

#if defined(WIN32) || defined(_WIN32)
#define EXPORT __declspec(dllexport)
#else
#define EXPORT
#endif

EXPORT int six = 6;

With this new dlopen module, we can open up this lib, get the six symbol pointer, and then read out the integer value, all in JS:

var lib = dl.dlopen('libfoo.so');
var sixSym = dl.dlsym(lib, 'six');

sixSym.readPointer(0, os.sizeof.int).readInt32LE(0);
// 6

^ @bnoordhuis This also answers your question in #1750 regarding when you would ever use a value other than sizeof(void *) for the length (perhaps the name readPointer() ends up being misleading?)

[trevnorris]

This needs to be reconsidered because of the upcoming V8 changes. Here you'd essentially be using a Uint8Array(). Problem is that with the new implementation we are going to allow V8'd GC to handle this automatically, and there's no way to use conditional logic to make the typed array external and persistent on instantiation. Best you might be able to do is pass the new instance to another function that does this for you.

But for performance an simplifying code complexity I would instead suggest returning a Persistent<External>.

[TooTallNate]

Sorry, I'm obviously not getting something here. What exactly is the implied change here from the switch to Uint8Array? As long as the C++ layer can still unwrap to the bare char * then I think it shouldn't really matter. The GC handled Buffers before as well so I'm not sure what the change here will be.

Also this has basically been the recommended way of allocating memory for structs/etc. in node-ffi for years now, so I would hate for that to be an issue 😀

[trevnorris]

Also this has basically been the recommended way of allocating memory for structs/etc. in node-ffi for years now, so I would hate for that to be an issue

This hinges on whether you're alright with V8 cleaning up the pointer, or if you'd like manual control?

[TooTallNate]

Calling new Buffer() has always implied V8/node cleaning up the memory.

[trevnorris]

I remember you requested we allow a native module to supply the weak callback. Is that correct?

[TooTallNate]

Sorry man, still not sure what you're driving at.

Are you worried about automating dlclose() with when lib get GC'd? I'm not so worried about that, I consider that the user's responsibility.

[trevnorris]

I'm concerned with making sure the new Buffer implementation meets all your needs. When Buffer moves to Uint8Array it will not have a weak callback available. Anyone who wishes to use one will have to externalize the object and setup the weak callback on their own.

I'm trying to recall a conversation we had 3+ years ago when I was doing my first rewrite of Buffer. I vaguely recall that for certain cases you wanted access to the weak callback because the lifetime of the pointer might have been different than that of the Object.

[trevnorris]

@TooTallNate

If you as a user are worried about accessing the contents within the Buffer, then set its size to 0.

Do you mean just changing buffer.length = 0?

[TooTallNate]

@trevnorris

Do you mean just changing buffer.length = 0?

Not quite. I meant passing 0 to the "length" parameter on readPointer(offset, length).

[TooTallNate]

This one is basically at the point where we need #1750 (readPointer*() specifically) in order to write proper tests.

[TooTallNate]

Was able to add some initial tests, though they're failing until #1750 is merged.

[jasnell]

@TooTallNate @trevnorris ... any updates on this?

[jasnell]

Closing given the lack of activity or response. This is likely still something that should be looked at more tho. /cc @jbergstroem