[Discussion] FFI - Giving Buffer more low-level C functionality #1750

TooTallNate · 2015-05-20T19:31:13Z

This is a PR, with the intention of starting/continuing the discussion about bringing FFI into node/iojs core.

Overall, the biggest benefit to integrating an FFI interface to node-core is that we can avoid C++ compiled addons, for the common case (where extreme performance requirements would be the uncommon case). No more compiling is huge for the UX, especially on Windows.

As of now, this PR adds some low-level functions to the Buffer object. I believe that these are the building blocks necessary to remove the C++ compiled portion of ref, and leaves room to implement other kinds of native interfaces (we could build js-ctypes API on top of these low-level functions as well… I'm planning on doing exactly that if this gains momentum!)

New Buffer APIs

`readInt64BE()`, `readInt64LE()`, `readUInt64BE()`, and `readUInt64LE()`

Reads an (u)int64 value from the Buffer. Similar to the other read* functions, but if the int64 value falls outside the JS Number max value (±9007199254740992) then a String will be returned instead of a Number. This falls in-line with most Int64 library APIs, including the ctypes Int64 constructor, which accepts either a Number or String.

`writeInt64BE()`, `writeInt64LE()`, `writeUInt64BE()`, and `writeUInt64LE()`

Complimentary to the read* functions, these allow int64 range integers to be written to Buffer instances. A JS Number may be passed in, and it will be written accordingly. Alternatively, a JS String may be passed in and it will be converted to an (u)int64 via strtoll/strtoull and then written to the Buffer instance (there would probably be value in accepting a Buffer instance that already has a (u)int64 written to it, but that's not currently implemented).

`readPointerBE()` and `readPointerLE()`

These functions create a Buffer (pointer) instance from the memory address written to the parent buffer. This is essentially the "dereference" operator in C. Very important for interacting with C libraries via FFI!

Not implemented at the moment, but we could add an optional callback function here that would be invoked during the Buffer free callback, which the user could hook on to to free allocated memory from a C library, for example. This is easy to add if there's no objections.

`writePointerBE()` and `writePointerLE()`

The compliment to the readPointer* functions. Essentially the "reference" operator in C. Allows the user to write the memory address of one Buffer instance to an offset in another buffer.

`address()`

Returns a String representation of the hex address of the pointer in memory.

`inspect()`

The REPL inspect() function for Buffer gets a cute little update as well thanks to the new address() function. We include the hex pointer address after the constructor name:

> new Buffer(8)
<Buffer@0x10187aa08 00 20 00 00 00 00 00 00>

Missing pieces

So like mentioned above, this is really just the low-level C functionality that we need added as methods to Buffer. There's still some new interfaces we'll need to expose in order to implement a fully-featured FFI interface, or implement js-ctypes. Perhaps we can hash out the details of these in this thread as well:

~~sizeof mapping for standard types (int, double, etc.), for allocing Buffer instances of the correct size. Perhaps require('os').sizeof?~~ See [Discussion] FFI - export os.sizeof and os.alignof mappings #1759.
~~-alignof mapping for standard types (int, double, etc.), for creating structs, etc. Perhaps require('os').alignof?~~ See [Discussion] FFI - export os.sizeof and os.alignof mappings #1759.
~~dynamic library loader (uv_dlopen() and friends, but exposed to JS). Perhaps require('_dl') or something? There is the dlopen package which we could get inspiration from.~~ See [Discussion] FFI - require('dlopen') - libuv dynamic library loader binding #1762. Additionally, this could allow for process.dlopen() (the .node module loader) to be re-written in 90% JavaScript, which would be kind of cool.
low-level libffi bindings (bundle the gypified libffi into the node binary, expose a very basic JS interface). Perhaps require('_ffi')? node-ffi is great, but it's a bit too high-level for core IMO. It aims to be a more complete FFI solution, a la ctypes, rather than simple low-level bindings in which ctypes could be implemented on top of.. See [Discussion] FFI - require('ffi') - libffi low-level binding #1865

(not sure about the _ prefix on ffi and dl... just wanted to be conscious about not trampling over the existing npm package names)

Obviously, we'll need to compliment this with good docs and tests (some of which I could pull from ref most likely) before it's ready for merge. I just wanted to get this PR open first to see if there's momentum, and other comments/adjustments that will be needed to be addressed as well. Let's get the 🔮 rolling!

Thanks for reading. Cheers! 🍻

/cc @bnoordhuis @piscisaureus @rvagg @indutny @isaacs @kkoopa @trevnorris @rauchg @saghul @tracker1 @nodejs/build et al.

mscdex · 2015-05-20T19:39:30Z

src/node_buffer.cc

+// or String, based on whether or not a JS Number will lose precision.
+// http://stackoverflow.com/q/307179/376773
+#define JS_MAX_INT +9007199254740992LL
+#define JS_MIN_INT -9007199254740992LL


Both of these should be 9007199254740991 instead, since var n = 9007199254740992; console.log(n + 1); will output 9007199254740992 also.

ditto to this. Number.{MAX,MIN}_SAFE_INTEGER return the same as @mscdex suggested.

mscdex · 2015-05-20T19:41:07Z

I'm not particularly a fan of 'return a string or number depending on the value' for the read*() 64-bit integer functions. Keeping to one type avoids having to do conditionals everywhere.

TooTallNate · 2015-05-20T19:45:44Z

Keeping to one type avoids having to do conditionals everywhere.

The idea is that usually you'd be passing the return value to an Int64 JS implementation, for example the ctypes Int64 constructor or node-int64 or node-bigint, etc., which accept both numbers and strings.

kkoopa · 2015-05-20T20:20:38Z

Buffer will be completely rewritten based on typed arrays due to #1451.

trevnorris · 2015-05-20T20:40:10Z

lib/buffer.js

@@ -422,6 +426,16 @@ Buffer.prototype.fill = function fill(val, start, end) {
 };


+Buffer.prototype.address = function address() {
+ return binding.address(this);


Should have preliminary checks here that check that this is a Buffer.

As a note, both this check and the HasInstance() checks are necessary. Because the object must be an instance of Buffer, and it must contain external array data.

JS-side instanceof Buffer checks seem to current only happen in the write*() functions.
fill(), slice(), toString(), etc. don't seem to have these checks. Is that intentional?

No. They should in fact have these checks.

EDIT: I'll take care of the other cases in a different PR. Thanks for bringing them up.

trevnorris · 2015-05-20T20:52:00Z

Pointed out a couple things, but I believe more discussion will need to be had about how this fits in with the soon upcoming massive internal change to Buffer.

chrisdickinson · 2015-05-20T22:02:22Z

cc'ing @feross into this thread, since he maintains a browser compatibility shim for buffers and may have thoughts!

bnoordhuis · 2015-05-22T14:22:20Z

src/node_buffer.cc

+
+#ifdef _WIN32
+ #define __alignof__ __alignof
+ #define snprintf(buf, bufSize, format, arg) _snprintf_s(buf, bufSize, _TRUNCATE, format, arg)


Also seems to be completely unnecessary. Whoops! Removing.

The errno EINVAL behavior is a BSDism, and won't work cross platform. Instead, pass in the char* endptr value and check where it ended up.

TooTallNate · 2015-06-05T00:54:02Z

Pointer-related functions removed; will be moved to the ffi module in #1865.

strto(u)ll() usage updated to use an endptr variable, and base 10 radix as per @bnoordhuis' requests.

I think this one is ready for squashing!

trevnorris · 2015-06-05T01:43:44Z

@TooTallNate Great work. Glad to see the 64 bit operations added. Though I would like to see the Buffer address removed from the console output. The hex values from multiple buffers will be misaligned if address length is different.

@rvagg @bnoordhuis Does .address() fit under the same scope of pointer concerns that was discussed, or is the information benign enough that it doesn't present any other security concern?

On the side, I'd like to handle merging all buffer changes from master to next. Can that be done at any time, or should wait for the next release that includes these changes?

trevnorris · 2015-06-05T01:48:33Z

src/node_buffer.cc

+ const char* formatter) {
+ Environment* env = Environment::GetCurrent(args);
+
+ ARGS_THIS(args[0].As<Object>());


Need to check CHECK(HasInstance(args[0])); before doing this. It's easy enough to manipulate the prototype from JS to allow bypassing the JS check and then fatally error here. I don't mind the abort that CHECK() would cause because identifying the issue is much easier.

trevnorris · 2015-06-05T02:00:37Z

@TooTallNate Think it's a usability issue that I couldn't b.writeUint64LE(b.address())? Doesn't concern me either way, but I see people wanting to potentially do this.

Also, I'll do a follow up PR adding benchmarks for the new read/write methods. Been meaning to give those some love anyway, so don't worry about that here.

TooTallNate · 2015-06-05T02:11:45Z

Think it's a usability issue that I couldn't b.writeUint64LE(b.address())? Doesn't concern me either way, but I see people wanting to potentially do this.

Without being able to readPointer() the memory address back out to a new Buffer instance, I don't think it's exposing much.

jasnell · 2015-11-16T04:31:02Z

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

aayushkapoor206 · 2016-01-21T14:37:03Z

👍

tracker1 · 2016-01-21T20:16:30Z

Not seeing anything recent in this thread.. has there been any continued effort towards this?

TooTallNate · 2016-01-21T21:21:36Z

I'm still here and can rebase, etc. but I think I need another core committer to champion this effort or I'm afraid it'll code-rot again.

tracker1 · 2016-01-22T20:52:58Z

@TooTallNate thanks... honestly, I wish I were more skilled with this level of programming, as I feel getting ffi mainlined would benefit node a lot. IMHO It should have been a core feature much earlier on.

trevnorris · 2016-01-22T23:55:41Z

@TooTallNate Because of the size of this feature, I may suggest writing a proposal in https://github.com/nodejs/node-eps. If it gets hammered out there and is merged I'll volunteer to help get the patch into core.

jasnell · 2016-03-22T04:37:16Z

Marking as stalled. If there's no action on this one in the next few weeks I recommend closing

jasnell · 2016-04-04T22:07:05Z

Closing due to lack of further activity. @TooTallNate ... this is likely something that would still be worthwhile. As @trevnorris has discussed, however, using nodejs/node-eps would likely be the better forum to get things moving.

seishun · 2017-08-31T11:47:35Z

@jasnell I would like to add {read|write}[U]Int64{LE|BE} methods to Buffer, which would basically be a small subset of this PR. Does that require a node-eps proposal?

mscdex reviewed May 20, 2015
View reviewed changes

TooTallNate mentioned this pull request May 20, 2015

ref.alloc ignores alignment requirements TooTallNate/ref#28

Closed

trevnorris reviewed May 20, 2015
View reviewed changes

Fishrock123 added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks. labels May 20, 2015

TooTallNate changed the title ~~[Idea/Discussion] Giving Buffer more low-level C functionality~~ [Discussion] FFI - Giving Buffer more low-level C functionality May 21, 2015

TooTallNate added the ffi label May 21, 2015

This was referenced May 21, 2015

[Discussion] FFI - export os.sizeof and os.alignof mappings #1759

Closed

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

Closed

TooTallNate added the semver-minor PRs that contain new features and should be released in the next minor version. label May 22, 2015

bnoordhuis reviewed May 22, 2015
View reviewed changes

buffer: make strto(u)ll usage cross-platform

ce00983

The errno EINVAL behavior is a BSDism, and won't work cross platform. Instead, pass in the char* endptr value and check where it ended up.

trevnorris reviewed Jun 5, 2015
View reviewed changes

Fishrock123 mentioned this pull request Jun 15, 2015

Native FFI nodejs/NG#3

Open

rvagg force-pushed the master branch from 9d21135 to 628a3ab Compare June 25, 2015 09:18

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55

am11 mentioned this pull request Nov 2, 2015

Windows users are not happy. nodejs/node-gyp#629

Closed

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016

jasnell closed this Apr 4, 2016

seishun mentioned this pull request Sep 2, 2017

buffer: add {read|write}[U]Int64{BE|LE} methods #15152

Closed

4 tasks

mnpenner mentioned this pull request Jul 27, 2018

Feature request: Buffer.readUint(BE|LE) with BigInt support #21662

Closed

Trott mentioned this pull request Oct 11, 2021

(unintended?) npm@8 breaking change: return status different for npm outdated #40382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discussion] FFI - Giving Buffer more low-level C functionality #1750

[Discussion] FFI - Giving Buffer more low-level C functionality #1750

TooTallNate commented May 20, 2015

mscdex May 20, 2015

trevnorris May 20, 2015

mscdex commented May 20, 2015

TooTallNate commented May 20, 2015

kkoopa commented May 20, 2015

trevnorris May 20, 2015

trevnorris May 20, 2015

TooTallNate May 21, 2015

trevnorris May 24, 2015

trevnorris commented May 20, 2015

chrisdickinson commented May 20, 2015

bnoordhuis May 22, 2015

TooTallNate May 22, 2015

TooTallNate commented Jun 5, 2015

trevnorris commented Jun 5, 2015

trevnorris Jun 5, 2015

trevnorris commented Jun 5, 2015

TooTallNate commented Jun 5, 2015

jasnell commented Nov 16, 2015

aayushkapoor206 commented Jan 21, 2016

tracker1 commented Jan 21, 2016

TooTallNate commented Jan 21, 2016

tracker1 commented Jan 22, 2016

trevnorris commented Jan 22, 2016

jasnell commented Mar 22, 2016

jasnell commented Apr 4, 2016

seishun commented Aug 31, 2017 •

edited

Loading

[Discussion] FFI - Giving Buffer more low-level C functionality #1750

[Discussion] FFI - Giving Buffer more low-level C functionality #1750

Conversation

TooTallNate commented May 20, 2015

New Buffer APIs

readInt64BE(), readInt64LE(), readUInt64BE(), and readUInt64LE()

writeInt64BE(), writeInt64LE(), writeUInt64BE(), and writeUInt64LE()

readPointerBE() and readPointerLE()

writePointerBE() and writePointerLE()

address()

inspect()

Missing pieces

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented May 20, 2015

TooTallNate commented May 20, 2015

kkoopa commented May 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented May 20, 2015

chrisdickinson commented May 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TooTallNate commented Jun 5, 2015

trevnorris commented Jun 5, 2015

Choose a reason for hiding this comment

trevnorris commented Jun 5, 2015

TooTallNate commented Jun 5, 2015

jasnell commented Nov 16, 2015

aayushkapoor206 commented Jan 21, 2016

tracker1 commented Jan 21, 2016

TooTallNate commented Jan 21, 2016

tracker1 commented Jan 22, 2016

trevnorris commented Jan 22, 2016

jasnell commented Mar 22, 2016

jasnell commented Apr 4, 2016

seishun commented Aug 31, 2017 • edited Loading

`readInt64BE()`, `readInt64LE()`, `readUInt64BE()`, and `readUInt64LE()`

`writeInt64BE()`, `writeInt64LE()`, `writeUInt64BE()`, and `writeUInt64LE()`

`readPointerBE()` and `readPointerLE()`

`writePointerBE()` and `writePointerLE()`

`address()`

`inspect()`

seishun commented Aug 31, 2017 •

edited

Loading